matrei commented on PR #16184:
URL: https://github.com/apache/grails-core/pull/16184#issuecomment-5632355856
# AI Review
Verified the framework changes against the modules they touch and ran the
example. The lifecycle gate, the whole-configuration gate on
`CoreAutoConfiguration`, the auto-proxy creator move (Spring 7.0.9 still keeps
`APC_PRIORITY_LIST` as a mutable `ArrayList`, and the order of the two creators
is preserved), the write-whole view registry (up-to-date pages are
re-registered on every run, and the two compile tasks write to separate
directories) and the SiteMesh alias handling all check out. `@Integration` is
unaffected by the gate because `GrailsApplicationContextLoader` launches
through `GrailsApp`. One defect, a few test and doc gaps, and two design notes
below.
### Bug: `Sitemesh3LayoutTagLib` renders attribute values unencoded in a
standalone application
`TagLibraryLookupRegistrar.createTagLibBeanDefinition` registers the tag
libraries with no autowire mode, relying on each class's own `@Autowired`
declarations. `RenderTagLib` and `RenderSitemeshTagLib` declare them;
`Sitemesh3LayoutTagLib` does not - its `codecLookup` is a plain property that a
Grails application fills by name through `TagLibBeanDefinitionsPostProcessor`
(`AUTOWIRE_BY_NAME`). In the gsp-spring-boot example the field is null (probed
with a `@SpringBootTest`: `renderTagLib.codecLookup` is a `DefaultCodecLookup`,
`layoutTagLib.codecLookup` is `null`), so `captureTagContent` takes its
`htmlEncoder == null` branch and writes every attribute of the captured
`<head>`, `<body>`, `<title>` and friends verbatim. A layout or page with
`<body class="${...}">` puts the raw value into the response.
Fix: `@Autowired` on `Sitemesh3LayoutTagLib.codecLookup` (as
`RenderSitemeshTagLib` does), and a test that a captured attribute value is
HTML-encoded in the standalone context - either in
`TagLibraryLookupRegistrarSpec` or as a rendered-page assertion in the example.
### Tests
- `CoreGrailsPluginRegistrarSpec` ("nothing here loads
GrailsAutoConfiguration"): `APC_PRIORITY_LIST` is JVM-global and
`test-config.gradle` sets `forkEvery = 100`, so any spec that touched
`GrailsAutoConfiguration` earlier in the fork
(`EarlyPluginRegistrationOrderingSpec`, `GrailsAutoConfigurationArtefactsSpec`,
...) has already patched the list. Reverting the `registerWithAopConfigUtils()`
call in `CoreGrailsPlugin` does not reliably fail this test.
- `GroovyPageCompilerSpec` never sets `generatedGroovyPagesDirectory`, so
the compiler falls back to `${java.io.tmpdir}/gspcompile`, shared across
parallel forks and never cleaned. Point it at the `@TempDir`.
- `GspAutoConfigurationSpec` exercises only the protected
`resolvePrecompiledViews`. The public surface - that the `groovyPageLocator`
bean receives the map, and the new `paths.add(cleanUri(uri))` search path - is
covered only by the example's `PrecompiledViewTest`; nothing in the module
fails if either is reverted.
- `GroovyPagePluginFunctionalSpec`: the non-Grails project asserts the pages
are on the test runtime class path but not that `test` depends on
`compileGroovyPages`; the Grails project asserts the negative. The `builtBy`
wiring is the whole point of the change and is unverified.
- No test asserts that `CoreAutoConfiguration` stays out of the standalone
context once `GspAutoConfiguration` contributes its own `GrailsApplication`.
That holds today only because Core is `@AutoConfigureOrder(HIGHEST_PRECEDENCE)`
and is evaluated before the standalone `grailsApplication` bean definition
exists; if that ordering ever changed, core beans would land silently. A
`context.containsBean('pluginManager') == false` in the example would pin it.
### Docs
- `upgrading80x.adoc` (SiteMesh note) names
`Sitemesh3EnvironmentPostProcessor`, an internal class (rule 8). The sentence
works without it.
- Nothing user-facing documents the standalone additions: precompiled views
read from `classpath:gsp/views.properties` and skipped when a `file:` template
root is in use, the `serverpath = '/'` requirement, the empty
`grailsUrlMappingsHolder`, and `@Artefact("TagLib")` beans being picked up. The
module README is unchanged. Rule 7 asks for doc coverage with the feature.
### Design notes
- `@Bean(name = {"gspViewResolver", "jspViewResolver"})`: a Spring Boot
application that also serves JSP may well have its own `jspViewResolver`. User
configuration is processed before auto-configuration, and
`SimpleAliasRegistry.registerAlias` does not check bean definition names, so
the alias would silently take over `getBean("jspViewResolver")` rather than
fail. Registering the alias only when no bean of that name exists (the
`ReplaceViewResolverRegistrar` already does this kind of thing) would be safer.
- Example: `JspViewController`'s Javadoc says it "is mapped only where a JSP
can be served"; it is always mapped and the guard is the runtime `canServeJsp`
check. The `private static boolean jsp` in `WebController` is application-wide
state that `JspViewTest` has to reset; session state would be the idiomatic
example.
### Process
The thread still has the open question of whether this lands on 8.0.x before
RC1; the lifecycle gate changes what a Grails application's
`@SpringBootTest(classes = SomeConfig)` gets, which is the kind of change the
"review then commit" policy is for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
