matrei commented on PR #16275:
URL: https://github.com/apache/grails-core/pull/16275#issuecomment-5809880163
Reviewed at head `11b3d985bf` (`feat/openapi-springdoc-8.0.x`, 19 commits)
against `8.0.x`. Merge base `3dc36325dc`; `8.0.x` has taken 33 merges since,
and `git merge-tree` reports no conflicts. CI is green on the head.
Run locally at the head:
| Module | Tests | Result |
|---|---|---|
| `grails-openapi` (`check`: tests, CodeNarc, Checkstyle) | 76 | pass, no
violations |
| `grails-test-examples-openapi` (`integrationTest`) | 10 | pass |
| `grails-forge-core` (`OpenApiSpec`) | 3 | pass |
| `validateDependencyVersions` for both new projects | | pass |
I also ran the functional app with the default
`"/$controller/$action?/$id?(.$format)?"` mapping added and dumped
`/v3/api-docs` to check what an application actually gets, since the shipped
example does not exercise that path (see 4). The document was clean: `Book`
carried only `title`, `genre`, `id` and `version`, the expanded `/book/...`
routes appeared alongside `/books`, every reference resolved, and every
operation id was distinct. The `@Autowired(required = false)` property
injection of `grailsApplication` works in a real context. I also checked the
command object rule in `ActionAnnotations.isCommandObject` against
`ControllerActionTransformer` and it mirrors it exactly, including the
`Serializable` exclusion.
The design is sound: derive everything from the mappings and the mapping
context, let swagger-core resolve types, overlay the GORM constraints, and let
the standard annotations correct the result. The findings are mostly leftovers
from refactoring plus two behavioural gaps.
### 1. Dead code in `PersistentEntitySchemaBuilder`
`retainReferenced` (`:208`), `referencedNames` (`:222`) and
`collectReference` (`:233`) are private and nothing calls them; `buildCommand`
walks declared fields instead. CodeNarc does not flag them because the repo
does not enable `UnusedPrivateMethod`, but they should go, along with the
Javadoc at `:204` that describes them.
### 2. Orphaned and stacked Javadoc from earlier refactors
Groovydoc is published for this module, so these surface in the API docs.
Each is a Javadoc block sitting on the wrong member or stacked on top of
another block:
- `UrlMappingsOpenApiCustomizer.groovy:145` describes `addExpandedMappings`
but is attached to `addMappedOperation`; the real doc follows at `:151`.
- `UrlMappingsOpenApiCustomizer.groovy:423` and `:427` describe a schema
registration and a tag description method that no longer exist at that spot;
both are stacked over the `disambiguateOperationIds` doc at `:431`.
- `UrlMappingsOpenApiCustomizer.groovy:586` describes `asStaticName` but
sits on `describe`.
- `UrlMappingsOpenApiCustomizer.groovy:64` (class Javadoc) says mappings
whose controller is only known per request "are skipped", which
`addExpandedMappings` contradicts.
- `PersistentEntitySchemaBuilder.groovy:143` describes
`declaredPropertyTypes` but is stacked over the `collectTypeArguments` doc at
`:147`.
- `RestfulControllerActions.groovy:40` describes `DEFAULT_METHODS` but is
attached to `DEFAULT_SUCCESS_CODE`.
### 3. An application with more than one datastore gets no schemas, silently
`setMappingContextProvider` uses `getIfUnique()`, so with Hibernate and
MongoDB together (two `MappingContext` beans) `mappingContext` is `null` and
every operation is described without a shape. The Javadoc at `:93` acknowledges
the "exactly one" condition, but the user gets no hint why the schemas
vanished. Two options, in order of preference:
- Iterate `provider.orderedStream()` and index the entities of every
context, resolving the validator from the context that owns each entity.
`indexEntitiesByController` and `constraintsFor` are the only two places that
need the owning context.
- Or keep the single-context rule and log at INFO when the provider returned
nothing because it was ambiguous, and say so in the guide's limitations list.
### 4. The functional example does not cover the expansion path
`grails-test-examples/openapi/.../UrlMappings.groovy` has only the
`resources` mapping, so `addExpandedMappings`, and with it the injection of
`grailsApplication`, is only exercised by unit specs that wire the customizer
by hand. The PR body and guide advertise the default mapping routes as a
headline feature. Adding `"/$controller/$action?/$id?(.$format)?" {}` to the
example and one assertion on `/book/show/{id}` would make the functional spec
cover it. I confirmed it passes.
### 5. A mapping that names a controller but no action is described as an
action-less operation
For `"/catalogue"(controller: 'book')` Grails dispatches to the controller's
default action (`index`). `addMappedOperation` passes a `null` action through,
so the operation id becomes `book_get`, a `RestfulController` falls into the
generic 200 branch at `:736` with no paging parameters and a single-resource
schema rather than the collection, and the action annotations are not
consulted. Defaulting `mappedAction` to the controller's default action
(`GrailsControllerClass.defaultAction`, `index` when unset) when the mapping
names a controller only would make that case consistent with the rest.
### 6. Guide: nothing says the document is public
`openApi.adoc` never mentions that `/v3/api-docs` and Swagger UI are served
to anyone and enumerate every REST route, domain class and constraint. One
sentence pointing at securing the two paths with whatever the application uses,
or disabling them per environment (`springdoc.api-docs.enabled: false`),
belongs under Getting Started or Customizing.
### 7. Nits
- `settings.gradle:573`: `grails-test-examples-openapi` is inserted between
`latency` and `namespaces`, breaking the alphabetical order of that list.
- `UrlMappingsOpenApiCustomizer.groovy:83`, `:353`:
`java.util.regex.Pattern` and `Matcher` are fully qualified inline; `:386` does
the same with `grails.core.GrailsControllerClass`, and `ActionAnnotations`
qualifies `io.swagger.v3.oas.annotations.Operation` and friends throughout.
Aliased imports (as the file already does for `Parameter as ParameterModel`)
would read better.
- `PersistentEntitySchemaBuilder.entityNamed` scans every persistent entity
per resolved schema. Fine at these sizes, but a name-keyed map built once in
`build` is a one-liner.
- The two `MediaType` fallbacks (`:738`, `:804`) emit `"application/json":
{}` when the entity is unknown. Valid, but an operation with no content at all
reads better than a content entry with no schema. Preference only.
### Verified as correct
- `resources` really does generate `POST /books/{id}` for `update`
(`DefaultUrlMappingEvaluator.java:1025`), so the extra `post` on `/books/{id}`
is the mapping, not the customizer.
- The generated `META-INF/grails-plugin.xml` carries `dependsOn
urlMappings='version'` literally; every core plugin's descriptor does the same
and the runtime reads the instance property, so this is not new.
- springdoc 3.1.0 resolves against Spring Boot 4.1.1 and Framework 7.0.9 on
the module classpath, and the functional app serves both the document and
Swagger UI with it.
- `disambiguateOperationIds` runs after the `_byAction` suffix is applied at
expansion time, so the path-qualified suffix is only reached for two declared
mappings hitting the same action, which the `RestApiMappingSpec` covers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
