matrei commented on PR #16272:
URL: https://github.com/apache/grails-core/pull/16272#issuecomment-5858749949
Thanks @codeconsole. Second round, at head `d63aac9d10`. That covers the
seven commits from `e87bbf388f` to `e81621a6c2` plus two merges of `8.0.x`,
which are clean. CI is green on the head.
Run locally at the head with `--no-build-cache` after `cleanTest`:
| Module | Tests | Result |
|---|---|---|
| `grails-web-url-mappings` | 311 | pass |
| `grails-controllers` | 211 (1 skipped) | pass |
| `grails-scaffolding` | 79 | pass |
All five points from the last round are resolved:
1. Explicit `controller` on a resource link: `namesController` now skips
`resolveResourceTarget` in both the instance and the `Class` branch. The
namespace pin no longer checks `controllerAttribute == null`, since
`resourceTarget` is only set when the link was resolved. The
blank-versus-absent `Pamphlets` case pins it well: the name alone is ambiguous
there and only the resolution can pick `print`.
2. Reported-once sets: they now live on `ControllerIndex`, and the new specs
reload a controller on a plain `DefaultLinkGenerator` without calling
`resetControllerNamespaceCache()`. Those specs would fail on the previous head.
3. Unknown controller name: agreed on keeping it. The link/redirect symmetry
argument holds. There is one problem with the new wording, though; see below.
4. `resolveNamespace` now returns `String`, and the `GString` spec reads the
namespace through `GrailsWebRequest.controllerNamespace`, as link generation
does.
5. `LogCapture`: `slf4j-simple` is gone and Logback is the only binding on
`testRuntimeClasspath`. The `currentControllerIndex()` rename is a good catch.
### 1. The new upgrade-note example does not change in a `ControllerUnitTest`
`upgrading80x.adoc:793` now says that in unit tests of a namespaced
controller, `g.createLink(controller: 'other', action: 'list')` "now generates
`/admin/other/list` rather than `/other/list`". That came from my point 3 last
round, and I got it wrong. The test harness never puts the controller's
namespace on the request: `mockController` sets `controllerName` but not
`controllerNamespace`. So the link is resolved from the default namespace and
comes out the same as before. I checked this with a throwaway spec on this head
and on `8.0.x`. It used an `admin`-namespaced controller, no `OtherController`
registered, and a `UrlMappings` with a `/$namespace/$controller/...` mapping:
| | `webRequest.controllerNamespace` not set | set to `'admin'` in the test |
|---|---|---|
| `g.createLink` / `createLink` / `grailsLinkGenerator.link`, 8.0.x |
`/other/list` | `/other/list` |
| same, this PR | `/other/list` | `/admin/other/list` |
| `redirect(controller: 'other', ...)`, both | `/admin/other/list` |
`/admin/other/list` |
So the change shows where the request actually carries the namespace:
- at runtime: a page rendered by a namespaced controller, linking to a name
no controller registers
- in a test that sets `webRequest.controllerNamespace` itself
A plain `ControllerUnitTest` is not one of those places. Suggested wording:
> A name no registered controller has stays in the namespace the link is
made from, where a link previously dropped the namespace; a redirect to such a
name has always stayed in it. A page rendered by a controller in the `admin`
namespace that links with `controller: 'other'` now generates
`/admin/other/list` when no `OtherController` is registered, and so does a unit
test that sets `webRequest.controllerNamespace` itself. Pass `namespace` to
target another namespace.
That also names the `admin` namespace the example relies on, which the
current sentence leaves the reader to infer. I would drop "Register the other
controller in the test". The obvious way to do that is
`mockController(OtherController)`, which also switches
`webRequest.controllerName` to `other`, so later links that name no controller
in the same test would move with it.
This also makes the behaviour change smaller than I made it sound last
round, which helps the list discussion.
### Verified as correct
- `resolveLinkTarget` with `namesController` and an entity instance goes to
the `hasId` branch. There, `resource` only feeds the parent-resource tokens,
which a class property name never has, so nothing changes for a proxy instance.
- `ControllerIndex.EMPTY` also carries the two sets, but nothing can report
against it: no names, no serving controllers.
- The generated-controller templates in
`grails-scaffolding/src/main/templates/scaffolding/` still redirect with
`action: "show"` after the `8.0.x` merges that reworked scaffolding generation,
and no other controller template in the repository uses `redirect
${propertyName}`.
--
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]
