Reduce per-request work, and rework hidden HTTP method and multipart handling - #16149
Reduce per-request work, and rework hidden HTTP method and multipart handling#16149codeconsole wants to merge 76 commits into
Conversation
The request Grails exposed to controllers, tag libraries and GSPs was replaced with the resolved MultipartHttpServletRequest for file uploads. That discarded every request wrapper contributed after multipart resolution - the hidden HTTP method filter, Spring Security, and any application filter - and required a mutable pointer on GrailsWebRequest plus propagation code to maintain it. The request is now always the outermost request, and multipart capabilities are discovered from its wrapper chain via WebUtils.resolveMultipartRequest. When the DispatcherServlet resolves a request Grails had already bound, the wrapper sits above that request and cannot be reached by unwrapping, so it is also published as a request attribute. - Add WebUtils.resolveMultipartRequest and isMultipartContentType - Add the MultipartRequest read surface to HttpServletRequestExtension so request.getFile(..) and friends keep working, failing loudly rather than returning null when the request is not a resolved multipart request - Populate GrailsParameterMap through discovery rather than an instanceof check - Replace GrailsWebRequest.setMultipartRequest and the multipart branch in getCurrentRequest with multipartRequestResolved, which only invalidates the cached params (apachegh-13837) - Return the processed request from GrailsDispatcherServlet.checkMultipart, so the dispatch runs against it as Spring MVC expects - Delete the unreachable multipart resolution in DefaultUrlMappingInfo, along with the undocumented grails.web.disable.multipart setting - Make the SpringSecurityUtils multipart branch functional again; it read an attribute only the deleted DefaultUrlMappingInfo code ever wrote request instanceof MultipartHttpServletRequest and casts to that type no longer work; documented in the 8.0 upgrade guide.
isMultipartContentType had no production caller - only the test written for it. Condense the six per-method javadoc blocks on the file upload accessors into one.
…ntext GrailsWebRequest built a GrailsApplicationAttributes on every request, through a reflective Constructor.newInstance. That object holds no request state - it caches the beans its own comment calls "used very often" (template engine, GrailsApplication, GroovyPagesUriService, MessageSource, plugin manager) - so building one per request paid for the reflection and then discarded all five caches immediately. It is now created on first use and cached in the servlet context, and rebuilt only if the ApplicationContext it resolved against is no longer current, so a replaced or restarted context (as happens between tests) is never served a stale instance. Its lazily populated fields become volatile now that one instance is shared across request threads. Also stop allocating a UrlPathHelper per request or per call. Spring exposes UrlPathHelper.defaultInstance and none of the four Grails instances were configured, so they can share it.
…ceptors UrlMappingsHandlerMapping.getHandlerExecutionChain re-implemented the loop from AbstractHandlerMapping, so Grails-mapped requests silently missed whatever Spring added to that method later. Currently that is the API version deprecation interceptor, which means the Deprecation, Sunset and Link headers configured by spring.mvc.apiversion.* were never emitted for a Grails-mapped request. It now calls super and inserts the WebRequestInterceptors at the front, which keeps the "OSIV must run first" ordering that motivated the override. The two Grails interceptors are stateless, so they become shared instances instead of two allocations per request, and the @CompileDynamic MappedInterceptor cast helper goes away with the copied loop. Also: - Keep UrlMappingsHandlerMapping's own UrlPathHelper. UrlPathHelper.defaultInstance is read-only and that field is protected, so pointing it at the shared instance would break any subclass configuring it. It is a singleton bean, so there was no per-request allocation to save there anyway. - Restore the previous LocaleContext in GrailsWebRequestFilter rather than clearing it, so a LocaleContext set by a filter outside Grails survives, matching Spring's own RequestContextFilter.
GrailsParameterMap's constructor defensively copied request.getParameterMap() into a LinkedHashMap before walking it. updateNestedKeys only ever reads that map - every put it makes goes into wrappedMap or a nested map it created - so the copy was only ever needed to merge uploaded files in. The servlet map (immutable per the servlet contract) is now walked directly, and copied only when there are multipart files to merge, removing a map allocation and a full entry copy per request for every non-upload request.
A URL mapping cache miss was the most expensive thing in the request path by three orders of magnitude - 1964 ns for a URI only the default mapping serves, against 2.5 ns for a cache hit - because every miss ran a linear scan allocating a Matcher for each of the ~56 compiled patterns in a mid-size application. RegexUrlMapping now records each pattern's slash count at parse time and skips patterns whose segment count rules them out. Every construct convertToRegex emits is bounded to a single path segment except ".*", which comes only from a "**" token, so a pattern without "**" can only match a URI with exactly its slash count or one more, the extra one coming from the trailing "/??" every pattern ends with. Patterns containing "**" are never skipped. Candidates are skipped, never reordered, so the scan still returns the first mapping that matches and declaration precedence is unchanged. This replaces the patternByTokenCount map, which built exactly this index and was never read by anything. The holder computes the URI's slash count once per request rather than once per mapping, and hoists the per-candidate LOG.isDebugEnabled() call out of the three scan loops.
The adapter passed its callback to observe() as `{ -> i.before() } as BooleanSupplier`.
Groovy evaluates that coercion before entering observe(), so it ran even when the
ObservationRegistry is a no-op, and DefaultGroovyMethods.asType routes it through
CachedSAMClass.coerceToSAM to Proxy.newProxyInstance. Every matched interceptor
therefore cost a Closure, a Class[], a ConvertedClosure and a JDK dynamic proxy per
phase per request, with each callback dispatching reflectively through
ConversionHandler rather than calling the interceptor directly.
The phase is now a private enum that dispatches straight to before()/after(), so the
default path is a field read, a no-op check and an interface call. In the compiled
class groovy.lang.Reference references drop from 15 to 0 and the two closure classes
are gone. The observing path is structurally unchanged.
Also caches the logical interceptor name per class rather than recomputing it per
interceptor per phase per request, and reverses the matched-interceptor list in place
rather than copying it - the reversed list is stored back under the request attribute
and read by afterCompletion, so the ordering remains observable and unchanged.
Adds coverage for the observation path, which previously had none, including the
null-registry branch, the no-op registry branch, and error recording.
The controller AST transformer emitted the ALLOWED_METHODS_HANDLED request-attribute guard twice into the same generated wrapper - once from convertToMethodAction and again from wrapMethodBodyWithExceptionHandling - producing two byte-identical blocks where the second could never do anything, because the first had already set the attribute. It also emitted the guard, and its finally-block cleanup, for controllers that declare no allowedMethods at all. An action on a controller with no allowedMethods was paying four dynamic request property gets, two getAttribute, a setAttribute, a removeAttribute and a compareEqual per request for a guard that could never fire. Each request property get goes through an indy callsite and RequestContextHolder, so this was not free. The duplicate emission is removed, and the bookkeeping is now generated only for controllers that declare a non-empty allowedMethods map. Gating it per action rather than per controller looks equivalent but is not: the marker means "an action has already begun handling this request" (apachegh-11444), so an unrestricted action must still set it, or a restricted action it invokes programmatically starts rejecting the request. Controllers that use allowedMethods generate byte-identical code to before. Adds coverage for the command-object path, which had none.
…okup Binding a command object resolved the DataBindingSourceRegistry, the MimeTypeResolver and the GrailsWebDataBinder from the bean factory on every request, each with a containsBean followed by a getBean, and did so twice because bindObjectToInstance runs createDataBindingSource again. Holders.findApplication() is itself a getBean rather than a field read, and was called twice more per bind. These now resolve once per ApplicationContext, held in a single-entry volatile cache. A map keyed by ApplicationContext would retain every context ever seen, since the cached beans reference the context, so a single entry replaced whenever a different context appears is both cheaper and the correct invalidation signal for dev restarts and test contexts. Separately, getBindingIncludeList used getDeclaredField to look up the AST-injected whitelist field and cached the result after the call. For any class the transformer did not touch - inner-class command objects, precompiled classes, plain POJOs - that call threw, control jumped past the caching, and the exception was reconstructed on every subsequent bind. It now uses ReflectionUtils.findField, which returns null, and caches the negative result too, while still requiring the field to be declared on the class itself so an untransformed subclass does not inherit its parent's whitelist.
…ing paths Four lookups repeated per request, all resolving to values that are stable: - Every redirect read the controller's static namespace field reflectively, through a hierarchy walk plus makeAccessible plus Field.get. The in-code comment already noted this was avoidable. Now cached per controller Class; a reloaded class is a different Class object, so a stale namespace cannot be served. - Every redirect allocated a ResponseRedirector and called three setters on it. That object holds only configuration and takes the request, response and arguments per call, so one is now built lazily and reused. Each of its setters clears the cached instance, so a configuration change after the first redirect is still honoured. - Every template render resolved CompositeViewResolver from the bean factory. It is now held in a field, matching how this trait already caches the plugin manager, mime utility and layout selector. - The domain map constructor resolved the GrailsApplication and PersistentEntity, discarded them, then resolved both again to autowire the instance. They are now resolved once and passed down. The redirector is held in an AtomicReference rather than a volatile field: Groovy's trait field remapping drops the volatile modifier, and unlike the other cached values this object is constructed here after its setters run, so it needs safe publication.
GrailsWebRequest.getCurrentRequest() returned the resolved MultipartHttpServletRequest in place of the request Grails was bound to. That substitution is gone, so the method is now literally `return getRequest();`. The two can never disagree. getRequest() is final on Spring's ServletRequestAttributes and fixed at construction, and nothing wraps or replaces the request for the lifetime of a GrailsWebRequest: includes and forwards wrap only the response and dispatch the same request object, layout decoration swaps the response and re-renders against the original request, and async builds a new GrailsWebRequest around the request it is given. The two places that do cope with a later request wrapper avoid this method entirely - multipart through WebUtils.resolveMultipartRequest, and Spring Security by binding a fresh DelegatingGrailsWebRequest. All 65 framework call sites now use getRequest(). The method is deprecated rather than deleted so plugins keep compiling; removing it is a separate decision. - Move the "always the outermost request" note to the class javadoc, where it outlives the deprecated method - Keep getCurrentRequest in DelegatingGrailsWebRequest's @DeleGate exclusions. Delegating it would hand back the request from earlier in the filter chain, which is what that filter exists to prevent. Both reasons the exclusion list exists are now written down - Cover that filter with a spec; it had none - Stop JsonViewTemplateResolverSpec mocking GrailsWebRequest and stubbing getCurrentRequest(). It relied on the deprecated method being the only stubbable request accessor and produced an object whose two accessors disagreed; it now drives a real GrailsWebRequest over a MockHttpServletRequest
Covers controller action invocation (with and without allowedMethods, and a command-object action), the interceptor chain with a no-op and an observing registry, and collectControllerMappings - the uncached wrapper that runs on every request even when the URL mapping cache hits. The existing benchmarks measured GrailsWebRequest construction (12 ns) and multipart resolution (1.7 ns), neither of which is where request time goes.
getRequest() is final on ServletRequestAttributes, so getCurrentRequest() was the only stubbable request accessor on GrailsWebRequest. Tests that mocked it will see framework code take a different path now that it calls getRequest() directly.
The filter sets the locale from the request unconditionally, but restored the previous LocaleContext only on the outermost dispatch. An include or forward therefore left the enclosing request with the locale it had installed, and replaced any TimeZoneAwareLocaleContext with a plain SimpleLocaleContext for the remainder of that request. The restore now happens on every invocation, matching the unconditional set. Only the GrailsWebRequest handling stays branched, since an include restores the previous web request rather than clearing it. This filter had no test coverage; adds one, including a case that fails without the change.
…8.0.x Upstream landed the mass-assignment hardening (apache#15947) and the clearMissing work (apache#15950), both of which rewrote the DataBindingUtils methods this branch had touched in "Cache the data binding collaborators and the databinding whitelist lookup". The conflict is resolved in favour of upstream everywhere the two overlap, so that the deny-by-default binding behaviour is exactly the one upstream shipped. Superseded by upstream and dropped from this branch: * The whitelist include-list caching in getBindingIncludeList. Upstream's rewrite already caches the negative result behind a NO_BINDING_INCLUDE_LIST sentinel and resolves the runtime bindable names only on a cache miss, and it keys the cache on whether deny-by-default is enabled, which this branch's single cache could not express. The method is taken from upstream verbatim. * The resolveBindingIncludeList helper. Upstream's getField / getPairedField / getStaticListFieldValue replace it and fix the same defect: the lookup no longer lets getDeclaredField throw for a class the AST transform never enhanced, so nothing is owed here any more. The helper also honoured only a whitelist declared on the class itself, whereas upstream deliberately walks the superclass chain, so DataBindingUtilsSpec now asserts that an inherited whitelist applies. Its test of the private include-list cache is dropped: the negative result is still covered through the public binding API, and upstream now keeps two caches rather than the one the test reached into. Kept from this branch: * The ContextBoundBeans cache of the data binding collaborators, which upstream does not touch. * Resolving the GrailsApplication once per bind and passing it down. It now travels through a private bindObjectToDomainInstance overload which runs upstream's include normalisation, so the include.isEmpty() / NO_BINDABLE_PROPERTIES handling and the clearMissing && explicitInclude gating apply on every path, including bindToCollection.
…er tests Holders keeps its application discovery strategies in a static list and consults them in registration order, and tests share a JVM fork. The spec registered its own strategy but did not clear the list first, so a strategy left behind by an earlier test - holding an application context that had since been closed - was asked first and threw IllegalStateException before the spec's strategy was reached. Clearing in setup as well as cleanup makes the spec independent of whatever ran before it.
Review found the rule applied to some readers and not others, and one place where two readers disagreed inside a single dispatch. Both were mine. The handler mapping was told last time not to resolve "_method" for a forward or an include, because those inherit the parameters of the request that started them and would otherwise have an override re-derived for them. That guard was too broad: it also discarded an override the dispatcher had already resolved and published, so a forward routed as POST while the method-keyed name reader, the render context and link generation all still answered PUT from the attribute. The servlet filter's wrapper reports the overridden method for the whole of a request, forwards included, so discarding it was also the answer that diverged from filter mode. The mapping now honours a published override wherever it applies and refuses only to derive a fresh one for an internal dispatch, which is the case the guard was for. Command object initialization and request body binding still read the wire method. Binding a body is decided by whether the method is one that carries one, so a POST naming DELETE was having its body bound where the filter would have skipped it; command object initialization takes the POST construction path for a domain command object reached without an identifier. Both now read the effective method, and the upgrade note's table lists them. isAjax() reads the resolved multipart request tolerantly too. The first read was made tolerant last time and the second, on the multipart wrapper, was left as it was - which is the read more likely to force the parse that fails. Tests: a forward and an include carrying a resolved override route as that method and agree with what the other readers see; the body of a POST naming DELETE is left unbound while a plain POST still binds. The command object branch needs a GORM-backed controller to reach, so it is covered by the rule and the documentation rather than by a test of its own.
|
All three valid. Fixed in 2234fe9 — though one of them I fixed the other way round, and it is worth saying why. Command object and request body binding — fixed. Both read Stale override on a forward — real, but clearing it is the wrong repair. Under the servlet filter the wrapper reports the overridden method for the whole request, forwards included. Clearing the attribute would make dispatcher mode disagree with filter mode, which is the one thing moving the override was meant not to do. The inconsistency was my forward/include guard from the last round being too broad: it discarded an override the dispatcher had already published, so routing said
On coverage: agreed there is no single end-to-end request-path test, and that remains the honest gap. |
Review has asked twice for a test that exercises the path end to end rather than each piece on its own, and it was a fair thing to keep asking: every part of this branch has unit coverage, and nothing said the parts agree with each other. RequestPathController is mapped as a resources block and reports what one request looked like to each part of the path. The spec drives a real server through the servlet filter chain, so what it asserts is the whole chain at once: a multipart form POST naming PUT is resolved by the dispatcher after multipart parsing, the mapping picks the PUT route, allowedMethods admits it by the effective method, the multipart text field and the uploaded file both reach params, a command object binds from them, and the response renders - while the request itself still reports the POST it arrived as. Five more cases hold the surrounding behaviour in place: a bare POST to the member URL reaches update, which is the route issue apache#9926 asked for and only exists while the filter is off; a form POST naming DELETE reaches delete, and one naming PATCH reaches patch rather than update, which is what the suppressed _method parameter used to break; a POST naming GET stays a POST, since the override is deliberately narrower than the filter it replaces; and a real DELETE routes as itself. Spring Security is the one dimension asked for that this cannot cover - app1 does not have it on the classpath.
|
The end-to-end test now exists: 9763091 adds One round trip asserts the whole chain agrees: a multipart form POST naming PUT is resolved by the dispatcher after multipart parsing, the mapping picks the PUT route, Five more cases pin the surrounding behaviour: a bare POST to the member URL reaches Spring Security is the one dimension on your list this cannot cover — app1 does not have it on the classpath. |
Review asked why the link generator is reached through getGrailsLinkGenerator() here and by property access everywhere else. There is no reason: the explicit call arrived with the trait field cache that this branch added and then removed again, and the removal left the call site behind. Nothing turned on it either way - there is no field named grailsLinkGenerator, so the property resolves to the same getter. Putting it back leaves the trait matching 8.0.x apart from the getCurrentRequest call this branch does have a reason to change. The other call site, at resolveNamespace, is explicit on 8.0.x already and is left as it is.
…8.0.x # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
There was a problem hiding this comment.
I know you did not add this module, but it looks like it was added as a test style project and yet it didn't use our naming convention. This needs to either exist under grails-test-examples or it needs to be excluded under the root build file (see testProjectsStartWith in the root build file). Since this PR is significantly expanding, can you please do either of these?
There was a problem hiding this comment.
Taken the second option in 93c8ffd - grails-benchmarks is now named in testProjectsStartWith.
The two places that key off those prefixes are the BOM enumeration and the functional-test coordinate substitution, and neither has anything to take from it: the module deliberately omits the publish plugin, since its JMH dependency is Category X, so the BOM was already skipping it on the published-project check further down. Naming it says that on purpose instead of leaving it to that second check.
Renaming it under grails-test-examples looked like the worse of the two: the directory is referenced from the JMH workflow, the comparison tooling and the README, and that tree is for applications exercising Grails as an application rather than for a build-time harness. Happy to move it instead if you would rather the convention hold without exception.
There was a problem hiding this comment.
Correcting myself - I took the exclusion first and it was the wrong call. Moved instead, in 14ae491: grails-test-examples-benchmarks, under grails-test-examples/benchmarks, and the testProjectsStartWith entry reverted with it.
You were right that it is a rogue module. It was declared in the framework include(...) block between grails-bootstrap and grails-beans-dsl while being unpublishable, so three separate mechanisms that read the prefix had to be told about it individually - and its own build file already listed four plugins it omits for the same reason. My exclusion would have made five. The prefix now carries the classification instead.
Cost was five files: settings, root build, the workflow's task paths, the RAT exclusion for the golden report fixtures, and the module README. One thing to know: the workflow probes the base revision for the project directory before comparing against it, so a base from before this commit falls back to head-only mode, which it already handles and reports.
compileJmhJava, the module's tests, rat, and the BOM check all pass.
|
@codeconsole why did you rename currentRequest -> request throughout the code? What's the reasoning for this change? |
@jdaugherty |
Because it's been there since 2009 and has a distinguished name. It was clearly used throughout the code base too and resulted in a significant diff to remove it. |
Actually, 2009 is just when 1.1 was merged. It's been there since the start of Grails. |
…8.0.x # Conflicts: # grails-web-databinding/src/test/groovy/org/grails/web/databinding/bindingsource/AbstractRequestBodyDataBindingSourceCreatorSpec.groovy
…rf/request-path-8.0.x
@jdaugherty OLD 2009 IRRELEVANT CODE:public HttpServletRequest getCurrentRequest() {
if (multipartRequest != null) {
return multipartRequest;
}
else {
return getRequest();
}
}replaced with @Deprecated(since = "8.0")
public HttpServletRequest getCurrentRequest() {
return getRequest();
} |
… framework Review pointed out that the module carries neither the grails-test-suite nor the grails-test-examples prefix, so the two places that key off those prefixes treat it as a framework module: the BOM enumerates it as a candidate, and the functional test config offers it for coordinate substitution. Neither has anything to take from it. The module deliberately omits the publish plugin - its JMH dependency is Category X and must not be published - so the BOM already skipped it on the published-project check, and nothing can depend on a coordinate that is never released. Naming it in the list says that on purpose rather than leaving it to a second check further down. Renaming the module was the alternative offered. It reads as the worse of the two: the directory is referenced from the JMH workflow, the benchmark comparison tooling and the README, and grails-test-examples is for applications that exercise Grails as an application rather than for a build-time harness.
You're viewing it legacy because of an implementation detail, while I'm saying it's been a part of the public api since Grails inception and I'm objecting to that removal. |
@jdaugherty it's not removed, it's deprecated. The implementation detail is it no longer has an implementation. It is literally dead code. If it was only created for multi-part requests, how long do you want to keep it for? Do you have another use case you want to morph it into in the future? |
Review asked for one of two things: the module under grails-test-examples, or an exclusion in the root build. The exclusion went in first and was the wrong choice. Every project in the repository carries a prefix that says what it is - four grails-test-suite, sixty-seven grails-test-examples, one grails-doc, and the framework modules. This one was declared in the framework include block between grails-bootstrap and grails-beans-dsl while being unpublishable and existing only to exercise the framework, so three mechanisms that read the prefix - the test project list, the cli auto-provision default, the doc and cli classification - each had to be told about it separately. Its own build file already listed four plugins it omits for the same reason. Naming it in testProjectsStartWith would have made that five, and left the next mechanism to find out the hard way. The prefix now carries the classification, so the exclusion is reverted with it. Renaming costs five files: the settings and root build, the benchmark workflow's task paths, the RAT exclusion for the golden report fixtures, and the module's own README. The workflow also probes the base revision for the project directory before comparing against it; that path will not exist on a base from before this commit, which the workflow already handles by falling back to head-only mode.
@codeconsole We've been removing deprecations every release. I do not agree with it's deprecation or removal. I'm objecting to changing a public API that's been there since Grails started. I'm asking to revert the caller renames to simplify this diff since they are unrelated to the issue being fixed. |
I am trying to understand why don't you agree with its deprecation or removal. Give me a use case. Please help me understand why you want something other than "just because it has always been there" If the only reason of it being added in 2009 was for multi-part and this PR addresses request performance and eliminates the multi-part path, how is it unrelated? Is it performant to call a proxy method that does nothing? Please just give me a single use case how keeping it valuable or settle on a compromise of keeping the rename and removing the Is that a good compromise? remove |
The move went in without the four files that point at it, so settings.gradle still declared ':grails-benchmarks' at a path that no longer exists and every job failed at settings evaluation, before a task ran. Declares ':grails-test-examples-benchmarks' at grails-test-examples/benchmarks, and brings the rest of the rename with it: the benchmark workflow's task paths and its probe of the base revision, the RAT exclusion for the golden report fixtures, and the module's own README.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
I'm highly concerned about the vulnerabilities introduced in this PR, the removal of previous public methods, & the deprecation of the getCurrentRequest() method.
| static UrlMappingInfo[] matchAllUrlMappings(UrlMappingsHolder urlMappingsHolder, String requestUrl, | ||
| GrailsWebRequest grailsRequest, HttpServletResponseExtension extension) { | ||
| String method = grailsRequest.currentRequest.method | ||
| String method = grailsRequest.request.method |
There was a problem hiding this comment.
Effective-method split between security and dispatch (privilege-escalation surface). With the filter off (the new default), the security chain still matches URL mappings against grailsRequest.request.method (a plain POST), while GrailsDispatcherServlet resolves the _method override and UrlMappingsHandlerMapping/AllowedMethodsHelper route and authorize on HiddenHttpMethod.effectiveMethod(...). Combined with the new POST /$controller/$id -> update route, a browser form POST /book/1 with _method=DELETE is authorized by Spring Security as the update mapping, then executed by the dispatcher as delete. If update carries a weaker @Secured/rule than delete, that is an authorization bypass.
The PR upgrade notes acknowledge that update and delete are no longer distinguishable by path; flagging the concrete escalation path so reviewers weigh it explicitly. Consider publishing the resolved method to the request before the security chain runs (or documenting a required security-rule change) so the authorized method and the executed method agree. Otherwise, this is a CVE that can't be merged.
There was a problem hiding this comment.
Confirmed and fixed in a39a350. The mechanism is as you describe: AnnotationFilterInvocationDefinition resolves the URL to a controller/action inside the chain via matchAllUrlMappings, which matched on grailsRequest.request.method. So POST /book/1 with _method=DELETE resolved to the update mapping - reachable at all only because this branch generates that POST route - was authorized against update, then executed as delete.
Matching now resolves the override first, so the action security authorizes is the action that runs. It closes in the safe direction: adding _method can only select the stricter rule, never a weaker one, because both sides read the same resolution. Under the servlet filter the request already reports the overridden method and it resolves to that. Two tests in ReflectionUtilsSpec assert the method the mapping lookup is handed, with and without an override.
| try { | ||
| return read.get(); | ||
| } | ||
| catch (RuntimeException e) { |
There was a problem hiding this comment.
readTolerantly swallows every RuntimeException when the Content-Type is multipart, not only container part-parse failures. The only discriminator is isMultipartContentType(request), so an exception raised for an unrelated reason on a multipart request (a filter-contributed wrapper vetoing getParameterMap(), an already-consumed/aborted body, etc.) is downgraded to a DEBUG log and an empty result. If checkMultipart then succeeds (e.g. lazy resolution with the body within limits, or a different failure mode), the controller runs with silently-empty params and form fields bind as null instead of the request failing. Consider narrowing the catch to the multipart exception types Spring actually raises for a rejected body (e.g. MultipartException / MaxUploadSizeExceededException) rather than all RuntimeException.
There was a problem hiding this comment.
Kept the broad catch but made it non-silent, in a39a350. Narrowing to MultipartException would lose the cases this exists for - the container fails the parameter read itself, and Tomcat and Jetty do not all surface that as a Spring exception. So the fallback still applies to any RuntimeException on a multipart request, but only a MultipartException is the expected case: anything else is logged at warn rather than debug, so an unrelated failure is visible instead of passing as empty params.
| final TryCatchStatement tryCatchStatement = new TryCatchStatement(tryBlock, new EmptyStatement()); | ||
| tryCatchStatement.addCatch(catchStatement); | ||
|
|
||
| if (codeToHandleAllowedMethods.isEmpty()) { |
There was a problem hiding this comment.
Behavior change: controllers that declare no (or an empty) allowedMethods map no longer write the ALLOWED_METHODS_HANDLED request attribute. Previously every action generated the if (attr not set) { ...; set attr } wrapper unconditionally, so a restricted action invoked programmatically or via forward from an unrestricted controller had its method check suppressed (the attribute was already set). With this early-return the attribute is never set for such controllers, so a forward/chain from an unrestricted controller into a method-restricted action now runs that action's allowedMethods check against the original request method and can produce an unexpected 405 mid-request. Worth a test for cross-controller invocation from an allowedMethods-free controller.
There was a problem hiding this comment.
You are right, and it is fixed in a39a350 - every action writes the attribute again. The optimization looked safe from inside the controller being compiled, which is the flaw: the action that reads the attribute is in whichever controller is entered second, so it is not knowable from the one being compiled.
| * able to distinguish; it does add a member URL that answers POST, which the upgrade notes call out. | ||
| */ | ||
| private boolean isPostUpdateVariantEnabled() { | ||
| return grailsApplication != null && !HiddenHttpMethod.isServletFilterMode(grailsApplication.getConfig()); |
There was a problem hiding this comment.
Filter mode is derived only from the two config properties, not from whether a hidden-method filter bean is actually registered. The old registration used @ConditionalOnMissingBean(HiddenHttpMethodFilter.class), so an application supplying its own HiddenHttpMethodFilter bean was a supported extension point. isServletFilterMode() now checks only grails.web.hiddenmethod.filter.enabled / spring.mvc.hiddenmethod.filter.enabled, so such an app is treated as dispatcher mode: isPostUpdateVariantEnabled() returns true and every resources mapping silently gains a POST /$controller/$id -> update route the app never contemplated (and resolveHiddenHttpMethod is enabled alongside its filter). Consider keying the mode on the actual presence of a filter bean, or documenting that a self-registered filter must also set the property.
There was a problem hiding this comment.
Documented rather than detected, in a39a350. Keying the mode on bean presence means resolving a filter bean from three call sites during evaluation, one of which runs while bean definitions are still being registered, so I would rather not make mode resolution order-dependent in this PR. The upgrade note now says plainly that an application registering its own HiddenHttpMethodFilter must set the property too, otherwise it gets its filter and dispatcher mode, including the generated POST member route.
| public void setMultipartRequest(HttpServletRequest multipartRequest) { | ||
| this.multipartRequest = multipartRequest; | ||
| this.originalParams = null; // originalParams will need to be re-initialized. See https://github.com/apache/grails-core/issues/13837 | ||
| public void multipartRequestResolved() { |
There was a problem hiding this comment.
Public setMultipartRequest(HttpServletRequest) was removed outright — unlike getCurrentRequest() it is neither @Deprecated nor mentioned in the upgrade guide, and its replacement multipartRequestResolved() has different semantics (it takes no request and only nulls the cached params). A plugin or test harness that previously installed a resolved multipart request via webRequest.setMultipartRequest(resolved) now fails to compile/link with no drop-in replacement, and even after switching to multipartRequestResolved() must additionally set WebUtils.MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE for the request to be discoverable. Consider keeping a deprecated shim or calling this out in the upgrade notes alongside getCurrentRequest().
This is a breaking API change with no prior release deprecating the method and needs fixed.
There was a problem hiding this comment.
Fixed in a39a350. setMultipartRequest(..) is back, @Deprecated(since = "8.0"), publishing its argument as WebUtils.MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE and discarding the cached params, so an existing caller keeps working. Upgrade guide 54.4 now covers it beside getCurrentRequest(), with the attribute-plus-multipartRequestResolved() form for callers who want to move off it.
| if (resolved != request.getMethod() || WebUtils.isForwardOrInclude(request)) { | ||
| return resolved | ||
| } | ||
| HiddenHttpMethod.resolveOverride(request) ?: resolved |
There was a problem hiding this comment.
Fallback override is used for routing but never published as HiddenHttpMethod.OVERRIDDEN_METHOD_ATTRIBUTE. When the dispatcher did not resolve the override (e.g. a stock DispatcherServlet replaces GrailsDispatcherServlet, or the handler mapping is driven standalone), this branch derives resolveOverride(request) and matches mappings against it, but effectiveMethod(request) / AllowedMethodsHelper still see the raw POST. The request is then routed to the update/delete mapping yet fails its generated allowedMethods check with 405. If this fallback is meant to fully stand in for the dispatcher, it should also set the overridden-method attribute so the two agree.
There was a problem hiding this comment.
Fixed in a39a350 - the fallback now publishes what it resolves, so effectiveMethod and allowedMethods agree with the route it picked. Without it the request reached delete and was then refused a 405 for the POST it arrived as, exactly as you describe. Test added.
| return null; | ||
| } | ||
| String candidate = requested.toUpperCase(Locale.ROOT); | ||
| return OVERRIDABLE_METHODS.contains(candidate) ? candidate : null; |
There was a problem hiding this comment.
Silent narrowing of override handling in the new default mode. resolveOverride only honours a _method parameter on a POST, restricted to PUT/PATCH/DELETE; the previously-registered Grails filter also honoured the X-HTTP-Method-Override header and any method name. In default (dispatcher) mode the header is now ignored and an out-of-set _method is silently dropped rather than applied. This is called out in the upgrade guide, but it is a silent runtime change for header-based REST clients (e.g. a proxy that rewrites DELETE to POST + X-HTTP-Method-Override) — such a client's POST /books/1 now matches the new POST -> update route and returns 200 from update while the caller believes it issued a DELETE. Worth ensuring this is prominent in the migration notes.
There was a problem hiding this comment.
Made prominent in a39a350. The note now spells out the silent case rather than only stating the narrowing: a proxy or SDK that rewrites DELETE to POST plus X-HTTP-Method-Override has its POST /books/1 answered by update with a 200 while the caller believes it issued a DELETE, and the remedy - enable the filter for an application such a client talks to - is stated with it.
The security chain resolves a URL to a controller and action of its own accord, to find the rules that apply, and it matched on the method the request arrived as. The dispatcher resolves the override afterwards. So a form POST to a member URL carrying _method=DELETE was resolved by AnnotationFilterInvocationDefinition to the update mapping - reachable at all only because this branch generates a POST route for that URL - authorized against update's rules, and then executed by the dispatcher as delete. An application whose delete is more restricted than its update had that difference bypassed. Matching now resolves the override first, so the action security authorizes is the action that runs. Adding _method can only select the stricter rule, never a weaker one, because both sides read the same resolution. Under the servlet filter the request already reports the overridden method and this resolves to it. Three more from the same review: The handler mapping's fallback derived an override to route on but never published it, so an action reached that way was refused by allowedMethods with a 405 for the method it arrived as. It publishes what it resolves. Every action writes the ALLOWED_METHODS_HANDLED attribute again. Skipping it for a controller that restricts nothing looked safe from inside that controller: the action which reads it is in whichever controller is entered second, so a forward from an unrestricted controller into a restricted action began checking the original request method against allowedMethods and could answer 405 mid-request. setMultipartRequest is back as a deprecated shim. It was removed outright while getCurrentRequest beside it was deprecated, which is a breaking change with no release deprecating it first. It publishes its argument as the multipart request attribute and discards the cached params, so an existing caller keeps working. A tolerated parameter read still returns the fallback for anything a multipart request throws, since the container failures this exists for are not all MultipartException. But only a MultipartException is expected, so anything else is logged at warn instead of debug rather than passing silently. The upgrade notes gain the two silent cases review asked be made prominent: a client that sends X-HTTP-Method-Override now has its POST answered by update with a 200, and an application registering its own filter bean without setting the property gets dispatcher mode as well as its filter.
The wildcard resources mapping landed on 8.0.x while this branch was open, and its spec asserts the set of routes a resources mapping generates. This branch generates one more - POST to the member URL at update - whenever the hidden method filter is disabled, which is the default it establishes. So the spec counted eight and found nine, in five of its cases. The counts and the enumerated route set now include it, the same way RestfulResourceMappingSpec already does. The wording drops "eight", since the number is a property of the mode rather than of the mapping.
This comment has been minimized.
This comment has been minimized.
5 participants
Description
One PR covering the Grails 8 request path: per-request cost, hidden HTTP method handling, and multipart. This supersedes #16150, #16152 and #16183, which are merged in here with their history intact rather than squashed — they were interdependent enough that reviewing them separately meant tracking a merge order, which is not a reasonable thing to ask of a reviewer.
What is in here
1. Per-request work (was #16149)
Reduces allocation and repeated lookups across the request path: cached per-context collaborators, shared stateless interceptors,
UrlPathHelper.defaultInstanceinstead of per-request instances, and aGrailsApplicationAttributescached per servlet context rather than rebuilt per request. Benchmarks are ingrails-benchmarks.2. URL mapping names from the match (was #16150)
UrlMappingInforesolves controller and action names from the match rather than re-reading request parameters.3. Oversized uploads reach the application's error handling (was #16152, closes #16145)
Reading any parameter of a
multipart/form-datarequest makes the container parse the body, and a body past the configured limit fails that parse inside the filter chain, where noHandlerExceptionResolvercan see it. Worse, once part parsing has failed every later parameter read on that request fails too — including the reads the error handling itself performs.Framework parameter reads now go through one tolerant helper, so the failure is left for
checkMultipartto raise during dispatch where Spring routes it to the exception resolvers.500, raw container HTML413, handled by the application400, indistinguishable from a malformed request413, handled by the application413, empty body4.
requestis always the outermost request (was #16149)Grails used to substitute the resolved
MultipartHttpServletRequestfor the request it exposes, which discarded wrappers contributed by other filters. Multipart capability is now discovered by unwrapping, with an attribute for the case where the wrapper sits above the request Grails bound.GrailsWebRequest.getCurrentRequest()is deprecated and returnsgetRequest().5. The hidden HTTP method filter is now off by default (was #16183)
Grails registered a servlet filter rewriting
POSTintoPUT/PATCH/DELETEfrom a_methodparameter or anX-HTTP-Method-Overrideheader, ahead of the dispatcher and ahead of the Spring Security chain, with no way to turn it off.Reading a request parameter before the dispatcher is not free: the container resolves the dispatcher's
MultipartConfigElementat filter time, so reading_methodon a multipart POST parses the whole body — writing its temporary files — before the request has been routed or authenticated. The filter is order-170; the security chain is-100. Spring Boot disabled its equivalent in 2.2 for the same reason.The override is not lost. It moves into the dispatcher, resolved after multipart handling and after the filter chain, and published as a request attribute so
allowedMethodsand URL mapping resolution agree on the method. Restore the old behaviour withgrails.web.hiddenmethod.filter.enabled: true.Two supporting changes:
POST /$controller/$id→updateis generated by aresourcesmapping.RestfulControllerhas declaredupdate: ['PUT', 'POST']since #9926 — raised because AngularJS$resourceand the clients modelled on it POST to the member URL to save an existing object — but no route was ever generated, so the permission has been unreachable. It is also what lets a form submit without a_methodparameter.<g:form method="PUT">stops emitting_method, submitting a plain POST to the same URL.method="DELETE"still emits it, becausedeleteandupdateshare a URL. In the new default_methodappears in exactly one place: a delete form.The consequence to review before upgrading. Servlet filters and the Spring Security chain now see a form delete as a bare
POST /books/1. A rule matchingDELETE /books/**will no longer fire, and since the URL is unchanged,updateanddeleteare not distinguishable by path either. This is the only change here that fails silently.6. A startup failure new in 8.0
Boot's
WebMvcAutoConfigurationregisters its filter under the samehiddenHttpMethodFilterbean name, sospring.mvc.hiddenmethod.filter.enabled=truefailed startup with aBeanDefinitionOverrideException. Grails' registration moved to its own auto-configuration ordered after Boot's, detecting Boot's filter rather than assuming it from a property — and contributing its own when Boot cannot, which is the case under@EnableWebMvc.Review notes
GrailsWebRequest, @sbglasius onCachedBeanremembering a lookup miss, @matrei on the filter-mode flag being derived from only one of the two properties.Documentation
grails-docupgrade guide sections 52–54, and the REST guide's Linking to Resources page.