matrei commented on PR #16237:
URL: https://github.com/apache/grails-core/pull/16237#issuecomment-6209569838
## Review round 6
**Head:** `4608138a008f7efbafb8c41c967061048abe5bca`
(`codeconsole:feature/spring-media-type-negotiation-8.0.x`)
**Base:** `9.0.x`, merge-base `c948a638d9`. The branch is level with `9.0.x`.
**Delta since round 5 (`6fc4149bbb`):** the PR now targets `9.0.x`. There
are 30 PR commits, from `b98fe9612d` to `cea07d4272`, plus merges of `9.0.x`.
Together they cover jdaugherty's three reviews: Grails 9 keeps the legacy
defaults behind `grails.web.rendering.json.spring`,
`grails.web.rendering.xml.spring` and `grails.databinding.json.jackson`,
`grails-xml` is back in the web starter, and the upgrade notes moved to
`upgrading90x.adoc` as sections 2–10. I haven't re-raised anything from his
threads. Where his fixes touch the findings below, I checked them.
**What I ran locally:**
- `git show --remerge-diff` on every merge since round 5. Three of them
change files:
- `d7aa797479`: upgrade-guide add/add conflict, with GPars kept as section
1.
- `a94f9317dd`: restores `upgrading80x.adoc` to the `9.0.x` text now that
the notes live in the 9.0 guide.
- `e23a267116`: takes `9.0.x`'s `metadataJars` in
`grails-doc/build.gradle`, and switches `TableSpec` from `XML.parse` to
`XmlSlurper`.
All three are correct. `generateConfigReference` still lists the three new
settings.
- Module tests, all green:
| Module / suite | Tests |
|---|---|
| `grails-mimetypes` | 66 |
| `grails-web-common` | 127 |
| `grails-web-databinding` | 77 |
| `grails-controllers` | 224 |
| `grails-test-core` | 3 |
| `grails-testing-support-core` | 50 |
| `grails-converters` | 211 |
| `grails-rest-transforms` | 101 |
| `grails-xml` (`--no-build-cache`, `cleanTest`) | 77 |
| `grails-test-suite-web`, negotiation, JSON and XML specs
(`grails.rest.web.*`, `WithFormatSpec`, `ContentNegotiationSpec`,
`org.grails.web.mime.*`, `JSONConverterTests`, `JSONWriterSpec`) | 102 |
- `codeStyle` on the 12 touched production modules: 0 violations.
`projectHealth` on `grails-xml`, `grails-converters` and `grails-test-core`: no
duplicate classes.
- `:grails-doc:publishGuide`: builds with no xref warnings. The rendered
HTML is the source for the documentation findings.
- A differential spec that runs 34 `Accept` headers through the PR's
`DefaultAcceptHeaderParser` and through a copy of the `9.0.x` parser, against
three MIME configurations, as both `Accept` and `Content-Type`.
- Scratch specs reproducing each finding below. All of them have been
deleted since.
**Verdict: changes requested.** The default path is close to Grails 8, but
not quite there. Two findings change behaviour for an application that sets
nothing (5 and 6). One makes the opt-in write properties a renderer bean
excludes (1). CI is green, but none of these cases is covered by a test.
---
## Blocking
### 1. With the opt-in on, a renderer bean's own `includes`/`excludes` are
ignored
`JsonRenderer` applies its bean-level `includes`/`excludes` only in its
override of the legacy `renderJson(JSON, RenderContext)`
(`JsonRenderer.groovy:128`). Since `b355586f58`,
`DefaultRendererRegistry.addRenderer` (`:166`) gives renderer beans the
registry's `useSpringJson`. The Spring path in `DefaultJsonRenderer` reads only
`context.includes`/`context.excludes`, both for `JsonProjection.of` and for the
decision to stay on the legacy converter (`:275-281`). The bean's lists never
reach it.
`DefaultXmlRenderer.findSpringConverter` (`grails-xml`, `:171`) has the same
gap for `XmlRenderer` (`XmlRenderer.groovy:117`) with
`grails.web.rendering.xml.spring: true` and a non-domain type.
The test was a `ControllerUnitTest` with a nested `@Configuration` that
declares `new JsonRenderer<Account>(Account).tap { excludes = ['password'] }`.
The action calls `respond new Account(user: 'ada', password: 's3cret')`:
| | Body |
|---|---|
| `grails.web.rendering.json.spring` unset | `{"user":"ada"}` |
| `grails.web.rendering.json.spring: true` |
`{"password":"s3cret","user":"ada"}` |
XML shows the same with `XmlRenderer<Credentials>` and `excludes =
['password']`:
| | Body |
|---|---|
| `xml.spring` off |
`<scratchCredentials><user>a</user></scratchCredentials>` |
| `xml.spring: true` |
`<ScratchCredentials><password>secret</password><user>a</user></ScratchCredentials>`
|
`defaultRenderers.adoc` documents this bean pattern for hiding properties,
and section 3 says a `JsonRenderer` or `XmlRenderer` bean "follow[s] the same
settings" (`upgrading90x.adoc:121`). So an application that opts in, or every
application once Grails 10 flips the default, writes the properties it asked to
hide.
Suggested fix: add a protected hook on
`DefaultJsonRenderer`/`DefaultXmlRenderer` for the effective includes/excludes,
overridden in `JsonRenderer`/`XmlRenderer` the way `renderJson(JSON,
…)`/`renderXml(XML, …)` already are. Use it both for the projection and for the
stay-on-legacy check. Please add the excludes case to
`ControllerJsonSerializationSpec` and `SpringXmlRendererSpec`.
## Should fix
### 2. Mixed-case configured MIME types no longer match
Spring's `MediaType` lowercases type and subtype, so
`parseRequestedMimeTypes` (`DefaultAcceptHeaderParser.groovy:113-114`) now
produces lowercase names. `createMimeTypeAndAddToList` (`:131-135`) still
compares them with `mt.name == name` against the configured names, which keep
their case. The same parser resolves `Content-Type` for `request.format`, so
both directions are affected.
Test setup: `grails.mime.types` with `acme: 'application/vnd.Acme+json'`, a
request with `Accept` and `Content-Type` both set to
`application/vnd.Acme+json`, and `withFormat { json {…}; acme {…} }`:
| | Result |
|---|---|
| `9.0.x` | `acme`, for both the header and the body |
| PR | `response.format == 'json'`, `request.format == null`, the `json`
block runs, and `response.mimeTypes` is the full default list |
Registered types such as `application/vnd.ms-excel.sheet.macroEnabled.12`
are affected the same way. Section 9 (`upgrading90x.adoc:351`) and
`contentNegotiation.adoc:207` say names "are now matched case-insensitively",
but for these types an exact match that used to work now fails.
Suggested fix: use `mt.name.equalsIgnoreCase(name)` in both `find`s, which
also makes the documented claim true. Please add a mixed-case configured type
to `AcceptHeaderParserSpec` and `RequestAndResponseMimeTypesApiSpec`.
Related, for the same section: `Content-Type` matching is case-insensitive
now too. `Content-Type: Application/JSON` gives `request.format == 'json'` (it
was `null`), so the JSON binding source now binds a body that `9.0.x` ignored.
That's an improvement, but section 9 only mentions `Accept`.
### 3. The Jackson domain serializer loads lazy to-one proxies to write
their id
`writeAssociationReference` (`GrailsDomainJsonSerializer.java:216-218`)
reads `getProxyIdentifier(value)` and then always calls
`proxyHandler.unwrapIfProxy(value)`. Both Hibernate proxy handlers initialize
the proxy there. The legacy `DomainClassMarshaller.asShortObject` never unwraps.
With the opt-in on, `respond Book.list()` therefore runs one extra `SELECT`
per lazy `author`. A detached instance throws `LazyInitializationException`
where Grails 8 wrote `{"id": …}`.
I tested this with a KeyValue mapping context and an `EntityProxyHandler`
that counts unwraps:
| Path | Result |
|---|---|
| Jackson | `{"id":1,"title":"T","author":{"id":7}}`, 1 unwrap |
| Jackson, handler whose unwrap throws | `IllegalStateException: could not
initialize proxy - no Session` |
| Legacy marshaller, same throwing handler |
`{"id":1,"title":"T","author":{"id":7}}`, 0 unwraps |
Suggested fix: only unwrap when `identifier == null`, and add a spec that
asserts no unwrap happens.
### 4. An application `ValidationProblemDetailFactory` bean is ignored
unless it replaces the plugin's bean by name
`RestResponderGrailsPlugin` registers `validationProblemDetailFactory`
(`:70-77`) unconditionally. `DefaultRendererRegistry` autowires a single
`ValidationProblemDetailFactory` with `required = false` (`:93`). With two
beans of the type, Spring falls back to the field name, so the plugin's bean
wins.
I tested a nested `@Configuration` with `@Bean includingRejectedValues() {
new ValidationProblemDetailFactory(true) }` and the opt-in on. Both beans are
present, and `respond errors` has no `rejectedValue` in its `errors` entries.
Section 7 (`upgrading90x.adoc:332-333`) tells applications to "define their
own `new ValidationProblemDetailFactory(true)` bean". That works only under the
name `validationProblemDetailFactory`. Even then, the example has no
`MessageSource`, so messages come back as unresolved `{0}` templates instead of
the localized text the plugin's bean produces.
Suggested fix: either register the plugin's bean only when the application
has none, or have the guide say "override the `validationProblemDetailFactory`
bean" and show `new ValidationProblemDetailFactory(true, messageSource)`.
### 5. XML validation errors render an action-named `.xml.gsp` with status
200 (default path)
On `9.0.x`, the XML errors renderer was `new DefaultXmlRenderer(Errors)`,
created inside the registry. It wasn't a bean, so it had no locator and always
wrote the errors with `422`. Now `XmlErrorsRenderer` is a bean, so its
`groovyPageLocatorProvider` is injected. `DefaultXmlRenderer.render`
(`:124-135`) has no `!(object instanceof Errors)` guard; `DefaultJsonRenderer`
has always had one.
I tested a failed `save` with `Accept: application/xml`:
| Setup | Result |
|---|---|
| No `save.xml.gsp` | `422` with the `<errors>` body |
| `save.xml.gsp` present | `200` and the GSP |
| JSON with `save.json.gsp` | `422` with the errors body, as before |
In round 4 I listed "errors responses find GSP views the same way" as
verified. I was comparing against the round-3 branch rather than the base, and
against the base it's a behaviour change.
Suggested fix: add `&& !(object instanceof Errors)` to the view check, as in
the JSON renderer, and a row in `ControllerXmlRenderingSpec`.
### 6. `render` of a `JSONObject` that has a `json` key no longer renders
the object (default path)
`render(Map)` now checks `argMap.containsKey('json')`
(`ResponseRenderer.groovy:321`) before the existing `argMap instanceof
JSONElement` branch. `JSONObject` is a `Map`, so `render request.JSON` or
`render JSON.parse(upstream)` with a top-level `"json"` key now behaves
differently:
| Converters | Result |
|---|---|
| `9.0.x` | the whole object is written |
| Present | only the `json` value is written |
| Absent | `IllegalStateException: No JSON renderer is available.` |
Suggested fix: skip the named-JSON branch when `argMap instanceof
JSONElement`.
### 7. New startup cycle through `grailsMimeTypesWebMvcConfigurer`
`grailsMimeTypesWebMvcConfigurer` is an instance `@Bean` method on
`MimeTypesConfiguration` (`:128`).
`DelegatingWebMvcConfiguration.setConfigurers` therefore now creates
`MimeTypesConfiguration`. Its constructor takes the `List<MimeTypeProvider>`,
which includes every `Renderer`. So a renderer bean that depends on any Spring
MVC infrastructure bean (`LocaleResolver`, `mvcConversionService`, …) closes a
loop. Boot's default `allowCircularReferences=false` turns that loop into a
startup failure.
Repro: `@EnableWebMvc`, plus `MimeTypesConfiguration`, plus a
`MimeTypeProvider` bean that takes a `LocaleResolver`. It fails with
`BeanCurrentlyInCreationException: 'mimeTypesConfiguration'`. Removing only the
configurer bean definition lets the context start.
No renderer in this repo does this, so the chance is low, but there is no
opt-out. This is the same `MimeTypesConfiguration → MimeTypeProvider` path as
the round-4 follow-up, now with a new link added by this PR.
Suggested fix: make the bean method `static`, and resolve `MimeTypesHolder`
and the config through `ObjectProvider`s in
`configureContentNegotiation`/`getContentNegotiationStrategy`, memoised.
### 8. `vndError.adoc` describes the Grails 10 default as the current one
`vndError.adoc:20-55` says that "by default" `respond` sends validation
errors as an RFC 9457 problem, and that an application which "keeps the legacy
JSON converter" gets `{"errors": […]}`. For Grails 9 it's the other way round:
with `grails.web.rendering.json.spring` unset, `respond` writes the legacy
body, as section 7 says. Please show the legacy body as the default, and the
problem as what the opt-in gives (and the default from Grails 10). The line
"New applications should use the default RFC 9457 … response" needs the same
adjustment.
### 9. `openApi.adoc`: the new NOTE swallows the chapter's first paragraph
There's no blank line between the `NOTE:` (`openApi.adoc:20-22`) and "Grails
applications can publish…", so the built guide renders the chapter introduction
inside the note. Please add the blank line, and link the note to "XML Web
Support Moves to a Separate Module", as asked in round 5.
## Minor (none blocking)
- **`respond problem, jsonConfiguration: 'x'`** answers `200
application/json`. The named-configuration branch
(`DefaultJsonRenderer.groovy:170-177`) returns before the `ProblemDetail`
status, content type and `instance` handling at `:178`. Handle `ProblemDetail`
first.
- **Misleading fallback warning.** With `grails.web.rendering.json.spring:
true`, a response that still falls back to the legacy converter logs "…set
grails.web.rendering.json.spring=true…" (`:207-211`), which tells users to set
what they already set. This happens for a renderer with a `namedConfiguration`,
a projection on an application converter, or a type no converter can write.
When the setting is on, the message should give the actual reason.
`missingConvertersReported` (`:106`) is per renderer instance, unlike
`legacyFallbackReported`, so that warning repeats once per renderer.
- **Section 3 lists only pretty-print** as a legacy setting Jackson ignores
(`upgrading90x.adoc:119`). With the opt-in,
`grails.converters.json.default.deep` and `grails.converters.json.date` are
ignored too, and associations are always `{id}`. jdaugherty's thread on
`ConvertersConfigurationInitializer` raised both, but the reply covered
pretty-print only.
- **"A `ProblemDetail` is always written through Spring's message
converters"** (section 7) doesn't hold without an MVC adapter. With no
converters, `render(ProblemDetail.forStatus(422))` writes a legacy bean dump
(`{"detail":null,"instance":null,"properties":{…},…}`) and logs the "Legacy
respond() JSON rendering" warning. Please either qualify the sentence or log a
specific warning.
- **Excludes miss a narrowed proxy inside a collection.**
`GrailsDomainJsonSerializer.java:73-75` re-dispatches to the unwrapped
subclass, and `JsonProjection.appliesTo` matches exact classes. In `[plainBook,
proxyOfSpecialBook]` with `excludes: ['title']`, the proxy's `title` is
written. `respond` only unwraps the root, so this shows up for collection
elements and `render json:`.
- **Without `grails-xml`, XML requests fail quietly.**
- `respond errors` for XML answers `404` with no body:
`RestResponder.groovy:196` answers 404 when no errors renderer is found. That
branch predates the PR but is reachable now. A plain XML `respond` answers
`406`, which is right.
- An XML request body binds nothing and raises no error.
- `request.XML` and `encodeAsXML` fail with a bare
`MissingPropertyException`/`MissingMethodException`.
Either answer `406` for errors too, or list these outcomes in section 2
where it says JSON-only applications can drop the module. A `respond`-level
test without `grails-xml` would pin it down.
- **`XmlGrailsPlugin` depends on `dataBinding`** (`:49`), but
`grails-databinding`, which holds `DataBindingGrailsPlugin`, isn't on
`grails-xml`'s runtime classpath. An application without the web starter that
adds only `grails-xml`, as section 2 instructs, gets "cannot be loaded …
dependency [dataBinding] … is missing" and no XML support. Either add the
runtime dependency or use `loadAfter`.
- **`XML.java` logs "Legacy XML conversion is deprecated for removal in
Grails 11"** (`:133-136`), but `grails.converters.XML`, `XmlRenderer` and
`DefaultXmlRenderer` carry no `@Deprecated`. Only the HAL XML, Vnd.Error XML
and XML `ValidationErrorsMarshaller` classes do. Please either annotate the
others or soften the message.
- **`GrailsContentNegotiationStrategy` implements Spring's
`ContentNegotiationStrategy`**, but production only calls
`resolveAcceptedMimeTypes`. `resolveMediaTypes`/`resolveMimeTypes` are reached
only from `GrailsContentNegotiationStrategySpec`, which goes against rule 9.
- One of those features, "error dispatch resolves its format without
reading rejected request parameters" (`:90`), asserts something the real
`response.format` doesn't do.
`HttpServletResponseExtension.getMimeTypeForRequest` still reads
`params.format` on error dispatches, unchanged from `9.0.x`.
- Suggested fix: drop the interface and those two methods, and test
through `response.format`/`withFormat`. That also removes the risk the
configurer comment describes, should someone later register the class as a bean.
- **Smaller items:**
- `GrailsContentNegotiationStrategy.groovy:120` matches `X-Requested-With`
with `equalsIgnoreCase`, while `request.xhr` uses an exact match.
- The no-configurer fallback in `HttpServletResponseExtension`
(`:199-203`) builds a strategy, including the user-agent regex, on every
request.
- `Accept: *` now gives `response.mimeTypes == [*/*]` instead of the
default list. `response.format` and `withFormat` are unaffected.
- **Nits:**
- `selectedConfiguration ?: namedConfiguration`
(`DefaultJsonRenderer.groovy:213`) is dead; `selectedConfiguration` is always
empty there.
- The `MessageSource` import in `RestResponderGrailsPlugin` (`:36`) sits
among the `grails.*` imports.
- `RespondJsonConverterSelectionSpec`'s feature names still describe the
removed marshaller heuristic, and `RespondJsonMarshallerRegistererSpec` uses
the deprecated `doWithSpring`.
- `hal.adoc:133` and `atom.adoc:46` read as if `grails-xml` were missing,
though the 9 starter includes it.
- Stray blank lines are left in `ContentNegotiationSpec` and
`ContentFormatControllerTests` (`:36-38`) where the cache resets were removed.
## Round-5 and round-4 findings, status
| Finding | Status |
|---|---|
| R5 #1 `RestfulControllerRedirectTargetSpec` resets the removed cache |
Fixed in `4a583a3873` |
| R5 #2 `openapi-rest-api` example needs `grails-xml` | Fixed by
`b520a96840`, which puts `grails-xml` back in the web starter |
| R5 #2 note in `openApi.adoc` | Partly done: added, but without the link
and with the rendering problem in 9 |
| R5 #3 `grails-openapi` doesn't know about the Spring JSON opt-in | Open.
No mention in section 7 or `openApi.adoc`, and no follow-up issue.
`openApi.adoc:597` ("as the converters render it") also stops holding for beans
with `xml.spring: true` |
| R5 #4 `doWithSpring` wording | Fixed. Section 3 and `unitTesting.adoc`
point to a nested `@Configuration` |
| R4 `DefaultXmlRenderer` resolves its `ObjectProvider`s on every response |
Open (`:125`, `:129`). With `xml.spring`, `findSpringConverter` also resolves
the converters twice (`:171`, `:179`) |
| R4 `groovyPageLocator`/`rendererRegistry` no longer autowired | Open, and
not mentioned in section 2 |
| R4 `xmlRenderer` supplier sets the locator provider by hand | Open
(`XmlGrailsPlugin.groovy:75`) |
| R4 follow-up issue for the field-injected locator | Open. Finding 7 makes
the issue more relevant |
## Verified as correct
- **Negotiation on the default path matches `9.0.x`** except for finding 2
and the `Accept: *` note.
- Covered: `withFormat`, `response.format`, `response.mimeTypes`,
`request.format`, the `format` parameter and `RESPONSE_FORMAT`.
- The differential spec agrees on quality ordering, `v=`, the
`text/xml`/`application/xml` dedup, `+xml` priority, `charset`, invalid and
quoted `q`, empty tokens and multipart boundaries.
- `use.accept.header`, `disable.accept.header.userAgents` (as a pattern or
a list) and `userAgentsXhr` keep their semantics.
- `173e94f9b8` restores the Accept-header fallback for
`response.mimeTypes`.
- **The static MIME cache is gone and its replacement is immutable.** The
parser is created per call and never mutates the configured `MimeType`s.
- **Spring MVC negotiation is unaffected.** Only data-format aliases are
added, and Boot's or the application's `media-types` win. `produces` and 406 on
`@RestController` and actuator endpoints behave as before. The strategy isn't a
bean, so Spring Security doesn't adopt it.
- **Default `respond` JSON matches the legacy output, errors included.**
`new JSON(object)` is equivalent to the old `object as JSON` cast, and
`JSON.use(namedConfiguration)`, includes/excludes, the 422 status and the
content type are unchanged. The only intended difference is scalar roots
(section 5).
- **`WriterOutputStream` is correct.**
- Content: 2-, 3- and 4-byte characters, surrogate pairs, NUL and escapes.
- Chunking: 50 random chunkings from 1 to 9000 bytes, mixing `write(int)`,
`write(byte[])` and `flush()`.
- Encodings: UTF-8, UTF-16, UTF-16LE and GB18030 all round-trip.
- Edge cases: a truncated trailing sequence becomes U+FFFD on close, and
the writer is left open. An ISO-8859-1 response encoding gives the same result
as the legacy converter.
- **Domain JSON on the Jackson path matches `DomainClassMarshaller`**, apart
from findings 3 and 5:
- `id` and `version` are written only when set, and `version` only with
`include.version`.
- `class` comes from the entity name.
- Properties follow metadata order.
- Null associations are written as `null` and `Basic` collections as
values.
- The projection applies to `id`, `version` and `class`.
- **jdaugherty's implementation fixes hold.** Checked: the narrowed root
proxy, embedded-cycle `_ref`, single-pass `Iterable`, `text/json`,
`HttpStatus.resolve` for problem statuses, and `SpringMessageConverters` being
read when a response is written.
- **Shared state is scoped correctly.**
- `forGrails` caches per customizer bean, so per context, with nothing
shared across a devtools restart.
- `NamedJsonConfigurationRegistry` freezes configurations on registration
and caches the writer and mapper safely.
- The legacy warning fires once per registry.
- **`ProblemDetail` responses with the opt-in** set status and content type
before the body is written. A failed write restores the negotiated type, and
`instance` comes from the request URI.
- **The `grails-xml` move is clean.**
- No class exists in two modules, and the extension modules have distinct
names.
- `XmlConvertersConfigurationInitializer` is a faithful port: the
marshaller order, deep, encoding, circular-reference and pretty-print settings,
and the `ObjectMarshallerRegisterer` beans are all carried over.
- The XML binding creator beans keep their names.
- `findSpringConverter` isn't reached when `xml.spring` is off.
- **Packaging is complete.**
- `settings.gradle` and the publish list include `grails-xml`.
- `grails-base-bom` manages it, and the starter-web POM includes it.
- `generateConfigReference` lists the three new settings.
- The `ConfigurationMetadataPlugin` change has a regression spec.
- **Test-harness wiring matches section 10 and `unitTesting.adoc`.**
`GrailsMockHttpServletRequest` without `grails-xml` fails with a clear message
naming the module.
- **Upgrade guide sections 2–10** use the setting names and defaults the
code has, name Grails 10 for the flip and Grails 11 for removal, and mention no
internal classes. The PR description matches the code.
- **Source hygiene:** new files have license headers, and there are no
wildcard or `javax` imports. The only `@author` in an added file is on
`DefaultXmlRenderer`, a moved file that keeps its original tag, which is fine.
--
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]
