Repository navigation
Modernize MIME negotiation and serialization for Spring Boot 4.1 - #16237
jdaugherty merged 663 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.
|
82fe9d8 to
c86bcff
Compare
Why
|
|
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 |
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. |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
This review is about the migration path, not the implementation. The three rounds with @matrei cover the implementation and I have not re-reviewed them. Head reviewed: 520998f.
As it stands, an upgrading application gets four behaviour changes in the same release that introduces their replacements: respond() switches to Jackson by default, XML leaves the default classpath, JSON request binding switches parser, and validation errors change media type and body. Each has a legacy path, but none has a release in which the old behaviour is still the default and warns. That sequence is what I am -1 on, not the direction.
What I am asking for, with the specifics on the lines below:
- Grails 9: keep the legacy behaviour as the default, log one deprecation warning when it is used, and let applications opt in (
grails.web.rendering.json.spring: true, removinggrails-xmlby hand). - Grails 10: flip the defaults.
- The release after that: remove the legacy paths, and name that release now in every
forRemovalannotation and in the guide.
The current head is red in CI for exactly this reason: RestApiDocumentFunctionalSpec in grails-test-examples/openapi-rest-api, merged to 9.0.x after this branch, expects XML answers and fails because that example never added grails-xml (Build Grails shard 1). That is the upgrade every existing XML application will hit.
Blocking: the respond() default and XML leaving the default classpath. Should fix: the binding parser swap, the global Errors serializer, the unnamed removal release, and the overlap with #16414.
Implementation review
The inline threads above the policy ones carry the verified implementation findings, one blocking (the malformed Accept crash in DefaultAcceptHeaderParser) and the rest should-fix. Minor items that do not need a thread of their own:
SpringMediaTypeAdapter.toMediaTypethrowsIllegalArgumentExceptionforq=2.0andNumberFormatExceptionforq=abc, both of whichAcceptHeaderParserSpecsays the parser accepts. Not reachable through Spring's manager today, but it is a publicContentNegotiationStrategy; clamp or drop the parameter.GrailsMimeTypesWebMvcConfigurerparses everygrails.mime.typesname withMediaType.parseMediaType, so a configured name Spring rejects now fails startup insidemvcContentNegotiationManager; catch and skip.- Section 8 documents only the unknown
?format=case.response.mimeTypesnow also reads theRESPONSE_FORMATattribute thatgetFormatandwithFormatwrite, so after a matchedwithFormat { html {} }it returns[html]instead of the Accept list, and a known-but-unhandled format (?format=xmlwith onlyhtmlandjsonblocks) no longer falls back to the Accept header. Worth a sentence. render json: book, encoding: 'ISO-8859-1'ends upcharset=UTF-8:applyContentTypereturns false when onlyencoding:is given and the fallback hard-codes UTF-8, whilerespondhonoursgrails.converters.encodingandrender text:honoursencoding:.- The derived Grails converter is
new JacksonJsonHttpMessageConverter(forGrails(source.mapper)), which drops the source'ssupportedMediaTypes,defaultCharsetand JSON prefix; an application that setsetPrefixJson(true)gets the prefix on@RestControllerbodies and not onrespond. - An unknown
jsonConfigurationonrespondfails with the legacyConverter Configuration with name 'x' not found!, whilerender json:throwsNamed JSON configuration [x] is not registered.. - Association references write the identity property's name instead of the hard-coded
"id"the legacy marshaller wrote ({"tag":{"tagId":3}}forid name: 'tagId'). Fine, but it is a wire change section 2 should list. NamedJsonConfigurationRegistry.contains(null)throws fromConcurrentHashMap.containsKeywhilenullis the documented default name, andNamedJsonConfigurationkeeps public mutators afterregisteralthoughwriter()caches the first derived writer, so laterserializer()calls are silently ignored.JsonDataBindingSourceCreatorresolves the mapper withgetIfAvailable(),ConvertersGrailsPluginwithgetIfUnique()plus thejacksonJsonMapperfallback; the first surfacesNoUniqueBeanDefinitionExceptionas aDataBindingSourceCreationExceptionon the first request when two non-primary mappers exist. Align them.- grails-xml declares
api project(':grails-databinding')(the plugin module), which nothing in its sources references; what it compiles against isgrails-web-databinding, reached only transitively.grails-convertersandgrails-rest-transformsaddtestImplementation project(':grails-xml')to keep XML-only specs (XMLParseSpec,NonPublicClassMarshallingSpec,DomainClassMarshallerSpec, the XML cases ofVndErrorRenderingSpec) where they are; they belong in grails-xml with the code they test. - Tests through internal APIs:
ContentNegotiationSpec:63andContentFormatControllerTests:41callnew XmlConvertersConfigurationInitializer(...).initialize()with a comment saying the slice does not load the plugin, butGrailsApplicationBuildernow registers the XML plugin's beans for every web test and the initializer is anInitializingBean, so both calls are redundant.GrailsDomainSerializersSpecconstructs the package-privateGrailsDomainSerializersand callsfindSerializerwith a null config, duplicating whatGrailsJsonMapperCustomizerSpeccovers throughforGrails. JsonBindingSpec:136-137: the comment says the assertion does not pin the parser; the next line assertscontains 'tools.jackson'.GrailsMockHttpServletRequest:171-181throws the "Object-to-XML conversion requires grails-xml" message fromgetXML(), which parses rather than converts.DefaultJsonRenderer:83keeps the deprecatedHttpStatus.UNPROCESSABLE_ENTITYwhile the factory usesUNPROCESSABLE_CONTENT, andValidationProblemDetailFactorynever setsinstancealthough the resource path is in the context (Spring MVC sets it).
|
@jdaugherty Addressed on c76fe76, with 9.0.x merged in. Migration path:
Minor items:
|
…tion's messages ValidationProblemDetailFactory resolved the message of each error with the MessageSource for the current locale, but wrote the problem's title and detail in English, where Spring MVC resolves those of the problems it writes through message codes. The title and detail are now the messages of the grails.validation.problem.title and grails.validation.problem.detail codes, the latter given the number of errors, for the current locale, and the English text they had otherwise.
A map association written as references iterated its entries itself, so a null key was written as "null", where Jackson fails on a null key of a map unless the mapper has a null key serializer, as the upgrade guide says; and ORDER_MAP_ENTRIES_BY_KEYS did not order its entries. A null key is now written with the mapper's null key serializer, which fails by default, and the entries are ordered by their keys where that feature is enabled, a null key first, as Jackson's map serializer orders them, failing for keys that are not comparable where FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY is enabled.
grails-converters finds the XML converter of grails-xml by its name, since grails-xml depends on it, so that renaming the converter would still compile and leave the XML encoder escaping objects as their strings. The binding source registry and the converters initializer, which find grails-xml classes the same way, are tested from grails-xml; the encoder was not. grails-xml now tests that the encoder writes an object with the XML converter, and grails-converters that it escapes it as its string without grails-xml.
No example application set grails.web.rendering.json.spring or grails.databinding.json.jackson, so the Spring JSON path of respond, the Jackson parsing of request bodies and the problems Spring MVC writes were tested only in unit tests, whose wiring copies Spring Boot's. The spring-json-respond example sets them, with grails.converters.json.legacy false, and tests over HTTP that respond writes a domain class instance with the Grails mapper and the excludes it is given, that a JSON body is bound with Jackson, and that the validation errors of a body, a ProblemDetail and an ErrorResponse are written by Spring MVC with the application's ResponseBodyAdvice, in JSON for a client that asks for XML without jackson-dataformat-xml, and with the status alone for an Accept header Spring MVC cannot read.
Grails 10 makes Spring JSON rendering for respond and Jackson parsing of request bodies the default, while grails.converters.json.legacy was to change in a later release. Its defaults would then be the combination the upgrade guide warns against: data binding parsing strict JSON with Jackson, while request.JSON and JSON.parse accept what the legacy parser accepts, and render ... as JSON writing with the Grails 8 marshallers while respond writes with the Grails mapper. The guides, the startup warning, the configuration metadata and the deprecation notices of the legacy marshallers now say that Grails 10 changes the three JSON settings together. The Grails 9 defaults are unchanged.
With grails.web.rendering.json.spring, respond picked a converter of Spring MVC's and wrote the value itself, through the response's writer: with the Grails mapper after the converter's prefix, or with the converter through a bridge from its bytes to the writer, and a named configuration wrote to the writer directly. Only problems went through Spring MVC, so the application's ResponseBodyAdvice applied to them and to no other JSON respond wrote. The value is now written by Spring MVC's RequestResponseBodyMethodProcessor, as the value a @responsebody handler method of the controller returns: with the application's ResponseBodyAdvice beans, found as the handler adapter finds them and applied as to a method of the controller, in the type respond negotiated, with the converter it selected, or with a GrailsMapperJsonConverter that writes with the Grails mapper derived from Spring's Jackson converter, its types and prefix, and the projection of the response; a named configuration is written by a converter of its own. Since Spring's converter creates its generator apart from the write, a write of the Grails mapper keeps the objects it is writing in the writer, so that one a serializer writes with writePOJO is still found where it repeats. JSON written this way is in UTF-8 and labelled application/json, or text/json, as Spring MVC writes it, whatever grails.converters.encoding says, which the legacy converter and render json: still write in.
Spring MVC's ExceptionHandlerExceptionResolver applies @ExceptionHandler methods, those of a controller and of the @ControllerAdvice beans, only to a handler method, which a Grails action is not, so neither those methods nor Spring Boot's handler of spring.mvc.problemdetails.enabled saw an exception a Grails action threw, which the Grails exception resolver rendered the error page for. With grails.exceptionresolver.springHandlers, an ActionExceptionHandlerResolver, ordered ahead of Spring MVC's resolvers as that resolver is ahead of Spring MVC's others, gives Spring MVC's resolver the action as a handler method of the controller, so that the exception handler methods of the controller and of the advice scoped to it resolve the exception as for a Spring MVC controller, and one none of them handles is resolved as before. It is off by default in Grails 9 and planned as the default in Grails 10, as the other settings of this change are, since advice an application meant for its Spring MVC controllers alone would otherwise start answering the exceptions of its Grails actions.
…orward The gated writers wrap Jackson's writer of the entries of a @JsonAnyGetter and its writer of an unwrapped property in subclasses of those classes, which Jackson takes for its own, and forward to the writer they wrap the methods that read its state, such as the accessor and the serializer of the entries, which a subclass cannot read from another instance. A method a later Jackson added to those classes would run on the wrapper's own, unset state, and fail, or write ungated, only when a response wrote such a property. A contract test now fails where those classes declare a method the wrappers neither override nor are reviewed not to need. Jackson 3.2, which Grails manages, added two for an ObjectNode of entries, which only the methods the wrapper forwards call, and a test writes such entries with a projection. Javadoc of the gated writer says why the gates are writers rather than a PropertyFilter: a mapper has one FilterProvider, which the application's filters use, which Spring's Jackson converter replaces with the one a write is given, and which an application's SimpleFilterProvider fails for a filter id it does not know.
… grails-xml e0328c6 kept grails-xml's own uses of the deprecated XML converter free of removal warnings, but XmlRuntimeHints, which registers the converter by its class, was still reported by javac. It now suppresses them, as the marshallers do.
The Vnd.Error page told new applications to use the RFC 9457 problem that grails.web.rendering.json.spring opts into, ahead of the Grails 10 default, while Grails 9 sends the errors body the page shows first. It now describes the problem as what the setting sends.
ActionExceptionHandlerResolver looked up the action's method by the name the URL gives it, which the hyphenated URL converter turns into show-book for a showBook action, so the @ExceptionHandler methods of such an action were not found. It now looks the method up by the name the controller class maps the URL's name to.
An action that returns a ResponseEntity or another HttpEntity, HttpHeaders, a ProblemDetail, an ErrorResponse, a StreamingResponseBody or a ResponseBodyEmitter now has it written by the return value handlers of Spring MVC's handler adapter, as the value of a handler method is, where Grails did not write it. So does any value of an action annotated @responsebody, or of an action of a controller annotated so, also through an annotation such as @RestController, whose annotation Grails ignored. The ResponseBodyAdvice beans that apply to the controller, and the action's @JSONVIEW and @ResponseStatus, apply, and a CompletableFuture, DeferredResult or Callable is processed asynchronously, with the executor, timeout and interceptors of the handler adapter, and the value it completes with written on the redispatch. What an action of an error page returns is written whatever the status of the response, which the new GrailsWebRequest.isResponseRendered() tells apart from isRenderView(). A null value, the value of an action that rendered the response itself, redirected, forwarded or processes the request asynchronously already, the model of an action not annotated @responsebody, and an application without Spring MVC's handler adapter keep the Grails behaviour.
Grails 8 ignored @responsebody on an action, or on its controller, also through @RestController, and rendered the action's view for a model, so Spring MVC writes the value of such an action as the body only with grails.controllers.responseBody: true, which Grails 10 makes the default, as grails.exceptionresolver.springHandlers opts actions into Spring MVC's exception handlers. The values Spring MVC writes whatever a handler method is annotated with, such as a ResponseEntity, which Grails 8 did not write, are written whatever it says.
With grails.web.rendering.xml.spring, respond wrote an ordinary bean with the Spring XML converter through the response's writer, so the application's ResponseBodyAdvice beans did not apply to it as they apply to the JSON respond writes through Spring MVC. It now writes it as Spring MVC writes the value of a @responsebody handler method of the controller, with the converter it selected and the type respond negotiated, in UTF-8 as Spring MVC writes XML, whatever grails.converters.encoding says. The writer bridge that only this path used, WriterOutputStream and SpringMessageConverters.writeCharset, is removed. The values the Grails XML converter writes keep its path.
respond wrote a value through Spring MVC's response body handler as the return value of a method that stood for the action, so Spring's JsonViewResponseBodyAdvice, which reads @JSONVIEW from the handler method, never saw the action's. The handler method is now the action's own, without parameters, which carries its annotations, of the controller, and declared to return Object, where the request names an action, as Grails and the controller unit tests name it. A problem respond writes keeps the stand-in, so that a view meant for the action's value does not leave its properties out, and a named configuration, which is not a Jackson converter, is not given the view.
The default detail of a validation problem nested {0,number,#} in a
choice format, whose parser in Java 17 and 21 reads the # as the limit
separator of a choice, so every validation problem failed with "Choice
Pattern incorrect" there, which Java 25 parses. {0,number,0} writes the
count the same way, without a grouping separator, on each of them.
|
@jdaugherty Thanks for round 4. Your notes are answered in their threads. Since the approved head (a30f1d4) there are 24 commits, and several change what round 4 verified, so they need a look before merge:
The module tests of each change, the |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Round 5, verified at 785f668: the 24 commits since a30f1d4, read in full, sources, tests, docs and the spring-json-respond example, plus the 9.0.x merge on top, which brings only the MongoDB runner image and the JaCoCo cli source set. CI on e8df1c3, the last head before that merge, is green but for two jobs that do not touch this change: the Hibernate7 functional job (Java 25, indy=false) failed with Geb's DriverCreationException creating the Chrome container driver, so OsivGspRenderingSpec, FieldsValidationSpec and ScaffoldingFunctionalSpec never ran, while SpringJsonRespondSpec passed all 17 features in that job and on the Java 21 lane; the Groovy snapshot canary failed on the MongoDB flake of #16030, which the merge's runner change addresses.
Verified this round:
- The four notes of round 4: the prefix is read once per converter in a weak map, with
DefaultJsonRendererSpeccovering a reconfigured converter and another converter;ProblemXmlandProblemBeansare gone and the XML renderer writes a problem as Spring MVC does;XmlRuntimeHintscompiles without the removal warning; the Vnd.Error page no longer steers new applications to the problem response. - Problems:
respondhands aProblemDetailto the first return value handler of the application'srequestMappingHandlerAdapterthat supportsResponseEntity<ProblemDetail>, which isHttpEntityMethodProcessorwith the adapter's converters, negotiation manager, advice andErrorResponse.Interceptors. The producible types are the problem types a converter writes,problem+xmlonly where the action listsxml, and the containing class of the return type is the controller, so@ControllerAdvice(assignableTypes = ...)scopes as for a Spring MVC controller. AnAcceptheader Spring MVC cannot read gives the status alone for a 4xx or 5xx andHttpMediaTypeNotAcceptableExceptionotherwise, which Spring MVC's composite resolver, at order 0, answers with 406 ahead of the GrailsexceptionHandlerbean, which keepsAbstractHandlerExceptionResolver's lowest precedence. AnErrorResponsehas its message codes resolved with the application's messages and the request locale before negotiation, and its headers reach the response on every path, a view's included. - JSON and XML through
RequestResponseBodyMethodProcessor: the handler method is the action's no-argument method, onto whichControllerActionTransformercopies the annotations, so@JsonViewapplies to an action that takes parameters;ActionReturnTypereports the controller as containing class andObjectas the type, and a problem or a named configuration keeps the stand-in. The advice is the@ControllerAdvicebeans that areResponseBodyAdvice, inControllerAdviceBeanorder ahead ofJsonViewResponseBodyAdvice, as the adapter orders them.WriterOutputStreamandwriteCharsetare gone with their last reader. - Actions returning Spring MVC values:
isBodyleavesnull, a rendered, committed, redirected or forwarded response and a request already in concurrent handling to Grails; the listed types are written whatever the settings, and@ResponseBodyvalues only withgrails.controllers.responseBody, read throughAnnotatedElementUtils, so@RestControllerand a superclass count. The value is typed by itself, as Spring MVC types the value of a method declared to returnObject; the@ResponseStatusis applied asRequestMappingHandlerAdapterapplies it,sendErrorfor a reason; the async state is copied from the adapter's private fields, whose names I checked against spring-webmvc 7.1.0-M2. Grails 8 answered a returnedResponseEntitywith an empty 200, since the adapter built noModelAndViewfor a value that is neither aMap, aModelAndViewnornull, so writing it now changes nothing an application could have relied on; the compatibility section and section 15 say so, and that is the right call for a value whose only effect was an empty response. grails.exceptionresolver.springHandlers:ActionExceptionHandlerResolver, at order -1, handsExceptionHandlerExceptionResolverthe action as aHandlerMethod, soshouldApplyTopasses and both the controller's@ExceptionHandlermethods and the advice scoped to it resolve the exception; without the setting, and for a handler that is not a Grails mapping, it answersnull. A Grails exception handler method, oneExceptionparameter, is still caught inside the action by the transformer, so it keeps precedence. The URL converter case resolves throughactionUriToViewName.grails.converters.json.legacychanging in Grails 10 together withgrails.web.rendering.json.springandgrails.databinding.json.jackson: confirmed. A Grails 10 that bound and responded with Jackson whilerequest.JSONandrender as JSONkept the Grails 8 parser and marshallers would be the mix the guide warns against. The schedule stays deprecate in 9, change in 10, remove in 11, and the guide, the metadata, the startup warning and the deprecation notices now all say Grails 10;MigrationSettingsMetadataSpecpins the five defaults to the metadata.- Binding reads the mapper of the adapter's first converter that reads a JSON
Map, Spring's Jackson converter only, so a Gson-first application keeps the application mapper. The validation problem's title and detail resolve throughgrails.validation.problem.titleand.detailwith the English text as fallback, and theChoiceFormatparse of{0,number,#}on Java 17 and 21 is fixed with{0,number,0}. - Web unit tests get a lazy
requestMappingHandlerAdapter, built byWebMvcConfigurationSupportfrom the test's converters, negotiation manager andWebMvcConfigurerbeans with the validator initialised;ControllerProblemHandlerAdapterSpecandControllerProblemSpringMvcSpeccover advice and interceptors, and a test's own adapter bean wins. - Map associations: a
nullkey fails with Jackson's own message, andORDER_MAP_ENTRIES_BY_KEYSorders the entries with anullkey first, as Jackson's map serializer does; the gated writers have a contract spec; the XML encoder's lookup ofgrails.converters.XMLis tested on both sides of the module boundary. - The
spring-json-respondexample runs every setting the later releases default to over HTTP,spring.mvc.problemdetails.enabledincluded.
Four non-blocking notes inline: the English fallback detail is formatted for the request's locale, text/json on the application-converter path, What's New for sections 14 and 15, and the Javadoc of MvcAdapter. Nothing blocks the merge once CI on the merge commit is green.
| return message; | ||
| } | ||
| } | ||
| return new MessageFormat(defaultMessage, locale).format(arguments); |
There was a problem hiding this comment.
Nit. The fallback text is English, but it is formatted for the request's locale, so a client in a locale whose numbering system is not Latin reads Request validation failed with ٢ errors. (ar-EG; fa formats 1000 as ۱۰۰۰). new MessageFormat(defaultMessage, Locale.ENGLISH) keeps the fallback consistent with its words; a translation of grails.validation.problem.detail still gets its own locale's digits through the MessageSource.
There was a problem hiding this comment.
Done in f6d5f9e. The default title and detail are formatted with Locale.ENGLISH, the language of their words, and a translation of grails.validation.problem.title or .detail that the MessageSource resolves keeps the digits of its locale. ValidationProblemDetailFactorySpec writes 2 for ar-EG and 1000 for fa.
| projection?.reportUnapplied() | ||
| return null | ||
| } | ||
| SpringResponseBody.write(object, (ServletRenderContext) context, converter, converterType) |
There was a problem hiding this comment.
Non-blocking. On this path a client that asked for text/json is answered as application/json: converterType is the rewritten type, and the fixed strategy SpringResponseBody builds from it selects it. The Grails-mapper path above passes mediaType, and GrailsMapperJsonConverter adds text/json, so only an application's own converter answers such a client with the other label; before 713e9ce the renderer's text/json stayed on the response, since the converter wrote into its writer. The respond reference says text/json for a client that asks for it. Either qualify that sentence, an application converter writes the JSON type it supports, or wrap the application converter as the Grails mapper is wrapped; the doc change is enough for me.
There was a problem hiding this comment.
Took the doc change in 6b59179. The respond reference and section 4 of the upgrade guide now say that a client asking for text/json gets it where the Grails mapper or a named configuration writes the JSON. They also say that an application's own JSON converter writes a JSON type it supports: a subclass of Spring's Jackson converter, one with mappers for the type, or a converter of another library.
| the action responds in XML and `jackson-dataformat-xml` is present, with the status the problem carries unless `respond` | ||
| is given one, as a `ResponseEntity` is in Spring MVC. A Spring `ErrorResponse`, such as an `ErrorResponseException`, | ||
| is answered the same way, with its status and headers. `ValidationProblemDetailFactory` creates a problem for | ||
| validation errors. |
There was a problem hiding this comment.
Commenting on the last line of this paragraph, though the ask is for the section: sections 14 and 15 of the upgrade guide, Spring MVC's @ExceptionHandler methods for actions (grails.exceptionresolver.springHandlers) and actions returning Spring MVC responses (ResponseEntity, ProblemDetail, StreamingResponseBody, and @ResponseBody with grails.controllers.responseBody), have no entry in What's New, and the pointer at the end of this section still reads "sections 3 to 10, 12 and 13". Both are headline features of 9.0 with a guide page each, <> and the addition to <>, so please give them a paragraph here and extend the pointer to 14 and 15.
There was a problem hiding this comment.
Added in 401c21c. One paragraph covers grails.exceptionresolver.springHandlers and links to Declarative Controller Exception Handling. Another covers actions returning Spring MVC responses, with @ResponseBody under grails.controllers.responseBody and the action's @JsonView for respond, and links to Returning Spring MVC Responses. The pointer now reads sections 3 to 10 and 12 to 15.
| * configuration and the application's {@code WebMvcConfigurer} beans set them. | ||
| */ | ||
| @CompileStatic | ||
| private static final class MvcAdapter { |
There was a problem hiding this comment.
Non-blocking. MvcAdapter reads four private fields of RequestMappingHandlerAdapter, which has no getters for them, and field answers null where a Spring release renames one, so the loss would be silent at runtime: the default SimpleAsyncTaskExecutor, the container's timeout, no interceptors. ActionReturnValuesSpec pins all four, the return-values- thread name, the 1234 timeout and the INTERCEPTED and DEFERRED attributes, so a rename fails the build rather than a request. Please say so in this Javadoc, as GatedPropertyWriter names its contract spec, so that whoever touches this next keeps that coverage.
There was a problem hiding this comment.
Done in 36a1e7b. The Javadoc says the four are read from private fields and that a renamed field reads as null, which falls back silently to the default executor, the container's timeout and no interceptors. It names ActionReturnValuesSpec as the spec that pins each of them, so that the coverage is kept.
Without a message of grails.validation.problem.title or .detail, the English default was formatted for the request's locale, so a client in ar-EG read "Request validation failed with ٢ errors." and one in fa read ۱۰۰۰ for 1000. The default is now formatted in English, the language of its words; a translation the MessageSource resolves keeps the digits of its locale.
respond answers a client that asks for text/json with it where the Grails mapper or a named configuration writes the JSON, both of which write text/json, but an application's own JSON converter, such as a subclass of Spring's Jackson converter, one with mappers for the type or a converter of another library, writes a JSON type it supports, which Spring MVC selects. The respond reference and the upgrade guide say so.
Sections 14 and 15 of the upgrade guide, Spring MVC's exception handlers for actions with grails.exceptionresolver.springHandlers and actions returning Spring MVC responses, with @responsebody under grails.controllers.responseBody, had no entry in What's New. Each now has a paragraph with its guide page, and the pointer to the upgrade guide names sections 12 to 15.
MvcAdapter reads the executor, timeout and interceptors of Spring MVC's handler adapter from its private fields, and a field a Spring release renames reads as null, which would fall back silently at run time. Its Javadoc now says so and names ActionReturnValuesSpec, which pins all four, as GatedPropertyWriter names its contract spec.
|
@jdaugherty Thanks for round 5. The four notes are answered in their threads. 00e5455 merges 9.0.x again, up to 2a92881, which brings the GORM multi-tenancy and Hibernate fixes, the GSP build cache and the CI policy. It merges cleanly and touches none of this change's files. The four commits for the notes are on top of it, so the run that has to be green is now the one on the head, 36a1e7b, which includes the merge. On that head, the module tests, the |
🚨 TestLens detected 239 failed tests 🚨Test SummaryTip Inspect all details with the TestLens CLI: CI / Build Grails Forge (Java 21, indy=false) > :test-core:test [grails-forge]
CI / Build Grails Forge (Java 21, indy=true) > :test-core:test [grails-forge]
Groovy Snapshot Canary Build / Build Grails (shard 0) > :grails-test-examples-mongodb-hibernate5:test
Groovy Snapshot Canary Build / Build Grails (shard 1) > :grails-data-mongodb-core:test (first 40 of 224)
Groovy Snapshot Canary Build / Build Grails (shard 1) > :grails-test-examples-mongodb-test-data-service:integrationTest
Groovy Snapshot Canary Build / Build Grails (shard 2) > :grails-data-mongodb-spring-boot:test
Groovy Snapshot Canary Build / Build Grails (shard 2) > :grails-test-examples-mongodb-base:test
🏷️ Commit: 36a1e7b Test Failures (first 10 of 236)BookFongoSpec (:grails-test-examples-mongodb-hibernate5:test in Groovy Snapshot Canary Build / Build Grails (shard 0))AggregateMethodSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))AssignedIdentifierSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))AttachMethodSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))AutowireServicesSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))BasicArraySpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))BasicCollectionTypeSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))BasicCollectionsSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))BatchUpdateDeleteSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))BeforeInsertUpdateSpec (:grails-data-mongodb-core:test in Groovy Snapshot Canary Build / Build Grails (shard 1))Rerun Controls (first 20 of 239)Select tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app/docs. |
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Approving. I did not find a production regression in the MIME and serialization changes. Grails 9 keeps the legacy JSON defaults, Spring rendering does not let the generic string and byte-array converters win JSON selection, and the XML extraction and problem-media-type changes match the documented behavior. Spring MVC keeps its own negotiation rather than inheriting Grails' permissive fallback parser. This is a source review; I did not re-run the Gradle suites.
4 participants
Summary
Modernizes Grails MIME negotiation and JSON/XML rendering on Spring Boot 4.1, with a migration path. In Grails 9 the Grails 8 behaviour stays the default, apart from the changes Compatibility lists. Grails 10 makes Spring JSON rendering for
respond, Jackson rendering forgrails.converters.JSON, Jackson parsing of request bodies for data binding, and Spring MVC's exception handlers and@ResponseBodyfor controller actions the default. Grails 11 removes the legacyrespondrendering and request-body parsing, the JSON converter's named configurations and legacy XML conversion.MediaType, and the Grails data formats are registered with Spring MVC as format aliases. The static negotiation state is gone.respond()can render JSON as Spring MVC writes the value a@ResponseBodyhandler method returns, with its response body handler, so the application'sResponseBodyAdvicebeans apply, scoped to the controller as for a Spring MVC controller, also for ajsonConfiguration, and the action's@JsonViewselects the properties written, other than a problem's or a named configuration's. A Grails mapper derived from Boot'sJsonMapperwrites domain objects with Jackson's bean serializer, shaped asgrails.converters.JSONrenders them withlegacy: false: persistent properties, association references, proxies on every GORM datastore,includes/excludesprojections and circular references. Jackson annotations, mix-ins and the mapper's naming and inclusion apply.grails.converters.JSONcan render through the application'sJsonMappertoo, andJSON.parseandrequest.JSONthen parse with it.NamedJsonConfigurationRegistryis shared byrender json:,respondand direct serialization, with a built-indeepconfiguration in place ofJSON.use('deep')._errors.gsonrenders them, as with JSON views, with a title and detail the application's messages translate.respondhands aProblemDetail, or anErrorResponsesuch asErrorResponseException, to the return value handler of Spring MVC's handler adapter, whatever the settings, so it is written as by a Spring MVC controller method that returns it: with the application'sResponseBodyAdviceandErrorResponse.Interceptorbeans, the problem's status, or astatusargument as the status of aResponseEntity, anErrorResponse's headers and message codes, and the type Spring MVC's content negotiation selects. That isapplication/problem+xmlonly where the client asks for a problem in XML, the action responds in XML and Spring MVC has an XML converter, which needsjackson-dataformat-xml, andapplication/problem+jsonotherwise, clients asking forapplication/xmland browsers included. A request Spring MVC cannot negotiate, such as one whoseAcceptheader has a quality value above 1, gets the problem's status alone, or Spring MVC'sHttpMediaTypeNotAcceptableException(406) for a status that is not an error status. A view renders a problem only whenrespondnames it, and where the formatrespondpicks has no view of the name, a GSP in HTML, the problem is written in that format if it is JSON or XML, the JSON view of the name renders it in any other if there is one, and otherwise it is written as without a view. A renderer the application registers for problems renders them; the default renderer of a format, and one registered forObject, do not.grails-xmlmodule, which the Grails 9 web starter andgrails-web-bootstill bring, so generated applications keep XML until Grails 10. Plugin descriptors are unchanged, and the XML escaping codec stays ingrails-converters.@RequestBodywith.grails.exceptionresolver.springHandlers: true, an exception a controller action throws is resolved first by the@ExceptionHandlermethods of the controller and of the@ControllerAdvicebeans that apply to it, Spring Boot'sspring.mvc.problemdetails.enabledhandler among them; one none of them handles reaches the Grails exception resolver as before.ResponseEntityor anotherHttpEntity,HttpHeaders, aProblemDetail, anErrorResponse, aStreamingResponseBodyor aResponseBodyEmitter, and, withgrails.controllers.responseBody: true, any value of an action annotated@ResponseBody, or of an action of a controller annotated so, also through@RestController. The return value handlers of Spring MVC's handler adapter write it, with theResponseBodyAdvicebeans that apply to the controller and the action's@JsonViewand@ResponseStatus, also for an action of an error page, and aCompletableFuture,DeferredResultorCallableis processed asynchronously with the adapter's executor, timeout and interceptors, the value it completes with written as the action's.doWithConfigis applied, its content negotiation manager, which decides the type of aProblemDetail, from those converters,spring.mvc.contentnegotiation.*and the test'sWebMvcConfigurerbeans, and Spring MVC's handler adapter from both, so that theResponseBodyAdviceandErrorResponse.Interceptorbeans a test declares apply, unless the test declares them. They keep the URL mappings holder, link generator and locale resolver a test defines, also under an alias of its name or from a@Beanmethod its configuration class inherits, and any locale resolver the context has, the one the application configures among them, asJsonViewUnitTestdoes now; with the URL mappings plugin included, a test declares its holder indoWithSpringorbeanRegistrar().Usage
Compatibility
JSON.registerObjectMarshallerstays supported forrender ... as JSONand the legacyrespond()path.JSON.createNamedConfig,JSON.use,JSON.getNamedConfigandJSON.withDefaultConfigurationare deprecated for removal in Grails 11 in favor of the registry.jsonConfiguration:selects only registry configurations, for the default JSON renderers andJsonRendererbeans;respondwrites aProblemDetailas Spring MVC writes it, whatever configuration,includesorexcludesit is given, and validation errors as they are without a configuration.withFormat,response.formatandresponse.mimeTypesresolve as in Grails 8, except that media type names in theAcceptandContent-Typeheaders now match case-insensitively, as do parameter names such asq; that the problem types of RFC 9457 are thejsonandxmlformats, read asapplication/jsonandapplication/xml, so a body sent asapplication/problem+jsonbinds as JSON, and a client that sends only one is answered as one that asks for that format; thattext/xmlandapplication/xmlin oneAcceptheader merge intoxmlwith the larger quality and the place of the one listed first; and that eachgrails.mime.disable.accept.header.userAgentsentry is a regular expression of its own, matched ignoring case, with an invalid one failing at startup. Spring MVC's own negotiation (@RestController,produces,spring.mvc.contentnegotiation.*) is unchanged. Grails registers its data-format aliases (json,xml,hal,atom,rss,csv,text) over Spring Framework's defaults, so a typegrails.mime.typesgives one of them replaces Spring's, whilespring.mvc.contentnegotiation.media-typesand the application'sWebMvcConfigurerbeans override them. Spring MVC negotiates with them only whenspring.mvc.contentnegotiation.favor-parameteris enabled; they also count as safe extensions in its reflected-file-download check, which onlyhalwas not already.grails.converters.json.legacy: false, the JSON converter has Jackson write collections, maps, arrays, records, enums and beans, as Spring Boot does, and Grails adds only its own rules (enums by name, the converter's includes and excludes, which name a property by its bean name or the name Jackson writes it with,@PersistenceMethod/@ControllerMethodmethods left out). Domain classes render as Jackson beans of their persistent properties. Validation errors are written without the rejected value, a map with anullkey fails, aMap.Entryis written as{"k":1}, a value nested deeper than the mapper's limit, 500 levels by default, fails with aConverterException, andJSON.parseaccepts strict JSON unlessspring.jackson.json.read.*features allow more, sograils.converters.json.date: javascript, whosenew Date(...)is not JSON, fails at startup withlegacy: false. The Grails 8 rendering stays the default in Grails 9 (legacy: true).GStringserializer, plus the domain serializer whengrails.converters.json.domain.jackson.enabledis set;Errorsand projection handling live on the Grails mapper, which writes every valuerespondwrites with Jackson but a problem, which Spring's converter writes with Boot's mapper. An application's Jackson serializers take precedence over the Grails ones in both the converter andrespond, also for an associated domain class instance, which is otherwise written as a reference, and whererespondwrites with a Jackson mapper it reports a failure to write as Spring MVC's converters do, so that Spring MVC answers it before the Grails exception resolver.grails-convertersdepends onspring-boot-jackson, so Spring Boot'sJsonMapperis in every application context that has the converters. An application withoutspring-boot-starter-web, which the web starter brings with SiteMesh 3, now has it too, and Spring MVC's JSON converter writes with it.respond problemDetailanswers as Spring MVC answers a handler method that returns it: with the problem's status, asapplication/problem+xmlfor a client that asks for a problem in XML where the action responds in XML and Spring MVC has an XML converter, and asapplication/problem+jsonfor any other, also withoutjackson-dataformat-xml. Grails 8 wrote the problem as a bean with a 200 to JSON and XML clients, and rendered the action's view with a 200 for any other.respond errorResponseanswers with its problem, status and headers, where Grails 8 wrote it as a bean.ResponseEntityor anotherHttpEntity,HttpHeaders, aProblemDetail, anErrorResponse, aStreamingResponseBodyor aResponseBodyEmitterhas it written, whatever the settings, which Grails 8 did not do. An action that returnsnull, or renders, redirects or forwards itself, is unaffected. Withgrails.controllers.responseBody: true, the value of an action annotated@ResponseBody, directly or through an annotation such as@RestController, whose annotation Grails 8 ignored, is written as the body in place of the action's view, before an interceptor'safterruns, which then sees no model; without it, such an action renders its view as in Grails 8. Spring MVC writes a returned domain class instance with Boot'sJsonMapper, as Grails writes it only withgrails.converters.json.domain.jackson.enabled.GrailsWebRequest.isResponseRendered()tells a response an action rendered from one whose status is an error's, whichisRenderView()is false for either way.respondwrites through Spring MVC, withgrails.web.rendering.json.springor ajsonConfiguration, and problems are written in UTF-8 and labelledapplication/json, ortext/jsonfor a client that asks for it, as Spring MVC writes JSON, whatevergrails.converters.encodingsays, which the legacy converter andrender json:still write in. So is the XMLrespondwrites with a Spring converter undergrails.web.rendering.xml.spring, in the typerespondnegotiated.respondanswers in the format it found a renderer for, labelled so, and a client that accepts any type in the first of the action's formats a renderer renders the value in, so withoutgrails-xmlan action that lists['xml', 'json']answers such clients in JSON. Validation errors try the client's next type where its first has no renderer. Grails 8 took the action's first format and sent 406, or 404 for validation errors, when nothing rendered it. With JSON views, validation errors with no errors template are written as without JSON views, with a 422, where Grails 8 rendered the object template,nullwith a 200.grails-xmlleaves the web starter andgrails-web-bootin Grails 10, when@Resourcebecomes JSON-only. Without it,request.XMLthrows anIllegalStateExceptionthat names the module; a library compiled against Grails 8 that readsrequest.XMLorrequest.JSONstatically runs without being recompiled. Legacy XML conversion is deprecated for removal in Grails 11:grails.converters.XML, the XML, HAL XML, Atom and Vnd.Error XML renderers, and the XML validation-error marshaller.grails.converters.json.legacysays, the converter's includes and excludes for a proxy, whichrespondpasses it, apply to the instance written in its place, with theDEFAULTcircular reference behaviour an object that refers to itself is written as{"_ref":".",…}, and a value nested deeper than the thread's stack lets the converter write fails with aConverterException; Grails 8 ignored the first, failed on the second and threw aStackOverflowErrorfor the third.respond's JSON, the XML converter and the HAL XML, Atom and Vnd.Error XML renderers when they first render, and request-body parsing at startup. Whilegrails.converters.json.legacyis not set, a warning at startup says its default changes in Grails 10. Withgrails.web.rendering.json.springenabled, each type the legacy converter still writes is reported once for each reason, such as a projection asked of an application's own converter.grails.web.rendering.json.spring,grails.web.rendering.xml.spring,grails.databinding.json.jackson,grails.exceptionresolver.springHandlersandgrails.controllers.responseBodyare constants ofgrails.config.Settings.Limitations
grails.web.rendering.xml.spring: true, only ordinary beans use Spring's XML converter. Domain objects, collections, maps, arrays, validation errors, projections and named configurations stay on the Grails XML converter, so XML validation errors keep the rejected values the problem JSON leaves out.grails.converters.json.legacy: false, the JSON converter writes a property a bean unwraps with the name of another twice, where Jackson, and the mappers of responses and named configurations, reject the bean: the class name and version of a domain class instance it writes depend on the rendering of each write, so the check would reject beans whose ownversionnever conflicts.respond.withFormat, and Spring MVC's the media type of what it writes, so the two can answer one request differently; resolving Grails formats through Spring MVC's negotiation is Resolve Grails formats through Spring MVC's content negotiation #16698.ResponseBodyAdviceapplies to the JSONrespondwrites through Spring MVC, to the ordinary beans it writes with Spring XML undergrails.web.rendering.xml.spring, to problems and to the values of actions Spring MVC writes; the legacy JSON and XML converters and views do not go through Spring MVC's response body handler.ResponseEntityof aStreamingResponseBodyor aResponseBodyEmitteris streamed where the action declares that type and returns it, not where an asynchronous value the action returns completes with it, which Spring MVC types by the value it completes with.