matrei commented on PR #16237:
URL: https://github.com/apache/grails-core/pull/16237#issuecomment-5679196827
# 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:
```groovy
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 cu
rrent "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 dr
ops 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 `@RestController`s. 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.
--
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]
