Merge 8.1.x into 9.0.x - #16342
Merged
Merged
Merge 8.1.x into 9.0.x#16342
Conversation
Assisted-by: opencode:gpt-5.6-sol
- Rename the Object-typed field to untypedProperty and reword test names: the property is excluded because it is raw Object-typed (DefaultASTDatabindingHelper#shouldFieldBeInWhiteList), not because it is generically "unlisted". - Drop the misleading hasOne association, which adds nothing to the contract under test since the typed address field alone puts it in the generated whitelist, and the mapping is incomplete (no belongsTo back-reference on WhitelistAddress). - Trim the domain test to its non-duplicated assertions and cross-reference DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec, which already pins id/version/dateCreated/lastUpdated exclusion for domain classes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
PR apache#16071 landed a `grails-benchmarks` module using the same package root as `grails-web-benchmarks`, with a comparison tool (`BenchmarkComparator`, `JmhCompare`, `CommentPoster`, sharding and golden-file tests) that automates the before/after comparison this branch was doing by hand. Move the request-path benchmarks into it and delete `grails-web-benchmarks`. Ported, regrouped into the package-per-subsystem layout the report aggregates on, and reworked to that module's conventions - benchmarks in `src/jmh/java`, setup in Groovy fixtures under `src/main/groovy`: controllers ControllerActionBenchmark, ControllerMappingCollectionBenchmark interceptors InterceptorChainBenchmark web GrailsWebRequestBenchmark, MultipartResolutionBenchmark, RequestPropertyAccessBenchmark Because the fixtures live in `main`, which the jmh plugin puts on the jmh compile classpath, the `UrlMappingsDefinition` / `InterceptorFactory` / `RequestPropertyReader` interfaces and their `Class.forName` lookups are gone - the Java benchmarks call the Groovy fixtures directly. `UrlMappingBenchmark` is dropped. Its `matchCachedHit` and `matchRestfulUriCacheMiss` duplicate `UrlMappingsBenchmark.matchWarmCache` and `matchColdVariedKeys`. The one shape it measured that upstream did not - a cold URI that only the catch-all `"/$controller/$action?/$id?"` mapping can serve, so every earlier mapping is considered and rejected before the match succeeds - is added to `UrlMappingsBenchmark` as `matchColdCatchAllFallThrough`. On the existing fixture that costs ~568 ns against ~411 ns for a cold URI the first mapping serves. BASELINE.md is dropped: paired before/after numbers are what the new report tooling produces per pull request. Its measured results, from two full suites run back to back on an idle M4 Max under JDK 21.0.7 (2 forks, 5x1s warmup + 5x1s measurement), were: ControllerActionBenchmark.plainAction 34.795 -> 3.592 -89.7% InterceptorChainBenchmark.oneInterceptorNoOpRegistry 295.293 -> 127.541 -56.8% InterceptorChainBenchmark.threeInterceptorsNoOp 1116.942 -> 545.608 -51.2% InterceptorChainBenchmark.threeInterceptorsObserving 1972.929 ->1134.393 -42.5% GrailsWebRequestBenchmark.construct 16.283 -> 11.710 -28.1% UrlMappingBenchmark.matchRestfulUriCacheMiss 1549.992 ->1287.754 -16.9% UrlMappingBenchmark.matchDefaultMappingUriCacheMiss 1881.323 ->1595.361 -15.2% The request-attribute counts `ControllerActionBenchmark` prints at setup are the independent evidence for the controller result and are unchanged by the move: an action on a controller with no `allowedMethods` performs no attribute operations at all, while one that declares `allowedMethods` still performs 2/1/1. `grails-benchmarks/build.gradle` gains `:grails-controllers`, `:grails-web-databinding`, `:grails-mimetypes`, `micrometer-observation` and `spring-webmvc`. The suite stays opt-in: `build` compiles the benchmarks, only the explicit `jmh` task runs them.
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.
Three caches were added as Groovy trait fields. A trait is a mixin and its fields are replicated into every implementing class, so that is per-controller scope for things that are application scoped. Groovy also remaps trait statics per implementing class, so the "static" namespace cache was really one Class-keyed map per controller class, each holding a single entry; and the redirector needed an AtomicReference only because Groovy silently drops volatile on trait fields. - The namespace is a static property of the controller class and DefaultGrailsControllerClass already resolves it at construction, so the reflective read and its cache are both gone - the namespace now comes from the artefact registry, keyed on the class issuing the redirect rather than the one currently executing. - CompositeViewResolver moves to DefaultGrailsApplicationAttributes, alongside the other beans it describes as used very often. That object is one instance per servlet context, so the resolver is now resolved once for the application rather than once per controller. - The ResponseRedirector cache is removed rather than relocated. Its inputs are uniformly injected, so it is effectively a singleton, but the trait's setters are public API and are exercised after construction - RedirectMethodTests registers a listener on a live controller and expects the next redirect to notify it - so a shared instance would need a per-controller override path anyway. It is built per redirect again, as before. This also removes the fields from Interceptor, which implements the same trait.
Neither had a benchmark, which is how three unmeasured caches ended up in the controller traits.
An upload breaching the configured multipart limits fails when the container parses the request parts, and every parameter read on that request fails with it from then on. Grails reads request parameters twice before the DispatcherServlet runs - HiddenHttpMethodFilter resolves the _method override, and GrailsParameterMap is built by any filter ahead of the dispatch, Spring Security among them. Either read aborted the request inside the filter chain, where no HandlerExceptionResolver can see it, so the application was left with the servlet container's own error page: a raw 413 on Tomcat, with the container's error page dispatch double-faulting on the same unreadable parameters. Both reads now tolerate a multipart request the container refuses to parse, so the failure is left for DispatcherServlet.checkMultipart to raise as a MultipartException during dispatch. The exception reaches the HandlerExceptionResolver chain and the response is rendered through the application's error dispatch. The tolerance is confined to multipart requests - an unreadable parameter map on any other request still propagates - and the parse failure is logged at debug rather than discarded. Such a request cannot reach a controller either way, because the dispatcher rejects it before handler resolution. - Add WebUtils.isMultipartContentType, shared by both call sites - Cover both the tolerated and the still-propagating case in the existing tests apachegh-16145
app1 runs a real embedded container with the Spring Security filter chain in front of it, which is the configuration both halves of apachegh-16145 describe. Neither half had a functional test. - An upload past the configured limit must be rendered by the application's error dispatch rather than by the container. Without the accompanying fix this returns Tomcat's own 413 HTML page. - g:uploadForm(method: 'PUT') emits multipart plus _method, so the override has to keep working for a multipart request rather than being skipped along with the parameter read. The existing specs already cover request.getFile(..) behind the security filter chain, which is the other behaviour the issue reports as broken. apachegh-16145
A multipart body the container refuses to parse - an upload past the configured limits - leaves every parameter read on that request failing from then on. Two of Grails' own reads already tolerated that: the _method override in HiddenHttpMethodFilter, and the GrailsParameterMap constructor. They were not the only ones. ParamsAwareLocaleChangeInterceptor falls back to LocaleChangeInterceptor, which reads the parameter straight off the request, and it runs on every Grails-mapped dispatch - including the container's error dispatch for the very failure being reported. That read threw and took the error page down with it: an oversized upload to an application with a "413" UrlMappings handler returned the container's raw 500 page instead of the mapped response. GrailsExceptionResolver enumerates the request parameters for its log when grails.exceptionresolver.logRequestParameters is set, which defaults to on in development, and GroovyPageView reads showSource when it renders a GSP in development - both on that same error path. The tolerance moves into WebUtils as readParameterMap, readParameter and readParameterNames, so one place decides what an unreadable multipart request yields, and one place keeps the tolerance confined to multipart requests. Every framework read goes through it. Verified against two containers, because the catch is deliberately not container-specific. Tomcat 11 throws org.apache.tomcat.util.http.InvalidParameterException from every parameter accessor; Jetty 12 throws org.eclipse.jetty.http.HttpException$IllegalStateException from getParameterMap while getParameter and getParameterNames succeed. Catching RuntimeException covers both. apachegh-16145
…-path Canonicalize interceptor and security matcher request paths
…E-tolerant for classpath descriptors
…alidation Validate dynamic finder and HQL list sort property names
…dening # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
Fix SAX parser feature URIs and reject DOCTYPE declarations
…ils-micronaut refactor: move Grails Micronaut to apache/grails-micronaut
…contract Characterize data-binding allowlist through public binding API
Merge 8.0.x into 8.1.x
Conflicts resolved by hand: - settings.gradle, dependencies.gradle, .github/workflows/gradle.yml: take the 8.1.x removal of the Grails-Micronaut island; keep the 9.0.x Groovy 6.0.0-beta-2 pin and the 9.0.x functional-test hang workaround. - XmlUtils.groovy: take the 8.1.x XML parser hardening (XmlParserFeature, DOCTYPE rejected); keep the 9.0.x ACCESS_EXTERNAL_DTD/SCHEMA properties.
The 8.1.x CompilePlugin compiles every module with indy off. On Groovy 6 classic call-site bytecode needs the optional groovy-callsite module on the compile classpath (GROOVY-11158), which only modules depending on grails-common carry, so grails-bootstrap, grails-codecs-core, grails-events-core and six other modules stopped compiling after the merge. Default grailsIndy to true on this line, which is what plain library modules compiled with before the merge. Grails plugin modules are unaffected: the Grails Gradle plugin applies its own indy-off default after evaluation and carries groovy-callsite through grails-common. -PgrailsIndy=<boolean> still overrides both.
…Indy
Honouring -PgrailsIndy=false in CompilePlugin made the indy=false CI cells
compile every library module with classic call sites, which on Groovy 6
need the optional groovy-callsite module that only modules depending on
grails-common carry. Injecting groovy-callsite from CompilePlugin is not
viable either: the grails-gradle modules compile with Gradle's Groovy 4
while their build's BOM map names Groovy 6, so the injected jar pulled
Groovy 6 onto that classpath and grails.util.Environment failed bytecode
verification at configuration time.
Grails 9 is indy-on, so CompilePlugin now sets indy=true unconditionally.
-PgrailsIndy still reaches the Grails plugin modules through
grails-extension-gradle-config.gradle, and applications opt out with
grails { indy = false }.
Groovy 6.0.0-beta-2 static type checking merges the flow state of a || inside a closure to void, so GrailsWebDataBinder failed to compile at the collection branch with "Cannot find matching method ArrayList#leftShift(void)". The array and Map branches already carry the same hoist; this applies it to the collection branch.
… Groovy 6 Groovy 6 dispatches attrs['gspTagSyntaxCall'] = value on a Map to put() rather than to the bean setter, so the subscript half of testAssigningGspTagSyntaxCallInvokesTheSetter failed on this line. The dotted form still invokes the setter, which is what TagOutput relies on. Split the subscript form into its own case that asserts the Groovy 6 behaviour: the attribute is stored and the flag is left alone.
The end-to-end projects compile their Spock specs against Groovy 6 with Spock 2.4-groovy-5.0, and the transform refuses to run: IncompatibleGroovyVersionException in taglib-index-incremental compileTestGroovy. The framework build already passes -Dspock.iKnowWhatImDoing.disableGroovyVersionCheck=true to the Groovy compiler daemon and the test JVM from CompilePlugin; do the same for the end-to-end subprojects.
The step that derives the Apache Groovy branch required the closing quote straight after the patch number, so the 9.0.x pin '6.0.0-beta-2' never matched and the job failed before checking anything out. Accept a qualifier after the patch number; major 6 already maps to master.
🚨 TestLens detected 1 failed test 🚨Here is what you can do:
Failed Jobs without Test Failures❌ Groovy Snapshot Canary Build / Build Grails (shard 0) Test SummaryCI / Build Grails-Core (Windows JDK 25 shard 0) > :grails-events-rxjava:test
🏷️ Commit: fa6bde1 Test FailuresPublishSubscribeSpringSpec > Test event publisher within Spring (:grails-events-rxjava:test in CI / Build Grails-Core (Windows JDK 25 shard 0))Rerun ControlsNote Checks are currently running using the configuration below. Select tests to mute in this pull request: 🔲 PublishSubscribeSpringSpec > Test event publisher within Spring Reuse successful test results: 🔲 ♻️ Only rerun the tests that failed or were muted before Click the checkbox to trigger a rerun: 🔲 Rerun jobs Learn more about TestLens at testlens.app/docs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release-line merge of
8.1.xinto9.0.x, not a squash.projectVersionremains9.0.0-SNAPSHOT.Five files conflicted and were resolved by hand in the merge commit:
settings.gradle,dependencies.gradle,.github/workflows/gradle.yml: took the 8.1.x removal of the Grails-Micronaut island (refactor: move Grails Micronaut to apache/grails-micronaut #16320). Kept the 9.0.x Groovy6.0.0-beta-2pin and the 9.0.x functional-test hang workaround.XmlUtils.groovy(grails-testing-support-http-client): took the 8.1.x XML parser hardening (Fix SAX parser feature URIs and reject DOCTYPE declarations #16331,XmlParserFeature, DOCTYPE rejected) and kept the 9.0.xACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_SCHEMAparser properties.Two follow-up commits make the branch build:
CompilePlugin keeps invokedynamic on for the framework's library modules. build: disable Groovy invokedynamic for the Grails 8 compile #16178 compiles every module indy-off via
CompilePlugin, and honours-PgrailsIndy=false. On Groovy 6 classic call-site bytecode needs the optionalgroovy-callsitemodule (GROOVY-11158), which only modules depending ongrails-commoncarry, sograils-bootstrap,grails-codecs-core,grails-events-coreand six others stopped compiling, and theindy=falseCI cells would do the same. Grails 9 is indy-on, soCompilePluginnow setsindy=trueunconditionally on this branch.-PgrailsIndystill governs the nine Grails plugin modules throughgrails-extension-gradle-config.gradle(they getgroovy-callsitefromgrails-common), and applications still opt out withgrails { indy = false }.CompilePluginSpecupdated.GrailsWebDataBindercollection branch gets the Groovy 6||flow-state hoist that the array and Map branches already had on 9.0.x. Without itgrails-web-databindingfails static type checking at line 560 (ArrayList#leftShift(void)). This was already failing on upstream 9.0.x before the merge (see the 9.0.x CI run for 51b1528); it is included here because it blocks every downstream module.GroovyPageAttributesTestssubscript case updated for Groovy 6. Groovy 6 dispatchesattrs['gspTagSyntaxCall'] = valueon a Map toput()rather than to the bean setter, so that half of the test failed on 9.0.x before the merge. The dotted form still invokes the setter, which is whatTagOutputrelies on. The subscript form now has its own case asserting the Groovy 6 behaviour.End-to-end build disables Spock's Groovy version check, the same
-Dspock.iKnowWhatImDoing.disableGroovyVersionCheck=truethe framework build passes fromCompilePlugin. Without ittaglib-index-incremental:compileTestGroovyfails withIncompatibleGroovyVersionException(Spock 2.4-groovy-5.0 on Groovy 6.0.0-beta-2).Groovy snapshot canary parses a pre-release pin. The branch-derivation step required the closing quote straight after the patch number, so
'6.0.0-beta-2'never matched and the job failed in seconds. A qualifier after the patch number is now accepted; major 6 already maps to Groovy's master branch.