Resolve URL mapping names from the match rather than the request - #16150
Resolve URL mapping names from the match rather than the request#16150codeconsole wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16150 +/- ##
==================================================
+ Coverage 54.1238% 54.1242% +0.0004%
- Complexity 20307 20317 +10
==================================================
Files 2107 2108 +1
Lines 101144 101158 +14
Branches 17921 17925 +4
==================================================
+ Hits 54743 54751 +8
- Misses 38595 38597 +2
- Partials 7806 7810 +4
🚀 New features to boost your workflow:
|
A mapping such as "/$controller/$action?/$id?" holds a closure for each
name it captures, and until now that closure answered by reaching for the
parameters bound to the current thread:
GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.currentRequestAttributes();
return webRequest.getParams().get(name);
The value it was reaching for is one the match already holds. Because the
mapping is built once and shared by every request it matches, the closure
could not read it directly, so `collectControllerMappings` had to call
`webRequest.resetParams()` and `info.configure(webRequest)` for *every*
candidate just to be able to read `info.controllerName` and look the
candidate up - rebuilding the parameter map once per candidate, and then
once more in `UrlMappingsHandlerMapping` for the winner, because after the
loop the parameters described the last candidate rather than the winner.
The evaluator now carries only the name of the token it resolves;
`AbstractUrlMappingInfo`, which is created per match and does hold the
captured values, resolves it from its own parameters. Candidates are
identified without touching the request, so the parameter map is built
once per request, for the winner.
Mappings that compute a name with a closure of their own - the documented
"/$controller" { action = { params.goHere } }
- still read request state, and still have the request configured before
their names are read. `UrlMappingInfo.isNameResolutionRequestDependent()`
is what tells the two apart; it defaults to true, so an implementation
outside the framework keeps the behaviour it has today.
Behaviour change: a request parameter no longer stands in for a token the
URI did not capture. Under "/$controller/$action?", a request for
`/article?action=gallery` now routes to the controller's default action
instead of to `gallery`.
Adds an upgrade note for the routing change - a request parameter no longer stands in for a token the URI did not capture - with the before and after for `/article?action=gallery`, and states the rule where the guide introduces dynamic controller and action names.
…anch The request-path performance branch adds sections 45 and 46, so this becomes 47 and the two can merge in either order without a docs conflict.
ccb8d7e to
8803bd6
Compare
…me-resolution-8.0.x # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
…me-resolution-8.0.x # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
✅ All tests passed ✅Test SummaryCI - Groovy Joint Validation Build / Build Grails with Groovy snapshot (shard 2) > :grails-test-examples-scaffolding:integrationTest
🏷️ Commit: 4691e25 Learn more about TestLens at testlens.app/docs. |
|
Consolidated into #16149, which now covers the request path as one reviewable change — per-request work, hidden HTTP method handling and multipart together. This branch was merged in with its history intact; no commits were squashed or dropped, and Closing in favour of that. The reason is review flow rather than anything wrong here: these branches collided in |
Resolves captured URL mapping names from the match itself rather than from the dispatching
request, which lets
collectControllerMappingsstop rebuilding the parameter map for everycandidate mapping on every request.
Measured
ControllerMappingCollectionBenchmark.oneCandidateControllerMappingCollectionBenchmark.twoCandidatesControllerMappingCollectionBenchmark.fourCandidatesUrlMappingBenchmark.matchCachedHitcollectControllerMappingsruns in full on every request even when the URL mapping cache hits,so this is on the always-on path. It still costs ~88x the cached match it wraps; what remains is
ControllerKeyallocation, the controller map lookup and the sort.Why it was slow
A mapping's
controllerName/actionName/namespacecould be a Closure that readRequestContextHolder.currentRequestAttributes().getParams()— reaching for thread-local state toread a value the mapping already held in its own
params. To ask a candidate "which controller areyou?", the framework had to call
webRequest.resetParams()andinfo.configure(webRequest)first,per candidate, cloning the parameter map each time.
Those names now resolve from the match.
configure()is still called for mappings whose names aregenuinely request-dependent.
Deliberately unchanged
action = { params.goHere }is a documented feature — those closures are meant to read the request,and they still do, taking the old path.
Behaviour change
A request parameter no longer stands in for a token the URI does not capture. Under
"/$controller/$action?",/article?action=gallerynow routes to the default action rather thangallery; a query string could previously steer which action ran.params.actionis still boundeither way. Documented as section 47 of the 8.0 upgrade guide and in the embedded-variables guide.
This required changing one existing assertion, in
UrlMappingParameterTests.testNotEqual, which isworth a reviewer's attention. That test originally asserted the mapping did not match
(b84ef59, GRAILS-2297); when it moved to
UrlMappingsUnitTestthe fall-through to the defaultmapping made
infonon-null and acontrollerName == 'foo'assertion was substituted — 'foo' beinga value seeded into the request params by the test itself, i.e. an artifact of the thread-local
resolution rather than the
notEqualconstraint the test is named for. It now asserts the value theURI actually captured.
Notes
grails-web-benchmarks(opt-in, not part ofbuild/check). The same module is added byReduce per-request work, and rework hidden HTTP method and multipart handling #16149; if that lands first this rebases and drops that commit.
UrlMappingInfogainsisNameResolutionRequestDependent()as a default method returningtrue,so third-party implementations keep current behaviour.