matrei commented on PR #16237:
URL: https://github.com/apache/grails-core/pull/16237#issuecomment-5726461771
# Review round 3
**Head:** `ab080d24ae173820e0b186b47d437abf74cfa4d9`
(`codeconsole:feature/spring-media-type-negotiation-8.0.x`)
**Base:** `8.0.x` — merge-base still `3067d0a855`, clean merge on top of it.
**Delta since round 2:** one commit, `ab080d24ae` (13 files, +188/−34).
**What I ran locally:**
- `./gradlew codeStyle` — green.
- Module tests with `--no-build-cache` and `cleanTest` (result XML confirmed
fresh), 0 failures: `grails-converters` (115), `grails-rest-transforms` (87),
`grails-web-common` (97), `grails-testing-support-core` (10), `grails-xml`
(29), `grails-gsp` plugin (666), `grails-test-suite-web` (454),
`grails-test-suite-uber` (576).
- `:grails-test-examples-hibernate7-demo33:test --tests
demo.PersonControllerHibernateSpec` — **fails**, same as CI (see point 1).
- Two scratch specs, since deleted: a `HibernateSpec` + `ControllerUnitTest`
probe in demo33 that prints singleton state before `controller` is touched, and
a `ControllerUnitTest` in `grails-test-suite-web` covering the plugin list,
bean count, link generation and a pretty-printed scalar root.
- CI on this head is **red**: 6 jobs. Four are the demo33 failure below
(Build Grails shard 1, three Hibernate7 functional jobs), one is the
known-flaky Geb login test (`UserControllerSpec > User list`, #16030), and the
macOS job timed out at 6h.
**Verdict: round-2 findings are resolved, but the harness fix introduced a
real regression that fails CI. One blocking item, one should-fix.** The curated
plugin graph is back (`core` + `restResponder`, 152 bean definitions,
`CompiledTagInvocationSpec` passes without its workaround), duplicate
definitions are gone, and every round-2 minor was addressed. What is left is a
bean cycle in `XmlGrailsPlugin` that the new registration order exposes, and an
incomplete `JSONWriter` change.
---
## Blocking
### 1. `xmlRenderer` ↔ `groovyPageLocator` cycle fails `HibernateSpec`
controller tests (CI red)
`PersonControllerHibernateSpec` in `grails-test-examples/hibernate7/demo33`
fails deterministically, locally and in four CI jobs:
```
BeanCreationException: Error creating bean with name 'xmlRenderer':
Instantiation of supplied bean failed
at GrailsUnitTest.defineBeans(GrailsUnitTest.groovy:110)
at GrailsWebUnitTest.mockController(GrailsWebUnitTest.groovy:132)
Caused by: UnsatisfiedDependencyException: Error creating bean with name
'groovyPageLocator': Unsatisfied dependency
expressed through method 'setMimeTypeResolver' parameter 0: Error creating
bean with name 'xmlRenderer':
Requested bean is currently in creation
at
XmlGrailsPlugin$_beanRegistrar_closure1.closure2$_closure5(XmlGrailsPlugin.groovy:75)
Caused by: BeanCurrentlyInCreationException: ... 'xmlRenderer'
at DefaultListableBeanFactory.resolveMultipleBeanCollection <-
Collection<MimeTypeProvider>
at ConstructorResolver.autowireConstructor <-
MimeTypesConfiguration
at ConstructorResolver.instantiateUsingFactoryMethod <-
mimeTypeResolver
```
The cycle is in production code, not in the harness:
`xmlRenderer` supplier (`XmlGrailsPlugin.groovy:75`,
`beanProvider(GrailsConventionGroovyPageLocator).getIfAvailable()`) →
`groovyPageLocator.setMimeTypeResolver`
(`GrailsConventionGroovyPageLocator.java:59`) → `mimeTypeResolver` factory
method (`MimeTypesConfiguration.groovy:123`) → `MimeTypesConfiguration`
constructor, which autowires every `MimeTypeProvider` → `Renderer extends
MimeTypeProvider` (`Renderer.groovy:29`) → `xmlRenderer`, which is currently in
creation. It is constructor injection on one side and a supplier on the other,
so `allowCircularReferences` cannot break it.
It only stays hidden while `mimeTypesConfiguration` happens to be
instantiated before `xmlRenderer`, or while `groovyPageLocator` does not exist
yet. In an ordinary `ControllerUnitTest` the second condition holds:
`xmlRenderer` is built at refresh, before `WebSetupSpecInterceptor` defines the
locator (my probe: `xmlRenderer` singleton present, its `groovyPageLocator` is
`null`). The Hibernate 7 `HibernateSpec` takes its "context already exists"
branch (`HibernateSpec.groovy:154`, `configureForBeanDefinitionRegistry` on the
live context), which registers definitions into the already refreshed context;
I did not trace which registration does it, but the effect is that singletons
built at refresh are gone afterwards. My probe in that spec, before
`controller` is touched:
```
XML_SINGLETON: false
LOCATOR_DEF: true
MIME_CFG: [mimeTypesConfiguration=false]
```
The next `defineBeans` → `preInstantiateSingletons()` then recreates
`xmlRenderer` with the locator defined and `mimeTypesConfiguration` not yet
built, and the cycle closes. The H5 `demo33` has the same spec and `grails-xml`
but no such branch, so it passes. Round 2's head was green because the
plugin-manager registration order differed; the hazard itself was already
there. A real application with `grails-xml` and GSP has the same ordering
dependence; the functional tests pass, so the order is favourable today, but
nothing guarantees it.
Suggested fix, in the plugin rather than the harness: do not resolve the
locator inside the `xmlRenderer` supplier. Either make the locator lazy (pass
an `ObjectProvider`/`Supplier<GrailsConventionGroovyPageLocator>` into
`DefaultXmlRenderer` and resolve on first view lookup), or drop it from the
bean and let `DefaultRendererRegistry` hand its own autowired
`groovyPageLocator` (`DefaultRendererRegistry.groovy:66`) to renderers when
they are registered. The lazy variant also fixes the side effect noted under
Minor. Please add a test that reproduces the ordering (define
`groovyPageLocator`, destroy the `xmlRenderer` and `mimeTypesConfiguration`
singletons, call `preInstantiateSingletons()`), so this does not depend on an
example app to be caught.
## Should fix before merge
### 2. Scalar JSON roots: `PrettyPrintJSONWriter` was not updated, and the
upgrade guide now contradicts the code
`JSONWriter.append(Writable)` now accepts `INIT` and moves to `DONE`
(`JSONWriter.java:113-126`), which is what makes `respond 'ok'` and `render
Role.HEAD as JSON` work on the legacy path. `PrettyPrintJSONWriter` overrides
`append` with its own copy of the old condition
(`PrettyPrintJSONWriter.java:80-103`, still `mode == OBJECT || mode == ARRAY`),
and `JSON.render` picks that writer whenever `prettyPrint` is on
(`JSON.java:111`). Scratch spec on this head:
```groovy
def json = new JSON('ok'); json.prettyPrint = true; json.render(out)
// ConverterException: JSONException: Value out of sequence: expected mode
to be OBJECT or ARRAY when writing 'ok' but was INIT
```
Same result through a controller with `render json`. So with
`grails.converters.json.pretty.print=true`, `respond 'ok'`, a root enum, number
or boolean still 500. `JSONWriterSpec` only covers the base class; the new
table should run against both writers.
This is also a user-visible behaviour change that goes beyond `respond`:
every `x as JSON` / `new JSON(x)` with a scalar root now renders instead of
throwing. `upgrading80x.adoc` section 25 ("Rendering Enum values as JSON", line
1095) still says rendering a single enum "will now instead throw a
`ConverterException`", and `JSONConverterTests.testJSONEnumConverting` was
flipped from asserting that exception to asserting `"HEAD"` in this commit.
Section 25 needs rewriting to the new outcome (a JSON string of the enum name,
via `SimpleEnumMarshaller`), and the scalar-root change deserves a sentence of
its own.
## Minor
- In web unit tests `xmlRenderer` is built before `groovyPageLocator`
exists, so its locator is `null` for the life of the spec and `respond` with
`Accept: application/xml` never looks for a GSP view. On `8.0.x` the registry
created the default XML renderer with its own autowired locator. The lazy
locator from point 1 restores that.
- `registerPluginBeans` (`GrailsApplicationBuilder.groovy:205-211`) guards
the XML plugin with `ClassUtils.isPresent` but loads `ConvertersGrailsPlugin`
unconditionally. It holds today because `grails-testing-support-web` brings
`grails-rest-transforms` → `grails-converters` via `api`, but the core module
excludes `grails-converters` (`build.gradle:48`), so the same guard would be
cheap insurance.
- `NamedJsonConfigurationRegistry.resolveMapper` calls the supplier, which
does Spring bean lookups, while holding the registry monitor. No deadlock path
that I can construct at request time, but a plain racy read-then-write of the
`volatile` would do the same job without a lock (`forGrails` is idempotent per
mapper).
- `DefaultJsonRenderer.groovy:62`: the new instance field sits above the
`static final PROBLEM_JSON` constant.
## Round-2 findings, status
| # | Finding | Status |
|---|---------|--------|
| 1 | Web unit tests booted the full controllers/urlMappings/i18n graph |
Fixed as suggested. Only `restResponder` is added to the filter
(`GrailsApplicationBuilder.groovy:232`); the converters and XML bean registrars
are applied directly before refresh (`:188-199`), skipped when the test
includes those plugins itself. Measured: plugins `[core, restResponder]`, 152
bean definitions, harness URL mapping `/(*)/(*)?/(*)?` as on `8.0.x`.
`CompiledTagInvocationSpec` lost its `controllerName` workaround and passes.
`ControllerJsonSerializationSpec` asserts the plugin set, no `UrlMappings`
artefact, and that an application `JsonMapperBuilderCustomizer` sees the same
`GrailsJsonMapperCustomizer` bean. Documented in `unitTesting.adoc` and section
65. Side effect: point 1 above. |
| 2 | Beans defined twice in the web harness | Fixed. `rendererRegistry`,
the first `localeResolver` and the post-refresh `ConvertersGrailsPlugin` are
gone from `WebSetupSpecInterceptor`; `grailsUrlMappingsHolder`,
`grailsLinkGenerator` and `localeResolver` are guarded by `containsBean`. |
| minor | `respond 'ok'` on the legacy path | Fixed for the default writer
(`new JSON(object)`, `DefaultJsonRenderer.groovy:173-176`, plus the
`JSONWriter` change). Not for pretty print, see point 2. |
| minor | Converter rebuilt per response | Fixed, cached per source
converter (`DefaultJsonRenderer.groovy:207-209`); the spec alternates two
source converters with different naming strategies. |
| minor | Registry supplier scanned on every write | Fixed, first non-null
mapper is cached and absence is retried; covered in
`NamedJsonConfigurationRegistrySpec`. |
| minor | `isWebTest()` overrides `getIncludePlugins()` undocumented | Fixed
in `unitTesting.adoc` and section 65. |
## Verified as correct
- Link generation in the web harness behaves as on `8.0.x`: a link with only
`action` fails against the curated `/(*)/(*)?/(*)?` mapping, not against
`DefaultUrlMappings`; the urlMappings plugin is not loaded.
- `encodeAsXML()` needing `mockCodec(XMLCodec)` in unit tests is unchanged
from `8.0.x`; registering only the XML bean registrar (not `providedArtefacts`)
does not regress it.
- The `grailsConverters` cache key is the source converter instance
(`HttpMessageConverter` has identity equality), and `forGrails` is itself
cached per mapper, so two converters sharing a mapper still share the derived
mapper.
- `JSONWriter` base class: a second root value after a scalar throws and
leaves the output untouched; `object()`/`array()` after `DONE` still throw.
`JSONWriterSpec` covers string escaping, `GString`, numbers, booleans and
`null`.
- The scaffolding `UserControllerSpec > User list` failure is the known Geb
login flake (#16030), unrelated to this PR.
- Everything in the round-1 and round-2 "Verified as correct" lists is
unchanged by this commit.
--
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]
