Modernize MIME negotiation and serialization for Spring Boot 4.1 - #16237
codeconsole wants to merge 86 commits into
Conversation
Deprecation migration examplesThis comment maps every API deprecated by this PR to its modern replacement. Overloads are grouped where their migration is identical.
1.
|
|
This is an extremely large change that I think needs deferred to 8.1 or possibly 9 with feedback from multiple committers to merge. |
|
@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.
…ia-type-negotiation-8.0.x
Review: PR #16237 — Modernize MIME negotiation and serialization for Spring Boot 4.1I agree on targeting Grails 9 if this merges Head: What I ran locally (all green):
Verdict: request changes. The negotiation cleanup (removal of the static state in Blocking1. GString values become bean garbage on the new default
|
|
@matrei Addressed the review in e8bb2eb0a5.
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 ( |
Review round 2Head: What I ran locally (all green):
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, Should fix before merge1. Web unit tests now boot the full controllers/urlMappings/i18n plugin graph, and it changes existing tests
On Any application Suggested fix: the Spring path needs only two things at refresh time, a 2. Beans defined twice in the web harnessWith the plugins loaded, Minor
Round-1 findings, status
Verified as correct
|
|
@matrei Addressed the valid findings from Review round 2 in ab080d2.
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 |
Review round 3Head: What I ran locally:
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 ( Blocking1.
|
| # | 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 onlyactionfails against the curated/(*)/(*)?/(*)?mapping, not againstDefaultUrlMappings; the urlMappings plugin is not loaded. encodeAsXML()needingmockCodec(XMLCodec)in unit tests is unchanged from8.0.x; registering only the XML bean registrar (notprovidedArtefacts) does not regress it.- The
grailsConverterscache key is the source converter instance (HttpMessageConverterhas identity equality), andforGrailsis itself cached per mapper, so two converters sharing a mapper still share the derived mapper. JSONWriterbase class: a second root value after a scalar throws and leaves the output untouched;object()/array()afterDONEstill throw.JSONWriterSpeccovers string escaping,GString, numbers, booleans andnull.- The scaffolding
UserControllerSpec > User listfailure 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 Addressed Review round 3 in d95b438c24. XML initialization and GSP lookup
Scalar JSON and documentation
Minor items
Validation
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
✅ All tests passed ✅🏷️ Commit: 3421915 Learn more about TestLens at testlens.app/docs. |
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.MediaType, remove static negotiation state, and resolve strategy bean ambiguity.*/*.NamedJsonConfigurationRegistryshared by controller rendering and direct serialization.grails-xml, keep plugin descriptors unchanged, and make new REST artefacts JSON-only by default.Compatibility and migration
grails.web.rendering.json.springdefaults tofalseon8.0.x. Set it totrueto opt into Spring JSON rendering. Legacy marshallers and ordinary legacy response shapes remain the default.render json: valueuses the default Jackson writer.render json: value, jsonConfiguration: 'deep'andrespond value, jsonConfiguration: 'deep'select the same registered configuration.Errorsserializers globally; the latter's error representation is documented.grails-xmland opt into XML. XML plugin descriptors are unaffected.respond. Use an explicitly registered Grails HAL renderer or Spring MVC controller handling. This PR does not add agrails-spring-hateoasmodule.?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, andgrails-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.