matrei commented on PR #16272:
URL: https://github.com/apache/grails-core/pull/16272#issuecomment-5885227836
Thanks @codeconsole. Fourth round, at head `f56bf2d950`. It covers the eight
commits since `5c4195c5a9`: the fixes for @jdaugherty's three inline findings,
the `rest-api` template change, and the four functional specs he suggested.
There are no merges this time. The PR still merges cleanly into the current
`8.0.x` (`12be92bbb8`), and CI is green on the head, including every functional
test job that runs the four new specs.
Run locally at the head with `--no-build-cache` after `cleanTest`:
| Module | Tests | Result |
|---|---|---|
| `grails-web-url-mappings` | 327 | pass |
| `grails-controllers` | 211 (1 skipped) | pass |
| `grails-scaffolding` | 79 | pass |
| `grails-views-gson` | 186 | pass |
| `grails-fields` | 685 (8 skipped) | pass |
All three fixes hold.
1. **Hyphenated converter** (`97bbb161c8`). The fix depends on the request
holding the name and the namespace in different forms, so I traced where each
one comes from:
- `DefaultUrlMappingInfo.getControllerName()` always runs the name
through `urlConverter.toUrlElement`, so the request holds `tour-desk`.
- The namespace goes through the converter too, but
`UrlMappingsInfoHandlerAdapter` then overwrites it with
`controllerClass.namespace`, so it ends up logical (`backOffice`).
So the right test is to compare each candidate's logical name, and its
`toUrlElement` form, with the request name, and to compare namespaces directly.
Applying the same check to the current-controller shortcut in
`getDefaultNamespace` was a good catch.
2. **Same simple name** (`d5b714c80a`). Recording the served class per
controller and dropping a namesake that serves a different class is the minimal
fix. The six `Item` rows in `LinkGeneratorResourceControllerSpec` cover every
direction.
3. **Only `RestfulController` counts** (`8b871ccd73`). `domainClassNameFor`
walks superclasses only, stops at `grails.rest.RestfulController` and reads its
type argument through `ResolvableType`. That covers:
- a direct subclass
- an intermediate base, including `RestfulServiceController<T extends
GormEntity<T>>`
- `@Scaffold` and `static scaffold`: `ScaffoldingControllerInjector` sets
`RestfulController<Domain>` together with `usingGenerics`, so the generic
signature is written to the class file. `ScaffoldingControllerInjectorSpec`
pins that, and `RenamedScaffoldLinksSpec` covers it end to end.
Matching on the class name, not the class, is the right way around the
module dependency. The docs, the upgrade note and the `rest-api` template now
all show the parameterised form, and no raw `extends RestfulController`
examples are left in `grails-doc` or the profile templates.
Two nits, both about wording:
### 1. The namesake rule is broader than its doc and comment say
`servingControllers` drops a controller named after the entity whenever it
serves any other domain class, not only one with the same simple name. I
checked this with a throwaway spec on this head. With `BookController extends
RestfulController<Publication>` next to `BooksController extends
RestfulController<Book>`, a `show` link to a `Book` goes to `/books/show/1`.
Before `d5b714c80a` it went to `/book/show/1`, where the name settled the tie.
That behaviour is right, since the dropped controller's `show` would load a
`Publication`. But the new paragraph in `restfulMappings.adoc` and the inline
comment (`// Named after the entity, but serving another domain class of the
same simple name.`) describe only the same-simple-name case. Suggested wording
for the doc:
> A controller named after the domain class that extends `RestfulController`
parameterised on another domain class serves that other class only, as a
`UserController` serving `com.example.community.User` does beside a
`com.example.User`.
For the comment: `// Named after the entity, but serving another domain
class.`
### 2. The `isRequestController` Javadoc limits the URL form to some mappings
It says the request holds the name "its URL mapping gave it, which a mapping
taking it from the URL writes in the URL converter's form". But
`getControllerName()` converts in every case, so a static `"/desk"(controller:
'tourDesk')` puts `tour-desk` on the request too. Something like "The request
holds the controller name as the URL converter writes it: `tour-desk` rather
than `tourDesk` under the hyphenated converter" would match what the code
handles.
Neither blocks. My approval stands.
--
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]
