matrei commented on PR #16237:
URL: https://github.com/apache/grails-core/pull/16237#issuecomment-5696991670
# 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 #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 `JsonMapperBuilderCustomizer`s
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.
--
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]
