Skip to content

Modernize MIME negotiation and serialization for Spring Boot 4.1 - #16237

Open
codeconsole wants to merge 86 commits into
apache:8.0.xfrom
codeconsole:feature/spring-media-type-negotiation-8.0.x
Open

codeconsole wants to merge 86 commits into
apache:8.0.xfrom
codeconsole:feature/spring-media-type-negotiation-8.0.x

Conversation

@codeconsole

@codeconsole codeconsole commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Modernize Grails MIME negotiation and provide an opt-in Spring/Jackson JSON rendering path on 8.0.x, while preserving legacy JSON rendering as the default.

  • Bridge Grails MIME types to Spring MediaType, remove static negotiation state, and resolve strategy bean ambiguity.
  • Integrate Spring MVC message converters with Grails rendering. JSON/XML selection requires a supported media type in the requested family, so generic string and byte-array converters cannot claim structured payloads through */*.
  • Provide an injectable, request-safe NamedJsonConfigurationRegistry shared by controller rendering and direct serialization.
  • Scope domain compatibility serialization to Grails writers; preserve Jackson's normal property model on the shared Boot mapper.
  • Extract HTTP XML payload support into optional grails-xml, keep plugin descriptors unchanged, and make new REST artefacts JSON-only by default.
  • Support RFC 9457 validation responses through Spring JSON rendering; retain explicit Vnd.Error, XML, HAL, and Atom renderer options.
  • Initialize controller unit tests with Boot Jackson configuration and real message converters. Restrict automatic web/XML plugin loading to web tests.

Compatibility and migration

  • grails.web.rendering.json.spring defaults to false on 8.0.x. Set it to true to opt into Spring JSON rendering. Legacy marshallers and ordinary legacy response shapes remain the default.
  • render json: value uses the default Jackson writer. render json: value, jsonConfiguration: 'deep' and respond value, jsonConfiguration: 'deep' select the same registered configuration.
  • Named configurations derive isolated writers from the configured mapper. Legacy named configurations remain supported as a fallback while their APIs are deprecated.
  • Grails domain compatibility writers use persistent-property and association rules. Their property handling differs from Jackson bean serialization; the upgrade guide documents those limits. Application serializers retain precedence.
  • The shared Boot mapper keeps normal Jackson annotations, mixins, naming strategies, inclusion, views, and derived getters for domain instances. Grails contributes GString and Spring Errors serializers globally; the latter's error representation is documented.
  • XML HTTP endpoints add grails-xml and opt into XML. XML plugin descriptors are unaffected.
  • Spring HATEOAS is not automatically bridged into Grails respond. Use an explicitly registered Grails HAL renderer or Spring MVC controller handling. This PR does not add a grails-spring-hateoas module.
  • The guide documents scalar JSON/XML handling, byte-array base64 output on the Jackson path, response encoding, and the fallback for unknown ?format= values.

Verification

The final scoped verification completed successfully: aggregate violations, checks for grails-web-common, grails-converters, grails-rest-transforms, grails-controllers, grails-xml, the three testing-support modules, grails-gsp, grails-test-suite-web, grails-test-suite-uber, and grails-fields, plus the guide build and combined Markdown test report.

The run used -PonlyCoreTests --max-workers=2 --no-daemon --continue -x aggregateGroovydoc. PMD and SpotBugs were not enabled. An earlier clean repository-wide attempt ended when the Gradle daemon disappeared; this is not a claim that the full repository suite passed.

@codecov

codecov Bot commented Aug 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.65162% with 123 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.9923%. Comparing base (4f1a5c8) to head (3421915).

Files with missing lines Patch % Lines
...converters/jackson/GrailsDomainJsonSerializer.java 78.2609% 4 Missing and 11 partials ⚠️
...ns/web/rest/render/json/DefaultJsonRenderer.groovy 75.4717% 0 Missing and 13 partials ⚠️
...gins/web/rest/render/xml/DefaultXmlRenderer.groovy 59.3750% 2 Missing and 11 partials ⚠️
...s/web/mime/GrailsContentNegotiationStrategy.groovy 73.1707% 3 Missing and 8 partials ⚠️
...lugins/testing/GrailsMockHttpServletRequest.groovy 50.0000% 5 Missing and 1 partial ⚠️
...uration/XmlConvertersConfigurationInitializer.java 90.9091% 4 Missing and 2 partials ⚠️
...onverters/json/NamedJsonConfigurationRegistry.java 86.4865% 2 Missing and 3 partials ⚠️
...rails/testing/spock/WebSetupSpecInterceptor.groovy 61.5385% 1 Missing and 4 partials ⚠️
...grails/converters/json/NamedJsonConfiguration.java 82.6087% 2 Missing and 2 partials ⚠️
...eb/converters/jackson/GrailsDomainSerializers.java 92.0000% 1 Missing and 3 partials ⚠️
... and 14 more
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #16237        +/-   ##
==================================================
+ Coverage     57.5998%   57.9923%   +0.3924%     
- Complexity      22697      22999       +302     
==================================================
  Files            2133       2151        +18     
  Lines          104226     104795       +569     
  Branches        18692      18800       +108     
==================================================
+ Hits            60034      60773       +739     
+ Misses          35906      35640       -266     
- Partials         8286       8382        +96     
Files with missing lines Coverage Δ
...erters/src/main/groovy/grails/converters/JSON.java 36.8231% <ø> (+11.5523%) ⬆️
...s/plugins/converters/ConvertersGrailsPlugin.groovy 100.0000% <100.0000%> (ø)
...g/grails/web/converters/ConvertersExtension.groovy 33.3333% <ø> (-23.8095%) ⬇️
...figuration/ConvertersConfigurationInitializer.java 82.9546% <ø> (-4.3470%) ⬇️
...converters/jackson/SpringErrorsJsonSerializer.java 100.0000% <100.0000%> (ø)
...plugins/databinding/DataBindingGrailsPlugin.groovy 100.0000% <ø> (ø)
...ils/plugins/web/mime/MimeTypesConfiguration.groovy 82.9268% <100.0000%> (+0.8755%) ⬆️
.../render/errors/ValidationProblemDetailFactory.java 100.0000% <100.0000%> (ø)
...s/web/rest/plugin/RestResponderGrailsPlugin.groovy 71.4286% <100.0000%> (+6.7227%) ⬆️
...ins/web/rest/render/DefaultRendererRegistry.groovy 73.1092% <100.0000%> (+2.4196%) ⬆️
... and 53 more

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codeconsole

Copy link
Copy Markdown
Contributor Author

Deprecation migration examples

This comment maps every API deprecated by this PR to its modern replacement. Overloads are grouped where their migration is identical.

The XML migrations intentionally change the response representation to JSON. The deprecated XML implementations remain available in grails-xml for clients that cannot migrate immediately.

1. JSON.registerObjectMarshaller(...) (all four overloads)

This covers (Class, Closure), (Class, int, Closure), (ObjectMarshaller), and (ObjectMarshaller, int).

Before:

JSON.registerObjectMarshaller(Book, 100) { Book book ->
    [id: book.id, title: book.title]
}

After, for an application-wide Jackson representation:

import org.springframework.boot.jackson.autoconfigure.JsonMapperBuilderCustomizer
import org.springframework.context.annotation.Bean
import tools.jackson.databind.module.SimpleModule

@Bean
JsonMapperBuilderCustomizer bookJsonCustomizer() {
    { builder ->
        builder.addModule(new SimpleModule()
                .addSerializer(Book, new BookValueSerializer()))
    } as JsonMapperBuilderCustomizer
}

BookValueSerializer is a Jackson 3 ValueSerializer<Book>. Jackson module registration replaces legacy marshaller priority; serializer/module ordering should be made explicit when serializers overlap.

2. JSON.withDefaultConfiguration(...)

Before:

JSON.withDefaultConfiguration {
    it.registerObjectMarshaller(Book) { Book book ->
        [id: book.id, title: book.title]
    }
}

After:

@Bean
JsonMapperBuilderCustomizer bookJsonCustomizer() {
    { builder ->
        builder.addModule(new SimpleModule()
                .addSerializer(Book, new BookValueSerializer()))
    } as JsonMapperBuilderCustomizer
}

This customizes Spring Boot's managed JsonMapper, so the representation is shared by respond, Spring MVC message conversion, and other Jackson integrations.

3. JSON.createNamedConfig(...)

Before:

JSON.createNamedConfig('deep') {
    it.registerObjectMarshaller(Type, deepObjectMarshaller)
    it.registerObjectMarshaller(Category, deepObjectMarshaller)
}

After:

import grails.converters.json.NamedJsonConfigurationRegistry

class BootStrap {
    NamedJsonConfigurationRegistry namedJsonConfigurationRegistry

    def init = { servletContext ->
        namedJsonConfigurationRegistry.register('deep') {
            it.serializer(Type, new TypeValueSerializer())
            it.serializer(Category, new CategoryValueSerializer())
        }
    }
}

The named configuration derives an isolated mapper from Boot's configured JsonMapper; it does not mutate global or thread-local converter state.

4. JSON.use(String, Closure)

Before:

String json = JSON.use('deep') {
    new JSON(book).toString()
}

After, for direct serialization:

String json = namedJsonConfigurationRegistry.writeValueAsString('deep', book)

Or stream directly:

namedJsonConfigurationRegistry.writeValue('deep', writer, book)

For controller responses, the same configuration works with both APIs:

render book, jsonConfiguration: 'deep'
respond book, jsonConfiguration: 'deep'

5. JSON.use(String)

Before:

JSON.use('deep')
try {
    String json = new JSON(book).toString()
} finally {
    JSON.use('default')
}

After:

String json = namedJsonConfigurationRegistry.writeValueAsString('deep', book)

Configuration is selected explicitly per operation, eliminating mutable thread-local state.

6. JSON.getNamedConfig(String)

Before:

def configuration = JSON.getNamedConfig('deep')

After:

ObjectWriter writer = namedJsonConfigurationRegistry.writer('deep')
String json = writer.writeValueAsString(book)

Registration remains encapsulated in NamedJsonConfigurationRegistry.register(...); consumers receive the configured, immutable-style Jackson writer used for serialization.

7. HalXmlRenderer

Before:

import grails.rest.render.hal.HalXmlRenderer

beans = {
    halBookRenderer(HalXmlRenderer, Book)
}

After, using Grails HAL JSON:

import grails.rest.render.hal.HalJsonRenderer

beans = {
    halBookRenderer(HalJsonRenderer, Book)
}

Clients negotiate it with:

Accept: application/hal+json

Applications that prefer Spring's hypermedia model can instead add the optional grails-spring-hateoas module.

8. HalXmlCollectionRenderer

Before:

import grails.rest.render.hal.HalXmlCollectionRenderer

beans = {
    halBooksRenderer(HalXmlCollectionRenderer, Book)
}

After:

import grails.rest.render.hal.HalJsonCollectionRenderer

beans = {
    halBooksRenderer(HalJsonCollectionRenderer, Book)
}

Clients use Accept: application/hal+json rather than application/hal+xml.

9. VndErrorXmlRenderer

Before:

import grails.rest.render.errors.VndErrorXmlRenderer

beans = {
    vndXmlErrorRenderer(VndErrorXmlRenderer)
}

// Client: Accept: application/vnd.error+xml
respond book.errors

After, using the default RFC 9457 validation response:

// No Vnd.Error renderer registration is required.
// Client: Accept: application/json or application/problem+json
respond book.errors

The response uses application/problem+json and status 422 for validation failures.

10. XML ValidationErrorsMarshaller

Before:

import grails.converters.XML
import org.grails.web.converters.marshaller.xml.ValidationErrorsMarshaller

XML.registerObjectMarshaller(new ValidationErrorsMarshaller(), 100)
render book.errors as XML

After:

// Client: Accept: application/json or application/problem+json
respond book.errors

Grails renders the validation errors as RFC 9457 application/problem+json. Keep the deprecated marshaller only while an existing client still requires the legacy XML error schema.

@codeconsole
codeconsole requested review from borinquenkid, jamesfredley, jdaugherty and matrei and removed request for jdaugherty and matrei September 2, 2026 03:47
@jdaugherty

Copy link
Copy Markdown
Contributor

This is an extremely large change that I think needs deferred to 8.1 or possibly 9 with feedback from multiple committers to merge.

@codeconsole

codeconsole commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor Author

@jdaugherty I think it is too much for 8.1. I am fine with 9 if we can get it reviewed in a timely manner and released as a 9.0 milestone prior to any 8.1 milestone

Conflict resolutions:

* SpringIOUtils: upstream replaced the inline setFeature block with the
  cached factories and the XmlParserFeature enum. That rewrite already
  carries the http:// feature identifiers this branch was fixing, so
  upstream's version is taken whole.

* upgrading80x.adoc: both sides appended sections. Upstream's 53-60 are
  kept in place and this branch's five sections are renumbered 61-65.

Semantic conflicts git did not flag:

* XmlDataBindingSourceCreatorSpec, added upstream in grails-web-databinding,
  moved to grails-xml, where this branch moved the creator it exercises.
  grails-web-databinding cannot depend on grails-xml.

* The XML NonPublicClassMarshallingSpec stays in grails-converters beside
  its JSON twin and the bean fixtures it shares, reached through the
  existing testImplementation project(':grails-xml').

* XmlCompatibilitySpec expected XML.parse to accept a DOCTYPE and resolve
  internal entities. Upstream's parser hardening refuses any declaration,
  internal or not, as upgrading.adoc and threat model P10 both state. The
  two affected features now assert refusal on the converter and binding
  paths.
@matrei

matrei commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Review: PR #16237 — Modernize MIME negotiation and serialization for Spring Boot 4.1

I agree on targeting Grails 9 if this merges

Head: d486fd9130908bd2f239f1e53fd69d26b08ba08f (codeconsole:feature/spring-media-type-negotiation-8.0.x)
Base: 8.0.x — merge-base 3067d0a855, branch is a clean merge on top of it (81 commits, 134 files, +5343/−472).

What I ran locally (all green):

  • ./gradlew codeStyle validateDependencyVersions
  • Module tests: grails-mimetypes (57), grails-web-common (97), grails-converters (110), grails-xml (28), grails-rest-transforms (76), grails-controllers (93), grails-web-databinding (60), grails-test-core (3), grails-testing-support-core (9), grails-databinding (33) — 0 failures.
  • check -x test (checkstyle / codenarc / pmd / spotbugs) on every changed module — no violations reported.
  • grails-test-suite-web (435), grails-test-suite-uber (576), grails-fields (682) — run with --no-build-cache and cleanTest so they actually executed; 0 failures.
  • A scratch Jackson 3.1.6 + Groovy 5.1.2 script to confirm the GString and String findings below.

Verdict: request changes. The negotiation cleanup (removal of the static state in HttpServletResponseExtension, the strategy held off the bean graph so Spring Security does not adopt it, Spring MediaType parsing) is good and I would take it on its own. The serialization half is not ready: it has two concrete output-corruption bugs on the new default path, the unit-test harness never exercises that path, and the default flips the JSON shape of every existing app's respond on a patch/minor line. I agree with @jdaugherty that this cannot land on 8.0.x; the findings below apply regardless of which line it targets.


Blocking

1. GString values become bean garbage on the new default respond path

GrailsJsonMapperCustomizer.customize (grails-converters/src/main/groovy/org/grails/web/converters/jackson/GrailsJsonMapperCustomizer.java:81-99) registers only the domain and Errors serializers. Nothing in the repo registers a GString/CharSequence serializer for Boot's mapper (grep confirms). Jackson serializes GStringImpl as a bean:

respond([message: "Saved ${book.title}"])
// production output on this branch:
{"message":{"blank":false,"bytes":"U2F2ZWQgeA==","empty":false,"strings":["Saved ",""],"valueCount":1,"values":["x"]}}

Verified with a scratch script against Jackson 3.1.6 / Groovy 5.1.2. The legacy converter handled CharSequence correctly, so this is a regression for one of the most common Grails idioms. Because the customizer is a JsonMapperBuilderCustomizer on the primary Boot mapper, a ToStringSerializer for GString belongs in that module. Neither GrailsJsonMapperCustomizerSpec nor DefaultJsonRendererSpec covers it — the renderer spec only uses Mock(HttpMessageConverter), so no test on this branch writes a real Jackson body through respond.

2. respond "text" writes the raw string, not a JSON string

DefaultJsonRenderer.renderWithSpringConverter picks the first converter whose canWrite returns true (grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/render/json/DefaultJsonRenderer.groovy:185). In Spring MVC's default list ByteArrayHttpMessageConverter and StringHttpMessageConverter precede the Jackson converter and both advertise */*, so respond 'ok' with Accept: application/json now emits ok where the legacy path emitted "ok"; respond bytes emits raw bytes. DefaultXmlRenderer.findSpringConverter (grails-xml/src/main/groovy/org/grails/plugins/web/rest/render/xml/DefaultXmlRenderer.groovy:160) has the same selection rule, so respond 'ok' as XML loses its <string> element. Select only converters whose supported media types for the target type include a concrete JSON/XML type (not */*), or route CharSequence/byte[] to the legacy path. The upgrade note at grails-doc/src/en/guide/upgrading/upgrading80x.adoc:3815 documents the current "first converter that can write" rule, so the doc needs the same correction.

3. Controller unit tests never exercise the Spring path — test and production diverge

SpringMessageConverters only receives a list when Spring MVC calls extendMessageConverters; until then it is List.of() (grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/render/SpringMessageConverters.groovy:40). The unit-test harness never initializes MVC (no WebMvcConfigurationSupport/converters anywhere under grails-testing-support-*), so DefaultJsonRenderer.canUseSpringConverter (DefaultJsonRenderer.groovy:177) is always false in a ControllerUnitTest and every test runs the legacy converter. Consequences:

  • A user's response.json assertions pass in unit tests against the legacy shape and then production serves the Jackson shape (bug 1 is invisible to every test).
  • respond book.errors renders {"errors":[{"object":..,"field":..,"rejected-value":..}]} as application/json in tests, but application/problem+json with a different body in production (the legacy shape is even asserted as the contract in JsonCompatibilitySpec:151).
  • The 22 test-suite-web specs that this PR touched all still pass on the legacy path, which is why they did not catch 1 or 2.

The web test support (WebSetupSpecInterceptor defines rendererRegistry) should give the registry a converter list built from the test context's JsonMapper, or the renderer should fall back to a JacksonJsonHttpMessageConverter when the supplier is empty.

4. Named JSON configurations cannot be used in unit tests at all

NamedJsonConfigurationRegistry.writer throws IllegalStateException when no JsonMapper bean exists (grails-converters/src/main/groovy/grails/converters/json/NamedJsonConfigurationRegistry.java:81), and the registry is created with a deferred beanProvider(JsonMapper) (grails-converters/src/main/groovy/org/grails/plugins/converters/ConvertersGrailsPlugin.groovy:79). The test harness registers no JsonMapper (grep of grails-testing-support-* and grails-test-core), so render json: x, jsonConfiguration: 'deep' and respond x, jsonConfiguration: 'deep' fail in every ControllerUnitTest. The PR's own specs avoid this: NamedJsonRenderArgumentSpec injects a hand-built renderer into a plain controller instance and RespondMethodSpec has no named-configuration case. Either testing support registers a mapper (built with GrailsJsonMapperCustomizer), or the registry falls back to one when Boot's is absent, and a spec must exercise the argument through the real harness.

5. Domain classes on the shared Boot mapper ignore Jackson's property model

GrailsDomainJsonSerializer.serialize writes persistent properties through BeanWrapper (grails-converters/src/main/groovy/org/grails/web/converters/jackson/GrailsDomainJsonSerializer.java:60-87). For every mapped domain class this bypasses @JsonIgnore, @JsonProperty, @JsonInclude, @JsonView, property-level @JsonFormat, mixins, spring.jackson.property-naming-strategy, spring.jackson.default-property-inclusion, and any transient/derived getter. Since GrailsJsonMapperCustomizer is applied to Boot's primary mapper, this also changes what a plain Spring @RestController returns for a domain instance — that worked with Jackson's normal rules on 7.x and 8.0.x today. The upgrade note claims the opposite: "Standard Jackson 3 modules, mixins, naming strategies, and mapper customizations therefore apply consistently to MVC and Grails REST responses" (upgrading80x.adoc:3819). Either build the serializer on Jackson's bean serializer (a BeanSerializerModifier that drops non-persistent properties and rewrites association properties would keep annotations working), or scope the domain serializer to Grails' own writers rather than the global mapper, and in both cases document and test what is honored.

Should fix before merge

6. The default flips every existing app's respond JSON on a minor line

useSpringJson defaults to true (grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/render/DefaultRendererRegistry.groovy:90). Beyond 1, 2 and 5, the default path changes: JSON.registerObjectMarshaller registrations are silently ignored by respond while still honored by render ... as JSON; Date moves from the legacy UTC yyyy-MM-dd'T'HH:mm:ss'Z' to Jackson's default; non-domain Groovy beans go through Jackson's bean serializer instead of the Groovy bean marshaller; self-referencing non-domain beans, which the legacy converter handled via circular.reference.behaviour, fail in Jackson; validation errors change content type, status body and shape. The flag is documented (upgrading80x.adoc:3813), but this is a behaviour change for every REST app with no code change on their side. If any of this lands on 8.0.x the default has to be false; otherwise it is a 9.0 change as already discussed on the PR.

7. DEFAULT_INCLUDED_PLUGINS now depends on an optional runtime module being on the classpath

grails-testing-support-core/src/main/groovy/org/grails/testing/GrailsApplicationBuilder.groovy:78 adds xml. IncludingPluginFilter expands dependsOn recursively, and XmlGrailsPlugin.dependsOn is [converters, dataBinding, restResponder], which in turn pull controllers, domainClass, urlMappings, i18n. So in any application that adds grails-xml, every unit test — ServiceUnitTest, plain GrailsUnitTest — now boots the web/REST/binding plugins, and the same test boots a different context depending on whether an optional module is present. The commit that added it (f54be58be9) did so to fix 22 test-suite-web specs, which is a symptom of 3/4 rather than a reason to change the global default. Load the XML plugin (or its beans) from the web test traits only, and document it.

8. beanProvider(MessageSource).getIfAvailable() fails startup with two MessageSource beans

grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/plugin/RestResponderGrailsPlugin.groovy:77. getIfAvailable() throws NoUniqueBeanDefinitionException when more than one non-primary candidate exists, which plugins that ship their own MessageSource do. Use it.bean('messageSource', MessageSource) or getIfUnique(). The JsonMapper provider in ConvertersGrailsPlugin.groovy:79 has the same exposure, deferred to the first write (a 500 instead of a startup failure).

9. render json: is only usable with a named configuration

ResponseRenderer intercepts any json argument (grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy:322) and throws if jsonConfiguration is missing (ResponseRenderer.groovy:289). render json: [ok: true] is the obvious thing to type and is now an IllegalArgumentException. Either write with the default mapper when no configuration is named, or name the argument after what it is (jsonConfiguration alone would do; the value could simply be render value, jsonConfiguration: 'deep' as respond already does).

10. Documentation claims not backed by code or tests

  • Section 68 (upgrading80x.adoc:3892-3905) says adding spring-boot-starter-hateoas and respond EntityModel.of(book) "is enough". With Accept: application/hal+json the Grails renderer registry has no HAL renderer for EntityModel, so respond does not reach the Boot HAL converter; with Accept: application/json Boot's HAL converter only accepts application/hal+json, so the plain Jackson converter writes links: [...] rather than _links. The module and tests that would have backed this were removed in 8c98b8db23; nothing on the branch tests it. Verify with a test or drop the section.
  • upgrading80x.adoc:3819 — see 5.
  • upgrading80x.adoc:3815 — see 2.
  • The PR description still lists "add an optional grails-spring-hateoas adapter module"; it was removed.

Minor

  • DefaultJsonRenderer.groovy:194 passes grails.converters.encoding to Jackson via the media type; Jackson only honours UTF-8/16/32 and otherwise emits UTF-8, so a non-UTF encoding is mis-decoded by WriterOutputStream. The comment above it claims the opposite. Either restrict to UTF encodings or decode with the charset Jackson actually used.
  • GrailsContentNegotiationStrategy.resolveMimeTypes (grails-mimetypes/src/main/groovy/org/grails/web/mime/GrailsContentNegotiationStrategy.groovy:88) returns mimeTypes[0] for an unknown ?format=, i.e. */* with the default config. Same as the old getMimeTypeForRequest fallback, but it is now also the answer for withFormat/response.mimeTypes, which previously ignored the parameter and used the Accept header. Worth a line in the upgrade notes.
  • WriterOutputStream.write(int) allocates a byte array per call; harmless because converters use bulk writes, but input.put((byte) b) + decode(false) is simpler.
  • SpringErrorsJsonSerializer (SpringErrorsJsonSerializer.java:44) makes every Errors on Boot's mapper serialize as {"errors":[...]} with no rejected values — including in @RestControllers. Fine as a default, but it is a global change and is not mentioned in the docs.

Verified as correct

  • Static negotiation state (disableForUserAgents, useAcceptHeader*, cached mimeTypes, the ShutdownOperations hook) is gone; no remaining references in the repo. The strategy is reachable only through GrailsMimeTypesWebMvcConfigurer, and SpringSecurityContentNegotiationSpec proves Spring Security does not adopt it.
  • The deleted test hunks are only the HttpServletResponseExtension.@mimeTypes = null isolation workarounds that the removed static made necessary. No assertions were removed.
  • DefaultAcceptHeaderParser parses through MediaType.parseMediaType with the lenient legacy fallback; pre-sorting by quality is stable, so the text/xml/application/xml merge and +xml reordering keep their header-order semantics. MimeType now trims the name before the ;.
  • GrailsContentNegotiationStrategy guards getParameter('format') with WebUtils.isError, so error dispatch no longer parses the request body.
  • GrailsMimeTypesWebMvcConfigurer only contributes extension aliases; Spring MVC's own negotiation, 406 behaviour and spring.mvc.contentnegotiation.* are untouched, as the upgrade note at upgrading80x.adoc:1526 says.
  • grails-xml: every moved file is content-identical apart from @Deprecated and @CompileStatic additions; XmlConvertersConfigurationInitializer reproduces the removed initXMLConfiguration/initDeepXMLConfiguration exactly; renderers are contributed as Renderer beans that DefaultRendererRegistry.setRenderers routes correctly (XmlErrorsRenderer as a container renderer keyed (Errors, Object)); module is in settings.gradle and publishedProjects (BOM), and the example apps that render XML add it.
  • application/problem+json bodies are flattened: Boot 4.1's JacksonAutoConfiguration$JsonProblemDetailsConfiguration registers ProblemDetailJacksonMixin on the primary mapper (checked in the jar), and DefaultJsonRenderer sets the content type before the first write so a committed response keeps it.
  • NamedJsonConfiguration derives one ObjectWriter per configuration lazily and thread-safely from Boot's mapper via rebuild(), so spring.jackson.* and application customizers are retained; per-response includes/excludes are layered as writer attributes and the domain serializer honours them.
  • GrailsDomainSerializers distinguishes "GORM not ready" (GrailsConfigurationException) from "not a domain class" and hands back a deferred serializer so Jackson cannot cache a bean serializer for a domain class; GrailsDomainSerializersSpec covers the ordering.
  • JsonDataBindingSourceCreator resolves the mapper lazily (avoids pulling Jackson auto-configuration ahead of GORM), reads floats as BigDecimal, and maps JacksonException to InvalidRequestBodyException.
  • GrailsMockHttpServletRequest keeps grails.converters.XML out of signatures so the class loads without grails-xml, and reports a clear error when XML conversion is requested without it.
  • REST profile templates advertise ['json'] only, consistent with the documented change.

@codeconsole

Copy link
Copy Markdown
Contributor Author

@matrei Addressed the review in e8bb2eb0a5.

  1. GString and converter selection: GStrings serialize as strings. JSON/XML renderers require converters to advertise a media type in the requested family, preventing wildcard string/byte converters from emitting raw JSON or losing the XML string element. Tests cover scalar and nested values, quoted strings, byte-array base64 output, and XML fallback.
  2. Controller test parity and named configurations: Web tests initialize Boot's Jackson configuration and populate real Spring converters from the context mapper. New controller tests exercise application customizers, named render/respond, domain values, and RFC 9457 response body/content type/status through the harness.
  3. Domain serialization scope: The domain compatibility serializer now belongs to isolated Grails writers. The shared Boot mapper retains Jackson annotations, mixins, naming, inclusion, views, formatting, and derived getters. Tests cover that separation and application serializer precedence; the guide documents the Grails writer's property-model limitations.
  4. 8.0 compatibility: This PR still targets 8.0.x, and grails.web.rendering.json.spring now defaults to false. Spring JSON rendering requires explicit opt-in.
  5. Plugin isolation and bean ambiguity: Web/XML plugins load automatically only for web tests, with service/plain-test isolation coverage. MessageSource resolution uses the application bean name; mapper resolution uses the unique/primary candidate or the conventional jacksonJsonMapper bean.
  6. Default JSON rendering: render json: [ok: true] now works without a named configuration.
  7. Documentation and minor fixes: Removed the automatic Spring HATEOAS integration claims and the stale adapter-module claim in the PR description. Documented unknown-format fallback and the global Spring Errors serializer. Jackson's intermediate output is decoded with its actual UTF-8 encoding before writing the configured response encoding, and WriterOutputStream.write(int) no longer allocates a byte array.

Validation: The final scoped run completed successfully, including aggregate violations, affected-module checks, GSP, web/uber suites, fields, the guide build, and combined test report (-PonlyCoreTests --max-workers=2 --no-daemon --continue -x aggregateGroovydoc). PMD/SpotBugs were not enabled. An earlier clean repository-wide run lost its Gradle daemon; the successful scoped run is not a full-repository pass.

@matrei

matrei commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Review round 2

Head: e8bb2eb0a5bf4227aee06cab334adb057e50a34f (codeconsole:feature/spring-media-type-negotiation-8.0.x)
Base: 8.0.x — merge-base still 3067d0a855, clean merge on top of it (82 commits, 140 files, +5894/−474).
Delta since round 1: one commit, e8bb2eb0a5 (29 files, +656/−107).

What I ran locally (all green):

  • ./gradlew codeStyle
  • Module tests with --no-build-cache and cleanTest (result XML confirmed fresh): grails-converters (114), grails-rest-transforms (84), grails-controllers (93), grails-xml (29), grails-web-common (97), grails-testing-support-core (10) — 0 failures.
  • Suites the same way: grails-test-suite-web (446), grails-test-suite-uber (576) — 0 failures.
  • check -x test (checkstyle / codenarc / pmd / spotbugs) on every changed module plus grails-gsp — no violations.
  • CI on this head is green (30805 tests; one known-flaky Geb login test, issue Test Dashboard #16030).
  • Three scratch specs, since deleted: serializer precedence with an initialised KeyValueMappingContext; a ControllerUnitTest that dumps the loaded plugin list, bean count and both render paths; and CompiledTagInvocationSpec with the new setup() removed.

Verdict: the serialization findings from round 1 are resolved. One new should-fix remains, in the test harness. Every blocking item (GString, converter selection, harness parity, named configurations in unit tests, domain serializer scope) and every should-fix (default flag, plugin loading, MessageSource ambiguity, render json:, docs) has a real code change with a test that exercises the production path. The fix for round-1 item 7, however, moved the plugin loading rather than removed it: every web unit test now boots nine plugins instead of two, and that is observable in user tests, which the PR's own GSP spec change demonstrates. Details below.


Should fix before merge

1. Web unit tests now boot the full controllers/urlMappings/i18n plugin graph, and it changes existing tests

GrailsApplicationBuilder.registerPluginDiscoveryBean (grails-testing-support-core/src/main/groovy/org/grails/testing/GrailsApplicationBuilder.groovy:208) adds converters, restResponder and xml for any GrailsWebUnitTest. IncludingPluginFilter expands dependsOn, and converters depends on controllers and domainClass, controllers on i18n and urlMappings, xml on dataBinding. Measured in a plain ControllerUnitTest on this branch:

plugins: controllers, converters, core, dataBinding, domainClass, i18n, restResponder, urlMappings, xml
bean definitions: 156

On 8.0.x the same test loads core and eventBus and the interceptor defines a curated bean set. The new graph is visible to user tests. UrlMappingsGrailsPlugin.groovy:119 registers DefaultUrlMappings when the application has none, so g.link/createLink without a controller in the request now throws instead of producing a URL. The PR hit this itself and patched the one in-repo spec: CompiledTagInvocationSpec gained webRequest.controllerName = 'book' (grails-gsp/plugin/src/test/groovy/org/grails/web/taglib/CompiledTagInvocationSpec.groovy:34). Removing that line reproduces it, four tests fail with:

grails.web.mapping.exceptions.UrlMappingException: Unable to create URL for mapping [/(*)/(*)?/(*)?(.(*))?] and parameters [{}]. Parameter [controller] is required, but was not specified!
    at org.grails.web.mapping.DefaultLinkGenerator.link(DefaultLinkGenerator.groovy:272)
    at org.grails.plugins.web.taglib.ApplicationTagLib.doCreate

Any application TagLibUnitTest or ControllerUnitTest that renders a link without a controller in the request will fail the same way after upgrading to this 8.0.x. Nothing in upgrading80x.adoc mentions it; section 65 (upgrading80x.adoc:3870) only says the XML plugin is loaded by web traits.

Suggested fix: the Spring path needs only two things at refresh time, a grailsJsonMapperCustomizer definition present before JacksonAutoConfiguration builds the mapper, and the restResponder beans (SpringMessageConverters, rendererRegistry, validationProblemDetailFactory). restResponder has no dependsOn, so including it alone drags nothing in. Register the customizer definition directly in the builder for web tests, and load XmlGrailsPlugin the way ConvertersGrailsPlugin and CodecsGrailsPlugin already are in WebSetupSpecInterceptor, via defineBeans(plugin) when the class is present. That keeps the context the harness had on 8.0.x. If the full graph is kept deliberately, it has to be documented as a behaviour change for existing unit tests, and the duplicate definitions in point 2 cleaned up.

2. Beans defined twice in the web harness

With the plugins loaded, WebSetupSpecInterceptor (grails-testing-support-web/src/main/groovy/org/grails/testing/spock/WebSetupSpecInterceptor.groovy:81-119) still defines rendererRegistry, grailsUrlMappingsHolder, grailsLinkGenerator, localeResolver and re-applies ConvertersGrailsPlugin after refresh. The later definition wins in every case, so tests pass, but the sequence is confusing: Boot's mapper is customised at refresh by the plugin's grailsJsonMapperCustomizer, then the interceptor replaces that bean with a second instance, and forGrails runs on the second one against a mapper customised by the first. It works only because customize and forGrails are independent. Whichever way point 1 goes, there should be one definition of each.

Minor

  • respond 'ok' on the legacy path, which is now the default, still throws GroovyCastException: Cannot cast object 'ok' ... to class 'grails.converters.JSON' (DefaultJsonRenderer.groovy:169 and :172, object as JSON; StringGroovyMethods.asType shadows the converters extension). This is pre-existing on 8.0.x, but this commit switched DefaultXmlRenderer to new XML(object) for exactly that reason (DefaultXmlRenderer.groovy:142), so the JSON renderer should get new JSON(object) too. Section 65 says "strings remain JSON strings" for the Spring path only; with the default off, a plain string still 500s.
  • DefaultJsonRenderer.groovy:200-204 builds a new JacksonJsonHttpMessageConverter on every response when it substitutes the Grails mapper. forGrails is cached per mapper; the converter could be cached per source converter the same way.
  • The NamedJsonConfigurationRegistry supplier (ConvertersGrailsPlugin.groovy:81-92) does a getIfUnique type scan plus a bean lookup on every writer() call. Memoise the resolved mapper after the first successful lookup.
  • isWebTest() adds the web plugins even when a spec overrides getIncludePlugins() to restrict the set. Reasonable, but worth a sentence in the testing docs.

Round-1 findings, status

# Finding Status
1 GString serialised as a bean Fixed. GrailsJsonMapperCustomizer.customize registers ToStringSerializer for GString on Boot's mapper (GrailsJsonMapperCustomizer.java:93); covered nested and root in GrailsJsonMapperCustomizerSpec, through real converters in DefaultJsonRendererSpec, and through the harness in ControllerJsonSerializationSpec.
2 respond 'text' / byte[] picked the String/ByteArray converter Fixed. Both renderers require a converter to advertise a JSON (DefaultJsonRenderer.groovy:190-193) or XML (DefaultXmlRenderer.groovy:160-163) media type for the class; tests use the real ByteArrayHttpMessageConverter, StringHttpMessageConverter and Jackson converter and assert "ok", "Saved Grails", base64 and the XML <string> element. Doc corrected (upgrading80x.adoc:3826-3832).
3 Unit tests never exercised the Spring path Fixed. JacksonAutoConfiguration is registered for web tests (GrailsApplicationBuilder.groovy:181), WebSetupSpecInterceptor populates SpringMessageConverters from the context mapper (:83-90), and ControllerJsonSerializationSpec asserts the Jackson shape, the application/problem+json 422 body, application JsonMapperBuilderCustomizers and named configurations through respond and render.
4 Named configurations unusable in unit tests Fixed, same mechanism; render json: x, jsonConfiguration: and respond x, jsonConfiguration: covered in the harness.
5 Domain serializer on the shared Boot mapper Fixed. customize no longer adds the domain module; forGrails derives a separate mapper (GrailsJsonMapperCustomizer.java:105-116) used only by the substituted converter and the named-configuration registry. See "Verified" for why the module reordering is sound. ControllerJsonSerializationSpec proves @JsonIgnore is honoured on Boot's mapper and ignored on the Grails path; the guide now states the domain-path limitations (upgrading80x.adoc:3834-3841).
6 Default flipped respond JSON on a minor line Fixed. grails.web.rendering.json.spring defaults to false (DefaultRendererRegistry.groovy:94); DefaultRendererRegistrySpec covers unset/false/true, RespondMethodSpec asserts the legacy shape by default. Doc rewritten as opt-in.
7 DEFAULT_INCLUDED_PLUGINS gained xml Reverted for non-web tests (NonWebPluginIsolationSpec, DefineBeansPluginHooksSpec assert it), but moved to web tests with a larger graph. See point 1 above.
8 getIfAvailable() with two MessageSource beans Fixed. it.bean('messageSource', MessageSource) (RestResponderGrailsPlugin.groovy:75), test with a second MessageSource bean. Mapper resolution uses getIfUnique() then the jacksonJsonMapper name (ConvertersGrailsPlugin.groovy:82-90), test with two mapper beans.
9 render json: required a configuration Fixed. Null name means the default writer (ResponseRenderer.groovy:287-292, NamedJsonConfigurationRegistry.java:74-85); covered in NamedJsonRenderArgumentSpec and the harness.
10 Doc claims Fixed. HATEOAS section replaced with an accurate "HAL Rendering" note (upgrading80x.adoc:3926, hal.adoc:128-131); the "Jackson annotations apply consistently" claim is replaced by the scoped description; PR description no longer lists the adapter module.
minor Encoding, unknown ?format=, write(int), global Errors serializer All addressed: UTF-8 intermediate bytes with the servlet writer applying the configured encoding (DefaultJsonRenderer.groovy:205-209, tested with ISO-8859-1); unknown-format fallback documented (upgrading80x.adoc:1538); write(int) no longer allocates; Errors serializer documented (upgrading80x.adoc:3843-3846).

Verified as correct

  • forGrails module reordering is sound on Jackson 3.1.6. MapperBuilder.saveStateApplyModules saves the builder state before modules run, and rebuild() restores that pristine state and re-adds the modules, so withModules → removeAllModules → domain module → addModules(original) genuinely applies the domain serializers first and the application's last. SimpleSerializers are prepended on registration, so the last module wins. My scratch spec with an initialised mapping context confirmed: an application ToStringSerializer for a domain class beats the domain serializer, another domain class still gets {"id":5,"name":"Ada"}, a snake-case naming strategy from the source mapper survives for non-domain beans, and forGrails returns the same instance per mapper.
  • The PR's own precedence spec (GrailsJsonMapperCustomizerSpec, "application domain serializers keep precedence") is discriminating even without a mapping context: DefaultGrailsApplication.getMappingContext() returns a throwing proxy, so the domain path yields a DeferredDomainSerializer that would raise IllegalStateException if it won.
  • ControllerJsonSerializationSpec runs the real Boot converter through respond; the problem body asserts status 422, content type application/problem+json;charset=UTF-8, resolved message, no rejectedValue, no properties wrapper.
  • SpringErrorsJsonSerializer and the GString serializer are the only global additions to Boot's mapper; both are now documented.
  • NamedJsonConfiguration.writer still caches one ObjectWriter per configuration; the derived writer now comes from the Grails mapper, so named configurations keep domain compatibility.
  • NonWebPluginIsolationSpec and DefineBeansPluginHooksSpec prove ServiceUnitTest/GrailsUnitTest contexts contain no rendererRegistry, xmlRenderer or namedJsonConfigurationRegistry even with grails-xml on the classpath.
  • WriterOutputStreamSpec now streams a 2000× multi-byte body with emoji through one-byte-per-write and stays intact.
  • DefaultXmlRenderer new XML(object) is equivalent to object as XML for non-String values and fixes the String case; SpringXmlRendererSpec covers the <string> element with a real StringHttpMessageConverter.
  • Everything in the round-1 "Verified as correct" list is unchanged by this commit.

@codeconsole

Copy link
Copy Markdown
Contributor Author

@matrei Addressed the valid findings from Review round 2 in ab080d2.

  • Web plugin graph: web traits automatically include only restResponder. JSON and optional XML bean definitions are contributed without discovering their plugins' transitive dependencies. The regression spec overrides getIncludePlugins() and verifies that only core and restResponder load, no default URL mappings appear, and JSON/XML support remains available. Removed the CompiledTagInvocationSpec controller-name workaround; its tests pass without it.
  • Duplicate definitions: removed the interceptor's second converters registration and renderer registry definition; URL mappings, link generator and locale resolver definitions are conditional on their absence. A regression assertion verifies that the mapper customizer used while Boot constructs the mapper is the same instance retained in the context.
  • Legacy strings: both paths now use new JSON(object). This also exposed a second underlying issue: JSONWriter rejected scalar roots. It now accepts one scalar root and rejects subsequent root values. Tests cover ordinary/named legacy string rendering, scalar values/escaping and existing enum rendering.
  • Converter allocation: cache the Grails Jackson converter per source converter, retaining the guards for custom converters and per-type mappers. Repeated-render coverage checks that distinct source mapper configurations remain distinct.
  • Mapper lookup: memoize the first successful supplier result; an absent mapper remains retryable. The regression test checks lookup counts and successful default/named output after an initial missing mapper.
  • Documentation: clarified automatic restResponder inclusion even with getIncludePlugins() overrides, optional XML bean contributions, and the absence of automatic controllers/URL mappings/i18n plugin loading.

One refinement to the suggested XML approach: contributing XML beans in the interceptor after refresh broke existing nested XML binding tests because the binding registry had already captured its source creators. Both JSON and optional XML bean definitions therefore need to be contributed before refresh, while still avoiding plugin dependency expansion. Existing XML binding tests now pass.

Validation was deliberately limited to the affected modules, with --max-workers=2 -PmaxTestParallel=1 --no-parallel; no full repository test sweep was run. The check tasks passed for grails-test-suite-web, grails-testing-support-core, grails-testing-support-web, grails-web-common, grails-converters, grails-rest-transforms, and grails-gsp. Their test reports total 1,480 tests, nine skipped, zero failures/errors (including the web suite's isolated tests). Checkstyle/CodeNarc XML reports contain zero violations. PMD/SpotBugs were requested, but this checkout does not expose codeAnalysis tasks in these modules, so those tools are not claimed as verified.

@matrei

matrei commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

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, Test Dashboard #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:

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 (Test Dashboard #16030), unrelated to this PR.
  • Everything in the round-1 and round-2 "Verified as correct" lists is unchanged by this commit.

@matrei matrei added this to the grails:9.0.0-M1 milestone Sep 18, 2026
@matrei matrei moved this to In Progress in Apache Grails Sep 18, 2026
@codeconsole

Copy link
Copy Markdown
Contributor Author

@matrei Addressed Review round 3 in d95b438c24.

XML initialization and GSP lookup

  • The XML renderer now resolves the GSP locator and renderer registry lazily when rendering. Deferring the registry is also necessary: its locator injection otherwise closes the same cycle when creation starts from mimeTypeResolver.
  • Added regression coverage that destroys the relevant singletons, recreates them starting from either xmlRenderer or mimeTypeResolver, and calls preInstantiateSingletons().
  • Added coverage for XML responses finding GSP views registered after renderer creation.
  • PersonControllerHibernateSpec now passes. Its fixture saves use failOnError: true so initialization failures surfaced as validation errors cannot silently turn into an empty result assertion.

Scalar JSON and documentation

  • PrettyPrintJSONWriter now accepts a scalar root and transitions to DONE.
  • The scalar/second-root rejection table runs against both writers. Controller coverage exercises pretty-printed strings, GStrings, enums, numbers, booleans, and direct conversion of null.
  • Rewrote upgrade-guide section 25 to describe enum-name strings and scalar roots. It also clarifies using new JSON(value) for strings and numbers, whose built-in Groovy casts do not support as JSON.

Minor items

  • Guarded optional converters-plugin loading with ClassUtils.isPresent.
  • Removed the registry monitor around mapper supplier calls, retaining the volatile cache and retry after absence. Added a concurrent-first-write regression test.
  • Moved PROBLEM_JSON above instance fields.

Validation

  • Clean aggregate style checks, core test aggregation, web checks, and guide build passed:
    clean aggregateViolations :grails-test-report:check :grails-test-suite-web:check :grails-doc:publishGuide -x aggregateGroovydoc --continue -PonlyCoreTests --max-workers=2 -PmaxTestParallel=1 --no-daemon.
  • Combined report: 8,377 tests, 0 failures/errors, 33 skipped.
  • Full :grails-test-examples-hibernate7-demo33:test: 79 tests, 0 failures/errors, 3 skipped, including the reported controller regression.
  • Checkstyle and CodeNarc reports are clean. PMD and SpotBugs were not enabled. The core-only sweep excludes the remaining database and functional suites.

The PR remains targeted at Grails 8.

…ia-type-negotiation-8.0.x

# Conflicts:
#	grails-doc/src/en/guide/upgrading/upgrading80x.adoc
…ia-type-negotiation-8.0.x

# Conflicts:
#	grails-doc/src/en/guide/upgrading/upgrading80x.adoc
@testlens-app

testlens-app Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 3421915
▶️ Tests: 96735 executed
⚪️ Checks: 90/90 completed


Learn more about TestLens at testlens.app/docs.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants