Repository navigation
Conversation
|
@matrei, @jdaugherty, @jamesfredley, as proposed by @matrei as a comment on the original PR, this is the first of two new PRs which split the original PR into the simpler The Second PR will be submitted after the first PR is merged with the 9.0.x branch since the adapter’s late-filter tests depend on fixes in this PR. |
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Not approved. A later filter that wraps the request is ignored while the Grails binding is still active, so the controller sees the earlier request.
| protected ServletRequestAttributes buildRequestAttributes(HttpServletRequest request, HttpServletResponse response, RequestAttributes previousAttributes) { | ||
| if (previousAttributes == null || !(previousAttributes instanceof GrailsWebRequest)) { | ||
| return buildGrailsWebRequest(request, response) | ||
| if (previousAttributes instanceof GrailsWebRequest && previousAttributes.active) { |
There was a problem hiding this comment.
The Grails binding is request R. A later filter passes a wrapper of R whose isUserInRole('ADMIN') returns true, and leaves the binding in place. This return reuses the previous attributes. lookup() then returns R, so the controller sees false. Publish the wrapper the dispatcher received.
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
A later filter that wraps the request now causes a new Grails web request to be published with that wrapper. The controller sees the wrapper, including isUserInRole.
✅ All tests passed ✅🏷️ Commit: 49840cf Learn more about TestLens at testlens.app/docs. |
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving. A filter that adds a parameter loses that value when a later filter wraps the request.
- Preserves filter-added `params` when later filters wrap the request/response or replace the thread binding. - Retains those values through URL mapping into controller rendering. - Keeps completed requests and async workers isolated. - Expanded regression tests and updated the `params` documentation.
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
A filter that adds a parameter is kept when a later filter wraps the request. Thread PRRT_kwDOAAfRJ861bcuw resolved. Not approving. A wrapper that replaces a parameter is overwritten with the original value.
| if (previousWebRequest != null && previousWebRequest.active) { | ||
| // URL mappings reset params before configuring a match, so retain filter changes in | ||
| // the baseline as well as the dispatch's mutable parameter map. | ||
| webRequest.originalParams.putAll(previousWebRequest.params) |
There was a problem hiding this comment.
GET /test?name=raw, then a wrapper whose parameter getters return name=wrapped. The controller sees params.name == raw. The wrapper's value is lost. Keep the wrapper's value unless a filter changed that parameter itself.
- Preserves filter-added `params` when later filters wrap the request/response or replace the thread binding. - Retains those values through URL mapping into controller rendering. - Keeps completed requests and async workers isolated. - Expanded regression tests and updated the `params` documentation.
359c6b3 to
fe68244
Compare
- Preserves filter-added `params` when later filters wrap the request/response or replace the thread binding. - Retains those values through URL mapping into controller rendering. - Keeps completed requests and async workers isolated. - Expanded regression tests and updated the `params` documentation.
fe68244 to
8385e3b
Compare
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving. A filter that changes one value of a multi-value parameter loses that change when a later wrapper replaces the parameter.
| if (value instanceof Map && originalValue instanceof Map && targetValue instanceof Map) { | ||
| mergeParameterChanges((Map) targetValue, (Map) originalValue, (Map) value) | ||
| } | ||
| else if (!original.containsKey(key) || !Objects.deepEquals(originalValue, value)) { |
There was a problem hiding this comment.
A filter sets params.tags[0] to filter on a request whose tags are raw and unchanged. A later wrapper supplies wrapped and values. The controller receives the wrapper's values. Keep the filter's edit.
The approval no longer holds: a filter's edit of one value is kept, but the wrapper's other value in that parameter is dropped.
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving. A filter's edit of one value is kept, but the wrapper's other value in that parameter is dropped. Cloning each array also splits the flat name from the nested name.
| mergeParameterChanges((Map) targetValue, (Map) originalValue, (Map) value) | ||
| } | ||
| else if (!original.containsKey(key) || !Objects.deepEquals(originalValue, value)) { | ||
| target.put(key, value) |
There was a problem hiding this comment.
Request tags are raw and unchanged. A filter sets params.tags[0] to filter. A later wrapper supplies wrapped and values. The controller should receive filter and values. It receives filter and unchanged, because any difference replaces the whole array. Apply the change at the index that changed, and leave the wrapper's other value. The same comparison also treats an unchanged cloned array as a filter edit, so a wrapper that removes book.tags still leaves the raw values on params.book.tags.
| if (entry.getValue() instanceof GrailsParameterMap) { | ||
| entry.setValue(((GrailsParameterMap) entry.getValue()).clone()); | ||
| } | ||
| else if (entry.getValue() instanceof String[]) { |
There was a problem hiding this comment.
book.tags starts as one array shared by the flat name and the nested name. Cloning each occurrence separately splits them. Setting params['book.tags'][0] then leaves params.book.tags[0] unchanged, with no later filter involved. Keep those two names on the same array inside the clone, and keep that array off the original.
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Review of #16873 at bd43f4fe630 (base 8.0.x). It is 5 commits on 47d8021e481, 3 commits behind the 8.0.x tip 88272db2f39, and it merges into the tip cleanly.
What I ran, on the PR merged into the 8.0.x tip unless it says 7.0.x:
- Unit suites, rerun without the build cache:
grails-web-common(343),grails-web-mvc(99),grails-web-url-mappings(380) andgrails-controllers(214). No failures. - On
7.0.x(356cde03177): the example app with and without the first commit's dispatcher change and with and without the late filter, a probe action and filter added to it, andgrails-web-mvc(20),grails-web-url-mappings(163),grails-controllers(22) andgrails-test-suite-web(379) with the change. No failures. The results are in 1. :grails-test-examples-issue-15819:integrationTestin four setups. The results are in 1.- The PR's new specs against
8.0.x:GrailsDispatcherServletSpec,GrailsDispatcherServletHiddenMethodSpec,UrlMappingsHandlerMappingSpecandGrailsParameterMapTests, withGrailsDispatcherServletandGrailsParameterMapback at8.0.x. - A probe spec, not committed.
GrailsWebRequestFilter, then a filter that changesparams, then optionally a later filter, thenGrailsDispatcherServletwithUrlMappingsHandlerMappingand a Grails action, plus a URL mapping that redirects withkeepParamsWhenRedirect. I ran it at the merge, with both main files back at8.0.x, and with the dispatcher from the first commit. The results are in 2 and 3.
#15819 no longer happens on 8.0.x. #16797, merged after my #16516 review, already fixed it there. So on 8.0.x this PR doesn't fix the bug. What it does do there is change behavior for apps that have no late RequestContextFilter, and make params depend on whether some unrelated filter wraps the request. The bug is still on 7.0.x, where the example app fails, and 7.1.x and 7.2.x have the same code. I'd retarget the PR to 7.0.x and keep only the first commit, 4bc3f4bdc6a, there, with one change so it compiles.
1. #15819 is already fixed on 8.0.x
The bug was a split between two GrailsWebRequest instances: ResponseRenderer cleared renderView on the instance bound to the thread, while the adapter read the one in the request attribute through GrailsWebRequest.lookup(request). #16797 (c4714bb1e0b, b35184a8f84) changed lookup(request) to return the instance bound to the thread whenever it wraps the same container request. Since then the adapter reads the dispatcher's instance, the same one ResponseRenderer uses.
8.0.x tip, with |
example app, both results rows |
|---|---|
GrailsDispatcherServlet and GrailsParameterMap as on 8.0.x |
pass |
the same, but lookup(request) as before #16797 |
fail |
the same, plus the first commit's GrailsDispatcherServlet |
pass |
| this PR | pass |
The PR's specs show the same thing. With both main files at 8.0.x:
GrailsDispatcherServletSpec: 7 of 25 fail, all of them rows ofa late filter preserves dispatcher wrappers .... The 3replaceBinding=falserows fail on the wrapper (see 4), and the 4replaceBinding=truerows fail onparams.retained(see 2). Every row ofservice selects an active request ...,nested #dispatchType through a late filter ...and theCallablefeature passes.UrlMappingsHandlerMappingSpec: the template is rendered in all 4renderTemplaterows. Two fail on the filter'snamein the model (see 2), and two onrenderingRequest.is(attributeRequest), so on8.0.xthe only difference left is which instance is in the request attribute.GrailsParameterMapTests:testCloneIsolatesMultiValuedParametersfails (see 3).
So none of the PR's tests for #15819 can fail on 8.0.x. They're fine as regression tests, but the dispatcher change isn't needed there.
7.0.x, 7.1.x and 7.2.x still have the old lookup(request) and the same buildRequestAttributes. I ran the example app on 7.0.x with a probe action next to it that reports what the controller gets and handles an upload:
7.0.x |
example app | upload (request.getFile, params.doc) |
lookup() is the request attribute |
|---|---|---|---|
| no late filter | pass | works | yes |
| late filter | fail, 500 | both missing | no |
| late filter, first commit's change | pass | works | yes |
So behind a late filter, uploads are broken as well, for the same reason: checkMultipart hands the multipart request to the instance in the request attribute, and the controller reads the other one. URL-mapped params.id, actionName, a conventional view, a redirect and the 500 page work in all three rows. Without the late filter the probe gives the same results with and without the change. That's what the code says too: with no previous binding, or an active GrailsWebRequest, the first commit does exactly what 7.0.x does.
The first commit doesn't compile on 7.0.x as it is. There grails-web-common has grails-gsp-core only as compileOnly, and GrailsApplicationAttributes declares a grails-gsp-core type (getPagesTemplateEngine()). So GrailsApplicationAttributes.WEB_REQUEST in GrailsDispatcherServlet fails with NoClassDefFoundError: org.grails.gsp.ResourceAwareTemplateEngine during compilation. WebUtils.storeGrailsWebRequest(webRequest) compiles and does the same here. It also binds the instance to the thread, which FrameworkServlet does right after buildRequestAttributes returns anyway. The four 7.0.x suites above pass with that change.
My suggestion: retarget to 7.0.x with the first commit's buildRequestAttributes, publishing through WebUtils.storeGrailsWebRequest, plus the example app with an upload added, and tests for the dispatcher without the rows for 2-4. GrailsDispatcherServletSpec and GrailsDispatcherServletHiddenMethodSpec don't exist on 7.0.x, so those tests need a new spec there. The merge-up then carries the fix and the regression tests to 8.0.x. Everything else should go to 9.0.x, if we want it at all, for the reasons below. If you'd rather keep this on 8.0.x, it should come down to the regression tests.
The changes after the first commit came from @jamesfredley's review rounds, and each one asks for something reasonable. But 8.0.x doesn't do any of it for an app without a late filter either, so doing it only behind one makes those apps the odd ones out.
2. A filter's changes to params reach the action only when a later filter wraps the request or replaces the binding
On 8.0.x, a URL-mapped action never sees a change a servlet filter made to params. Before it matches, UrlMappingsHandlerMapping calls webRequest.resetParams() (UrlMappingsHandlerMapping.groovy:244), which rebuilds params from originalParams, the request's own parameters. The scenarios in the review rounds don't show this, because they map /test with a SimpleUrlHandlerMapping, and that doesn't reset anything.
This PR copies the filter's changes into the new instance's originalParams (GrailsDispatcherServlet.groovy:76-77), so they survive the reset. But it only does that when the dispatcher builds its own instance. In the probe, the request has name=raw and tags=raw&tags=unchanged. The filter sets params.added and params.name to 'filter', and sets params.tags[0] = 'filter'. The action reports added, name and tags:
| after the filter | 8.0.x |
this PR |
|---|---|---|
| nothing | null, raw, [filter, unchanged] |
null, raw, [raw, unchanged] |
a later RequestContextFilter |
null, raw, [filter, unchanged] |
filter, filter, [filter, unchanged] |
| a later filter that only wraps the request | null, raw, [filter, unchanged] |
filter, filter, [filter, unchanged] |
| a later filter that only wraps the response | null, raw, [filter, unchanged] |
filter, filter, [filter, unchanged] |
any of these, on a multipart POST |
null, raw, [filter, unchanged] |
null, raw, [raw, unchanged] |
On 8.0.x every row is the same, and so is every row with the first commit's dispatcher. With this PR, the same action gets different params depending on whether a later filter wraps the request or the response. Filters like that are common, see 4. Two more effects:
- A URL mapping that redirects with
keepParamsWhenRedirectappendsoriginalParamsto the redirect (ResponseRedirector.groovy:111-113). With a later filter, the filter's values end up in theLocationheader, including any it added for its own use:/probe/show?name=filter&tags=filter&tags=unchanged&added=filter, where8.0.xsends?name=raw&tags=filter&tags=unchanged. - On a multipart request,
checkMultipartcallsmultipartRequestResolved()(GrailsDispatcherServlet.groovy:144), which discards both maps, so the copied changes are lost again (last row).
The new params.adoc paragraph documents this as a property of params. It holds neither in an app without a later filter nor on a multipart request. If the changes a filter makes to params should reach the action, that's a feature for every app, so it belongs in 9.0.x, with docs and an upgrade note. There it would also have to survive resetParams() without a late filter.
3. GrailsParameterMap.clone() now copies arrays, which changes params in every app
resetParams() clones originalParams. On 8.0.x the clone shares the String[] values, so an in-place edit such as params.tags[0] = 'filter' also changes originalParams, and survives every later resetParams(). That's the [filter, unchanged] in the 8.0.x column above, and it's why the new GrailsParameterMapTests test fails on 8.0.x. The PR copies the arrays (GrailsParameterMap.java:145-147), so in an app without a later filter, the filter's edit is now lost when the URL mappings reset params: the first row of the PR column.
I'd call the sharing accidental. But it's what every app gets today, and the copy is only needed as the baseline for the comparison in 2. Once 2 is gone, this can go too.
4. A filter that only wraps the request or response now gets its own GrailsWebRequest
This came in with the first review round. On 8.0.x, when a later filter wraps the request or the response but leaves the binding alone, the dispatcher reuses the filter's instance. Nothing breaks there, because RequestContextHolder and the request attribute hold the same instance, so it isn't #15819. The controller sees the request and response GrailsWebRequestFilter saw, like in every other app. With this PR it gets a new instance built with the wrappers, and every request in such an app goes through the copying from 2.
In #16516 I suggested that giving the late-filter apps the same request and response as every other app should wait for 9.0.x. This is the same kind of change, just the other way round: apps without a late RequestContextFilter now get a different request and response than they get on 8.0.x. Wrapping filters after GrailsWebRequestFilter (-150) aren't rare. A FilterRegistrationBean or @Component filter without an order runs at LOWEST_PRECEDENCE, for example a logging filter with a ContentCachingRequestWrapper. Two examples of what changes:
- Spring Security's
FilterChainProxyruns at -100 and wraps the request and response on every request. On the plugin's default chains nothing changes, becauseUpdateRequestContextHolderExceptionTranslationFilteralready binds aDelegatingGrailsWebRequestbuilt with those wrappers, and the dispatcher receives the same objects. On a chain without that filter, such as the REST plugin's documented/api/**chain (-exceptionTranslationFilter),request.userPrincipal,request.remoteUserandrequest.isUserInRole(...)in a controller now answer from Spring Security instead of the container. - A response wrapper such as
ContentCachingResponseWrappernow gets the action's output. On8.0.xthe output goes to the response underneath it.
I got both from reading the code, not from running an app. The PR's own spec asserts the underlying behavior in the replaceBinding=false rows of a late filter preserves dispatcher wrappers ....
Both may well be what those apps want, but they're changes in behavior on a patch branch, and they're unrelated to #15819. The first commit reuses any active GrailsWebRequest, as 8.0.x does.
5. Tests
- With 2-4 gone, the
replaceBinding=falserows and theparamsassertions ina late filter preserves dispatcher wrappers ...go too, and so do theparamsassertions and thefilterChangesNamerows of theUrlMappingsHandlerMappingSpecfeature. GrailsDispatcherServletHiddenMethodSpec: the helper now assertsGrailsWebRequest.lookup().is(GrailsWebRequest.lookup(req))andparams.retained == 'filter value'(:131-132) in every hidden-method feature. Both are about reusing the filter's instance, which thethreadrow ofservice selects an active request ...already covers. If one of them ever fails, it fails in a feature named for the hidden method. I'd drop them. Going throughservice()instead of callingcheckMultipartdirectly is fine.UrlMappingsHandlerMappingSpecimportsorg.grails.web.util.GrailsApplicationAttributestwice (lines 42 and 44), and the new imports aren't in the file's order.
6. Smaller points
- The commit messages are named after review rounds, so please squash on merge with a short message.
- The PR description doesn't mention the
paramshandling or theGrailsParameterMapchange. That's moot if they go.
Verified as correct
- The first commit's fix: it publishes the instance it binds when a later filter replaced the binding, so the request attribute and
RequestContextHolderagree. On7.0.xit turns the failing example app into a passing one and makes uploads behind a late filter work (1). Callablethreads (point 1 of #16516): anullprevious binding gets its own instance and leaves the request attribute alone.a Callable keeps an active request binding after the initial dispatch returnscovers it.- Forwards and includes through a late filter:
GrailsWebRequestFilterbinds a new instance onFORWARDandINCLUDE, the dispatcher reuses it, and the enclosing instance is restored afterwards.nested #dispatchType through a late filter ...covers both. - The example app registers the late filter the way the issue does, has no conventional
results.gsp, and asserts on the HTTP response. It fails with the oldlookup(request), so it's a real reproducer.
bd43f4f to
41dfd96
Compare
|
@matrei, thanks very much for the exhaustive review, and for your patience. To keep things simple, I started with the following changes, as you requested. Then we can build from there as you see fit:
Please let me know what the next step is, if any. cc: @jamesfredley |
41dfd96 to
1588c19
Compare
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving. The flat and nested names still share their array. Publishing the new request drops a filter's edit of one value, so the controller sees only the wrapper's values.
| // and rendering agree, while preserving the request and response wrappers seen here. | ||
| // FrameworkServlet passes null on Callable threads, which need their own instance | ||
| // without replacing the attribute still used by the dispatch. | ||
| request.setAttribute(GrailsApplicationAttributes.WEB_REQUEST, webRequest) |
There was a problem hiding this comment.
Request tags are raw and unchanged. A filter sets params.tags[0] to filter. A later wrapper supplies wrapped and values, and a late filter replaces the thread binding. The controller should receive filter and values. It receives wrapped and values, because the new request is published without the filter's edit. Carry that edit onto the published request.
Description
The Fix
Fixes #15819
Splits the dispatcher fix from #16516 as requested by @matrei:
Generated with GPT 6 Astra via Cline API Provider in IntelliJ IDEA 2025.3.3
Contributor Checklist
Please review the following checklist before submitting your pull request. Pull requests that do not meet these requirements may be closed without review.
Issue and Scope
7.0.x): Bug fixes only. No new features or API changes.7.1.x): New features are welcome, but breaking existing APIs must be avoided.8.0.x): Reserved for major changes. Breaking API changes are permitted.Code Quality
./gradlew build --rerun-tasks../gradlew codeStyleand resolved any violations. See Code Style for details.Licensing and Attribution
Documentation