\Drupal\openapi\Controller\OpenApiController covers the route callback and the actual generation of OpenAPI output

This should be refactored so that it is easier to generate OpenAPI output in different cases.

Idea:

OpenApiDownloadsController
uses a OpenApiGeneratorInterface to generate output

OpenApiGeneratorInterface
setSerializer

OpenApiRestGeneratorInterface extends OpenApiGeneratorInterface
setEntityTypes
setBundle

OpenApiJsonAPIGeneratorInterface extends OpenApiGeneratorInterface

Plugins?

Should these be plugins so that other modules can define?

Output per serializer?

As part of this there OpenApi should be generated for 1 serializer at a time. For example for core REST it really doesn't make sense to make 1 OpenApi json file for both json and hal_json serializers. This because paths portion must either include or point to a schema definition. This is the portion that will come schemata in #2870393: Remove openapi_json_schema submodule and add dependency on Schemata module. Of the schema would be different depending on hal_json or json.

This will mean there will a different OpenApi export and docs for REST json and REST hal_json . While this is not be ideal I don't see another way to document resources that intersperse different serializers in OpenApi spec. My guess it is edge that site would need this.

Include entities and non-entities in single output

Currently there is different output for ReST resources that don't relate to entities. They should probably be one output.

CommentFileSizeAuthor
#11 2874877-interdiff-7-11.txt2.73 KBrichgerdes
#11 2874877-11.patch122.72 KBrichgerdes
#7 2874877-7.patch39.1 KBtedbow
#7 interdiff-5-7.txt18.08 KBtedbow
#5 2874877-5-with_renames.patch40.91 KBtedbow
#4 2874877-4-refactor-openapicontroller.patch117.21 KBrichgerdes

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue summary: View changes
richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented

Here is one approach to solving this,

I think we could refactor the OpenApiGenerators into a plugin. This would allow projects such as GraphQL or Rest Views (#2875213: Add Compatibility with REST View exports) to supply their own open api schema, without us having to maintain integration with those projects. This would allow us to have a standard controller, and move the logic into the generators.

Refactoring the generators as plugins does bring up an issue. Currently OpenAPI dynamically sets the routes for these controllers only if the dependent projects are installed. With plugins that would happen automatically through routes, however we would not be able to supply those plugins with this module, they would need to be shipped as part of separate modules in order to not force the dependencies on the base openapi module. For jsonapi, thats an easy options, as its a contrib project, and we could have them include the plugin. For Rest, we would probably just provide an additional sub module to this project that provides the desired function.

For addressing #2894623: Generate a spec for non entity service, hopefully that can be handled as part of the plugin design. I think the main challenge there is determining the result schema for the paths, but nothing should be preventing us from providing that data as part of the schema.

For addressing the different types of schema, I think this could be done in a few different ways. The OpenApi Specs, do address this case for XML. Properties can provide an xml block in schema to designate additionally meta data for the result when displayed as xml. We could provide something similar based upon the different sterilizers (x-hal-json or x-json) to designate different properties being only available in one or the other. Idk if that's the best way, but it might solve some of our issues there and not require multiple output files. It of course does make the rest generator more complicated.

We may also be able to implement the Swagger And ReDoc sub modules around a plugin as well. This would allow the potential for adding things such as #2874897: Determine how to prepopulate session key for X-CSRF token and #2874891: Allow filtering doc pages per resource to hopefully fall in place easily and effect the plugins more broadly.

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2874877-4-refactor-openapicontroller.patch117.21 KB

I took a first pass at this. See the attached patch.

At this point, the patch takes the existing controllers and generators and converts them into a unified controller and a generator plugin system. In the plugin manager I currently am conditionally creating the plugins only if the corresponding module dependencies, rest and jsonapi, are installed. I did that this way to preserve the existing module functionality as a first patch. Paragraph two in #3 was under the assumption that this would not work, and that having the plugins as part of the base module would cause exceptions. This is not an issue as long as the classes are not instantiated, which the logic prevents.

Per further discussion on slack with @tedbow, we planned to factor the plugins out into their own sub modules and and make the openapi module a hidden library type project. This would make the module list a little cluttered by listing extra projects just to provide the plugins, but it provides a better out of the box experience for anyone using the module since they would have to install rest, json, or another project that provides functions. As a downside though, the swagger and reDoc sub modules could be enabled and when neither the json or rest module is and this would provide the same poor experience with no visible functionality. This needs to be addressed before I would merge this patch regardless of if we choose to split out the generator plugins, as there should be a helpful message displayed on the download list, as well as the swagger and redoc root pages.

Its current state the patch above preserves all functionality as is and should pass all the existing tests. At this point the patch still needs work, but I want to run the tests against it.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
StatusFileSize
new 2874877-5-with_renames.patch40.91 KB

@richgerdes thanks for getting this going.

I haven't had a chance to look at it yet.

I am just uploading a patch that I generated that takes renames in account. I didn't make any changes.

Adding 

[diff]
  renames = copies

To .gitconfig will make git find renames. It makes the patch about 1/3 the size. Hopefully that will help with reviewing.

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented

Good call on the rename, git usually catches those for me, so I didn't think to even check for it. Not sure why it missed them here.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
StatusFileSize
new interdiff-5-7.txt18.08 KB
new 2874877-7.patch39.1 KB
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented 
  1. +++ b/modules/openapi_redoc/src/Controller/DocController.php
@@ -50,7 +50,7 @@ class DocController extends ControllerBase {
-      '#openapi_url' => Url::fromRoute("openapi.$api_module", [], ['query' => ['_format' => 'json', 'options' => $options]])->setAbsolute()->toString(),
+      '#openapi_url' => Url::fromRoute("openapi.download", ['openapi_generator' => $api_module], ['query' => ['_format' => 'json', 'options' => $options]])->setAbsolute()->toString(),

    Needed to updated to use the new routes. Before this patch the doc modules weren't working but we don't have any tests. 

  2. +++ b/modules/openapi_swagger_ui/src/Controller/SwaggerUIControllerBase.php
@@ -81,17 +87,9 @@ abstract class SwaggerUIControllerBase extends ControllerBase {
-    $json_url = Url::fromRoute($this->getJsonGeneratorRoute());
+    $json_url = Url::fromRoute("openapi.download", ['openapi_generator' => $this->generator_plugin_id]);

    New routes update 

  3. +++ b/modules/openapi_swagger_ui/src/Controller/SwaggerUIControllerBase.php
@@ -81,17 +87,9 @@ abstract class SwaggerUIControllerBase extends ControllerBase {
-  /**
-   * Returns route for generating JSON.
-   *
-   * @return string
-   *   The route name.
-   */
-  abstract protected function getJsonGeneratorRoute();

    Because we are using 1 route for all generators we don't need this anymore. 

  4. +++ b/src/Plugin/openapi/OpenApiGeneratorManager.php
@@ -39,28 +39,13 @@ class OpenApiGeneratorManager extends DefaultPluginManager implements OpenApiGen
+  protected function findDefinitions() {
+    $definitions = parent::findDefinitions();
+    foreach (['jsonapi', 'rest'] as $api_module) {
+      if (isset($definitions[$api_module]) && !$this->moduleHandler->moduleExists($api_module)) {
+        unset($definitions[$api_module]);
...
diff --git a/src/Plugin/openapi/OpenApiGeneratorManagerInterface.php b/src/Plugin/openapi/OpenApiGeneratorManagerInterface.php

diff --git a/src/Plugin/openapi/OpenApiGeneratorManagerInterface.php b/src/Plugin/openapi/OpenApiGeneratorManagerInterface.php
deleted file mode 100644

    By overriding getDefinitions() to not we don't get definitions rest/jsonapi definitions if the modules aren't enabled we don't need the new interface.

    That way we can get the generator plugins just by the methods on PluginManagerInterface. So if another module want to load these plugins they don't need to anything special about our manager. 

  5. +++ b/src/RestInspectionTrait.php
@@ -0,0 +1,95 @@
+/**
+ * Common functions for inspecting REST resources.
+ */
+trait RestInspectionTrait {

    I added back this trait because it was used by the Swagger UI submodule.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented 
+++ b/modules/openapi_swagger_ui/src/Controller/SwaggerUIRestController.php
@@ -12,6 +14,25 @@ class SwaggerUIRestController extends SwaggerUIControllerBase {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, RequestStack $request) {
+    parent::__construct($entity_type_manager, $request);
+    $this->entityTypeManager = $entity_type_manager;
+    $this->request = $request;
+  }

Overriding the constructor was a mistake on my part.

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented
Status: Needs review » Needs work

Looking this changes over, I hadn't looked at the docs modules, so I am glad that I didn't create too much work.

The only thing I think would worth thinking over is the implementation of the RestInspectionTrait. In theSwaggerUIRestController this is only used for for getRestEnabledEntityTypes. Ideally this should probably be a generic function (say getEnabledEntityTypes) and be on the plugins. The other functions in the trait are not used by swagger at this point.

I would recommend that we leave those functions on the generator and that we make the scope of getRestEnabledEntityTypes. However at this point the swagger routes don't deal with the plugins at all so that's not implemented. The other consideration here is that the calling function (listResources) isn't part of the swagger pages, but is part of a list builder page to show the available doc pages per entity type. For the time being, I think its safe to leave this as is. I have opened #2948570: Refactor doc module implementations to track the updating and conversion of the doc modules to a more extensible format.

I like the change to #4. I didn't actually know that was a thing. Didn't come across it documented anywhere, but I like it much better then the custom functions.

I also opened #2948572: Support additional details for alternative serializers. to address the question about supporting different serializers.

Reading over the above thread, there is only one outstanding task that I can see which is to provide a help message on the download listing when not plugins are available. Is there anything else that needs to be implemented?

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2874877-11.patch122.72 KB
new 2874877-interdiff-7-11.txt2.73 KB

Attached patch adds in the above mentioned message to the user. This text here may need to be thought over if its not clear enough. I am currently using drupal_set_message to add a warning message in the status message area. I think that seems like a sufficient solution, unless there is another suggestion.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

WOAH! >100K patch! 😮 👏

It looks like there's a lot of moved code? Tip: the patch could become a lot smaller by doing git diff -M5%.

  • richgerdes authored dc3eddb on 8.x-1.x 
    Issue #2874877 by tedbow, richgerdes: Refactor OpenApiController
richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented
Status: Needs review » Fixed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Congrats!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.