Repository navigation
Write grails.converters.JSON through Spring Boot's JsonMapper - #16414
codeconsole wants to merge 29 commits into
Conversation
grails.converters.JSON now writes JSON with the application's Jackson JsonMapper, the one Spring Boot auto-configures, instead of Grails' own JSON writer and a marshaller per value type. Values render as Spring Boot renders them, and spring.jackson.* properties apply to render ... as JSON. - JacksonJSONWriter is a JSONWriter that writes to a JsonGenerator, keeping JSONWriter's state and value semantics, so existing object marshallers and the PathCapturingJSONWriterWrapper keep working - JsonMapperValueMarshaller renders the values the mapper has a dedicated single-value serializer for (dates and times, Month, UUID, Locale, byte[], @jsonvalue and Jackson module types); beans, enums, records, Optional and containers stay with Grails marshallers, so the values they contain are rendered as any other value is - RecordMarshaller and OptionalMarshaller render records and Optional as Spring Boot does - HtmlSafeJsonWriter keeps escaping </ and U+2028/U+2029 in every string - Map keys are written by the mapper's key serializers - Enums keep rendering by name(); a Month renders as its number and binds back from it - The per-type JSON marshallers for dates, times, Locale and byte arrays are no longer registered; the java.time ones are removed - PathCapturingJSONWriterWrapper handles null and Number values and single values The differences from Grails 8 are listed in the new 9.0 upgrade guide.
…shallers and parser JSONWriter itself now writes through a Jackson JsonGenerator, in place of the separate JacksonJSONWriter. It keeps its public methods and state machine, so marshallers and PathCapturingJSONWriterWrapper are unchanged. It never flushes or closes the caller's Writer, and closes the generator once the JSON text is complete. JsonMapperSupport, JsonMapperValue and HtmlSafeJsonWriter move to grails-web-common (org.grails.web.json). Removed: - PrettyPrintJSONWriter (use new JSONWriter(writer, new JsonMapperSupport(mapper), true)) - the JSON DateMarshaller, CalendarMarshaller, ToStringBeanMarshaller and ByteArrayMarshaller, which are no longer registered (XML ones stay) - the unused JavaCC parser in org.grails.web.json.parser and jsonparser.jj; JSON.parse and JSONObject parse with JSONTokener PathCapturingJSONWriterWrapper also passes flush() and append(Writable) on, and a JsonMapperValue is written by its own mapper. The 9.0 upgrade guide lists the removals and the JSONWriter changes.
|
Reviewed at head 1.
|
|
Following up on the custom-serializer gap above: this is 9.0.x, so the fix doesn't need to be bulletproofing `registerObjectMarshaller` against every nested custom Jackson serializer — that's a compatibility guarantee worth breaking on purpose here, not preserving by accident. The registered `JsonMapper` is the standard Spring pattern (that's what my repro used), and once an app hands Jackson a serializer, Jackson owns what's reachable from it — that's a reasonable 9.0 stance. It just needs to be a stated decision, not a silent one: a line in the 9.0 upgrade guide that a value nested inside a custom Jackson serializer renders as Jackson renders it, not through Grails marshallers, so apps relying on `registerObjectMarshaller` for something like redaction know to also handle it as a Jackson serializer if it can show up under a custom one. |
|
Proposal for the custom-serializer gap above: don't sandbox it, remove the reason it exists. Go fully through Spring Boot's own Jackson customization model instead of the hybrid "ask Jackson if it owns this type" approach, and `registerObjectMarshaller`/`rendersValue()`'s private-API dependency both disappear at the root. Verified against the real `spring-boot-jackson-4.1.1` sources (Boot 4 split this out of `spring-boot-autoconfigure`). `JacksonAutoConfiguration` already does this: @Bean
StandardJsonMapperBuilderCustomizer standardJsonMapperBuilderCustomizer(
ObjectProvider<JacksonModule> modules, AutowireCapableBeanFactory beanFactory) {
return new StandardJsonMapperBuilderCustomizer(this.jacksonProperties, modules.stream().toList(), beanFactory);
}Any `@Bean JacksonModule` in the app context is auto-collected and folded into the one `JsonMapper` Boot builds. Three standard, already-documented ways an app registers a custom marshaller once `ConvertersConfigurationInitializer` is consuming that same mapper (which it already does, via `getBeanProvider(JsonMapper.class).getIfUnique()`):
If `grails.converters.JSON` fully delegated instead of asking "does Jackson recognize this type," an app's custom marshaller becomes a plain `@Bean JacksonModule` or `@JacksonComponent` — no Grails API, and it composes correctly with everything else because there's exactly one serialization mechanism instead of two that have to interoperate. That removes the nested-custom-serializer bypass from above (nothing left to bypass) and the `_serializationContext()` dependency (no "does Jackson own this" question to answer — it owns everything). The part that doesn't fall out for free: `DomainClassMarshaller` — proxies, circular references, `includes`/`excludes`, `deep` associations. No Jackson equivalent exists for that today. Going fully Spring Boot means it has to become a `JacksonModule` Grails ships (GORM-aware serializers registered the same way), not stay a separate `ObjectMarshaller` SPI bolted on the side. That's a bigger scope than this PR currently takes on, but it's the architecturally coherent version of the same goal, and 9.0.x is the right place to make that call rather than carry the hybrid forward. |
Keep Jackson-backed converter rendering while retaining the upstream date-binding fixes and strict month validation. Reconcile date/time tests with the mapper output, share the upstream fixtures, and update the Grails 9 upgrade notes.
JsonMapperSupport called ObjectMapper._serializationContext(), which Jackson documents as public only for testing. The mapper's acceptJsonFormatVisitor passes the same context to the visitor before it visits a type, so take it from there. The application mapper's date format now also has a map key test.
A serializer that a Jackson module provides writes the values nested in its type through the mapper, so a Grails marshaller registered for a nested type does not apply there. State this in the 9.0 upgrade guide and the converters guide, and test it, along with a marshaller for the outer type taking precedence over the module serializer.
A serializer that a Jackson module provides wrote the values nested in its type through the mapper, so a marshaller registered for a nested type, such as one that redacts it, did not apply there, and a cycle through it was not detected. The converter now writes with a copy of the application's JsonMapper whose serializers offer each nested value back to the converter, which renders it as any other value unless it would render it with the mapper anyway. The application's own mapper is unchanged. JSONWriter.writeNested writes such a value and returns the writer to its state, and the PATH writer records the value's location in the mapper's output.
DomainClassSerializer renders a domain class instance as DomainClassMarshaller
did: the id, the version and class name when they are included, the persistent
properties, and associated instances as {"id": ...} references or, rendering
deep, in full. Grails registers it with the application's JsonMapper as the
domainClassJacksonModule bean, so writing a domain class with the mapper,
including returning one from a Spring MVC controller, renders it as
render ... as JSON does. grails.converters.json.domain.jackson.enabled: false
keeps Jackson's bean serialization.
DomainClassMarshaller and DeepDomainClassMarshaller hand the instance to the
serializer, with the converter's includes and excludes and their protected
hooks, and the converter renders the property values. They, and
ProxyUnwrappingMarshaller, are consulted before the mapper's value serializers.
A property annotated @JsonIgnore or write-only is no longer written. The
mapper also leaves out the properties @JsonIgnoreProperties names, as Jackson
does; the converter still renders those, because GORM adds that annotation,
naming the version and tenantId, to every domain class.
A to-many association whose value is a Map failed to render; it is now an
object of references.
|
The parser behaviors are very different between jackson / openjson / the vendored json. I'm ok supplementing it, but I'm not sold on replacing it wholesale. I've mentioned this before on #15958 - the behavior differences have real consequences. Can you make a case on why we should do this? The behaviors are very different and much more strict on jackson. |
|
I asked AI to summarize some of the differences to help this discussion, it's included below (it was done against the 7.x branch, but I think this comparison brings to light several of the problems): Grails JSON: OpenJSON versus JacksonJackson is the stronger long-term option, but neither replacement preserves Grails behavior automatically. OpenJSON is closer to the original API; Jackson offers more control over parsing, numeric precision, and resource limits. I compared the Grails The syntax differences are substantial. Grails deliberately accepts several nonstandard forms, documented in its API. Jackson rejects most by default, with optional features for selected extensions. Sources: Grails JSONObject API and Jackson JsonReadFeature API.
The trailing-content difference is a documented Jackson 3 default change: That exposes a weakness in the “OpenJSON is a compatible replacement” argument: it can both reject previously accepted input and silently change the resulting data. Numeric handling is probably the most consequential difference—even for valid JSON. These are results from the probes:
Grails uses a custom selection rule: integral numbers generally become Jackson can preserve decimal precision with OpenJSON’s parser instead tries The returned object model is a separate compatibility problem.
Grails’ collection interfaces are part of its public API. Replacing them affects casts, collection operations, nested access, and callers expecting the Grails classes—not merely imports. Sources: Grails JSONObject API and Grails JSONArray API. My assessment of the tradeoffs:
OpenJSON explicitly describes itself as an Android-derived clean-room implementation, rather than the same source Grails forked. Its inspected parser also has a default nesting limit of 100. Jackson provides configurable limits for nesting and number/string lengths through I would not assume a performance winner without measuring the actual Grails path. Jackson’s streaming advantage can disappear if the implementation constructs a Jackson tree and then copies it into Grails containers. My recommendation is Jackson behind a Grails compatibility facade, with explicit decisions about which legacy behaviors to retain. An alternative framing is to separate two proposals:
That separation makes the proposal much easier to evaluate. Switching parsers should not implicitly switch Grails databinding or JSON marshalling to Jackson POJO behavior. Before accepting either replacement, I’d require a differential test suite covering the examples above, nested containers, missing versus explicit null, numeric precision and scale, errors, and size/depth limits. OpenJSON needs that suite just as much as Jackson does; API resemblance is insufficient evidence of compatibility. |
|
Even AI recommends this, but acknowledges the scope of it. That's why I'm struggling to accept this. |
|
@jdaugherty I think it summarized best by do we want to support sloppy syntax or go the same direction Spring Boot already went. Open up your browser dev console, paste {"x":TRUE}hit enter |
|
If you can't articulate why we should change, I don't think we should make the change. The same argument could be made for groovy vs java - why don't we use semicolons everywhere? At a minimum, this change is large enough that it should be proposed in a plan and it should be voted on after discussion. I'm a -1 on this for now as a result. I'm not against supporting both, but removing the functionality will break several existing apps with no off ramp / transition. We need to discuss the differences and plan on how to transition. This PR doesn't do either and I don't think this is something we should do on a PR alone. |
|
@jdaugherty It's not a question of groovy vs java. It's a question of valid syntax. All your examples don't work inside a basic browser console. Why would you want to allow Of course I can articulate why we should change, I was just quickly commenting on your comparison. All I was mentioning was Spring Boot 4 went pure Jackson 3. If we are going to leverage and supplement Spring Boot, we probably shouldn't add inconsistencies that basically just introduce bugs downstream. Why would you ever send Why would you be -1 on something that
? You prefer unmaintained Grails json code written 10 years ago and all its "quirks" OR
var x = TRUEdoesn't work in Groovy, Java or Javascript |
|
Groovy PR for Jackson 2 -> Jackson 3 |
|
Like I said, I don't dispute it's probably the right direction, but I'm against telling our users: sorry, it's broken with no gradual migration path. Also, not everyone uses a Grails app with a browser only - https://github.com/openjson/openjson was built by the Android team and has all of the same syntax. This PR is proposing no migration window / timeline. It's just a hard cut off. For something so critical to the framework, that's a huge direction shift and it's not something I want to support. If we're going to do a gradual migration, I'm fine with it. Especially because people can opt into the change when they're ready. |
JSON.parse, and so request.JSON, still parse with Grails' JSONTokener. The spec pins what it accepts beyond strict JSON (single quotes, comments, unquoted values, uppercase literals, trailing commas, octal numbers, new Date(...)), the number types it returns, and its Map and List containers.
…, deprecated Removing them broke code that uses them with no transition: the grails-datastore-rest-client-legacy plugin's RestBuilder creates a ByteArrayMarshaller, and applications import JSONParser directly. They are restored as they were in Grails 8, deprecated for removal in Grails 10. The value marshallers and Grails 8's SimpleTypeMarshaller entries are no longer registered by default. PrettyPrintJSONWriter writes through the generator with a pretty printer that indents as Grails 8's PrettyPrintJSONWriter did, which a new protected JSONWriter constructor takes. JSONParser is not used by Grails; JSON.parse and request.JSON parse with JSONTokener.
A transition to the Jackson rendering, deprecated for removal in Grails 10. With grails.converters.json.legacy: true, the JSON converter registers the marshallers of Grails 8 instead of the JsonMapper value marshallers, indents pretty printed JSON as Grails 8 did, both when rendering with grails.converters.json.pretty.print and from toString(true), and the domain class Jackson module is not registered with the application's JsonMapper. A warning is logged on startup. Grails8JsonRenderingSpec compares the legacy output with text rendered by Grails 8.0.x (232bb34) for 111 values and both pretty print paths. Comparing that text with the Jackson rendering showed that Grails 8 already rendered records as an object of their components, so the upgrade guide no longer lists records as changed. The guide states that parsing is unchanged, and shows the per-type marshallers that keep a Grails 8 format.
|
@jdaugherty This PR doesn't change parsing.
For what the PR does change, which is writing:
So an app can move to 9 with one setting and drop it type by type before 10. If you'd still like this on dev@ before it merges, I'll start the thread. |
|
@borinquenkid Addressed:
|
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Reviewed at 6f42133. This is about the migration path; the implementation points raised earlier on the PR were addressed and I have not re-reviewed them.
The shape is right: parsing is untouched and JsonParsingSpec pins the whole syntax and number table, nothing is deleted, every deprecation names Grails 10, and grails.converters.json.legacy reproduces 8.0.x text byte for byte for 111 values including the C1 and U+2028 string escapes and both pretty-print layouts. What is still the other way round is the order. Grails 9 ships the new output for every application by default, and the Grails 8 output is an opt-out that is deprecated, and warns, from the release that introduces it. The asks below put the Grails 8 output first in 9.x, flip the default in 10 and remove in 11, the same sequence as on #16237, and close two gaps in what the switch covers.
Verified along the way: JSONWriterSpec moved from the web suite to grails-web-common with 18 features instead of 2; under the switch the non-pretty text still comes from the Jackson generator, which the fidelity spec covers; the legacy warning logs once at startup. The red job on this head is a Hibernate 7 datastore spec (SingleTenantSpec), outside this PR's files.
Implementation review
Four inline threads above carry the verified implementation findings (a hang on exotic Number types during a nested hand-off, serializeWithType missing on the domain serializer, the annotation sentence in the guide, and RecordMarshaller). Verified along the way: numbers and strings, including BigDecimal scale, -0.0, control characters, the C1 range, </ and U+2028, are byte-identical to the Grails 8 writer; the state machine throws the same exceptions in the same places; GrailsPrettyPrinter reproduces the old layout; ByDatasourceDomainClassFetcher is still consulted; narrowed proxies and subclass instances resolve to the runtime entity; the converter's circular-reference behaviours still apply to values a module serializer writes; registered marshallers do outrank the mapper (priority -1, -2, ... in list order, registerers at 0).
Minor items that do not need a thread:
JSON.toString(false)withgrails.converters.json.pretty.print=truenow writes compact JSON on both paths; Grails 8 ignored the argument and wrote pretty. No caller in the repo passesfalse; document it or special-case it in the legacy branch.JsonMapperValueMarshaller's javadoc says it sits ahead of the domain class marshaller;addJsonMarshallersregistersProxyUnwrappingMarshallerand the domain class marshaller before it (and the guide says so).ConvertersConfigurationHolder.isLegacyJson()/setLegacyJson()are new public API that exists only for a setting that isforRemoval; they carry no deprecation.- The table of values that render differently misses
char[](Grails 8 wrote[{},{}], the mapper writes"ab") and the string-like JDK types (File,Path,Pattern,Charset,InetAddress,ByteBuffer) that now render as strings where Grails 8 wrote bean properties. The time-zone statement about keys holds forDate/Calendarkeys, but aZonedDateTime/OffsetDateTimekey keeps its own offset. - A MongoDB
ObjectIdidentifier renders as a bean at the top level (writePOJO(id)) and as a hex string in references (id.toString()); the converter path had the same split, so not a regression, but the new comment makes it look intended. DomainClassJacksonModule.serializer()resolves the rendering supplier before checking whether the type is a domain class, so the first mapper use for any bean fixesgrailsApplication/proxyHandlerfor the mapper's lifetime; checking the type first removes the ordering sensitivity.HtmlSafeJsonWriter.close()is unreachable: both generator factories disableAUTO_CLOSE_TARGETandFLUSH_PASSED_TO_STREAM, so the spec case for a trailing<on close documents nothing that runs.append(Writable)andvalue(JSONElement)now materialise the value into aStringbeforewriteRawValue, where the old writer streamedwriteTo(writer); same output, one extra copy for large GSPWritables.formatKeybuilds aSerializationContextand aTokenBufferper non-String map key; fine for normal maps, measurable for maps with thousands of numeric keys.- Test gaps: a
@JsonTypeInfodomain class, a subclass seen through a superclass-typed property, a composite id,domain.jackson.enabled=false, circular references with domain instances on the mapper path,GString/StreamCharBufferescaping through the publicJSONWriter(only the package-privateHtmlSafeJsonWriterasserts</and U+2028),char[],Throwable,Errorsprecedence over the mapper, a record with Jackson annotations, a registerer with an explicit negative priority,JSON.use('deep')on the new marshaller set, and the legacy spec asserting that a@JsonValuetype or a module serializer is ignored under the switch.JsonMapperRenderingSpec:325sets the protectedJSON.writerfield directly; the pattern pre-exists in four other specs.
|
|
||
| Config grailsConfig = getGrailsConfig(); | ||
| ProxyHandler proxyHandler = getProxyHandler(); | ||
| boolean legacy = grailsConfig.getProperty(SETTING_CONVERTERS_JSON_LEGACY, Boolean.class, false); |
There was a problem hiding this comment.
This is the line that decides the sequence. As written, Grails 9 renders the new output for every application, and the Grails 8 output is an opt-out that is deprecated and warns from the release that introduces it (SETTING_CONVERTERS_JSON_LEGACY is forRemoval on line 85 from day one).
Please invert it for 9.x: default to the Grails 8 marshallers and layout, log this warning once when an application is on them (it already says what to do), and let applications opt into the mapper with the setting. Flip the default in Grails 10 and remove the Grails 8 path in 11, and move the "removed in Grails 10" statements with it (guide lines 115, 133, 137 and 160, the marshaller javadocs, and PrettyPrintJSONWriter/JSONParser). With that order the setting is a migration window rather than an escape hatch an application discovers after its clients break.
There was a problem hiding this comment.
For breaking default changes in a major, Grails 8 did what this PR does. The new behaviour became the default, and an application could restore the old one with a setting:
- Unconstrained GORM properties became nullable, restored with
grails.gorm.default.nullable: false(upgrade guide §27). - Native queries return
java.timetypes, restored withhibernate.query.native.prefer_jdbc_datetime_types: true"during a phased migration" (§26.5). - The
Acceptheader is honoured for browsers, restored by settinggrails.mime.disable.accept.header.userAgents(§30). defaultViewResolverand the welcome page are removed, restored withgrails.web.removeDefaultViewResolverBean: falseandgrails.web.removeWelcomePageMapping: false(§31).
The Grails 8 change closest to this one, the enum JSON shape, had no switch at all (§8: "There is no Grails 8 configuration switch that restores the verbose enum-object shape").
So I'd keep the new output as the 9.0 default, with grails.converters.json.legacy: true as the way back. That holds once the legacy-mode gaps in my review are fixed, so that the switch really gives Grails 8's text.
Where I agree is the timeline. None of the Grails 8 switches above was deprecated for removal in the release that added it. Making this one forRemoval and logging a warning from day one leaves less room than we gave there. I'd keep the setting without a deprecation in 9, or at least not name Grails 10 for its removal until we know how many applications depend on it.
There was a problem hiding this comment.
Thanks both. I've gone with matrei's reading of the precedent: the new output stays the 9.0 default, with grails.converters.json.legacy: true as the way back. On the timeline, I agree with both of you that day-one removal was wrong:
- The setting is no longer deprecated, logs no warning, and names no release that removes it (1b3a980).
- The "removed in Grails 10" statements are gone from the guide, the marshaller javadocs,
PrettyPrintJSONWriterandJSONParser. The classes the switch uses stay plain@Deprecated(since = "9.0"), withoutforRemoval.
The switch is now a complete way back, so it's safe to rely on:
- Domain classes render with the Grails 8 marshaller body (e41e7e4).
- It writes with a default
JsonMapper, so the application'sspring.jackson.*settings and modules can't leak into the text. - Map keys and
toString(false)behave as in Grails 8.
Grails8JsonRenderingSpec checks all of that against 8.0.x output.
There was a problem hiding this comment.
I'm still asking for the inversion, and I'm -1 until the default changes.
The Grails 8 precedent doesn't carry here. 9.0 is being accelerated, with few milestones, so the new output gets little pre-release exposure before it ships, and this change is not being backported to 8.1.x, so there is no release in which an application can opt into the mapper-backed output before it becomes the default. The first release that has the new output is also the one that turns it on for everyone. For a part of the framework this central, where the text goes to clients outside the application, that is changing a default, not offering a migration.
So for 9.x: default to the Grails 8 marshallers and layout, let an application opt into the mapper with the setting, and flip the default in 10. The switch being complete and undeprecated is good, and the inversion doesn't lose any of that work.
There was a problem hiding this comment.
Inverted in 6e9b680. In 9, grails.converters.json.legacy defaults to true, so the converter renders JSON as Grails 8 did. An application opts into the mapper with legacy: false, and Grails 10 makes that the default. The setting stays undeprecated, and nothing is logged.
- The initializer,
ConvertersConfigurationHolderand the configuration metadata default totrue. - The specs for the mapper path opt in explicitly.
Grails8JsonRenderingSpechas a feature for the default with no setting at all: every value, both pretty print paths and the 11 domain scenarios match the 8.0.x text.- The edits this PR had made to existing tests only because the default changed are reverted, so those files are identical to 9.0.x again:
JsonDateTimeRenderingSpec, the app1 and hibernate7/app1 date marshaller controllers and specs, andMonthBindingSpec's comment. - Section 1 of the guide and the REST guide now describe the mapper as an opt-in that Grails 10 makes the default.
This also corrects the cross-PR story I posted: in 9, the converter renders Grails 8's JSON by default, rather than writing through the mapper. With #16237 keeping respond() on the converter in 9, an application's JSON stays as Grails 8 rendered it unless it opts in, apart from what Grails 8 failed to render or rendered as invalid JSON. Both layers then flip in 10.
The 12 suites pass again: 3,769 tests, including the full app1 and hibernate7/app1 functional suites.
| Grails marshallers still render domain classes, beans, enums, collections, maps and arrays, and now records (as an | ||
| object of their components) and `Optional` (as its value), so the values they contain are rendered as any other value | ||
| is. A marshaller that an application registers, with `JSON.registerObjectMarshaller(...)` or an | ||
| `ObjectMarshallerRegisterer`, still takes precedence over the mapper for the types it supports. |
There was a problem hiding this comment.
This sentence, and the registerObjectMarshaller examples at lines 167-169, present a registered marshaller as the supported way to keep a Grails 8 format per type. #16237, also on 9.0.x, deprecates JSON.registerObjectMarshaller for removal and uses a registered marshaller as the trigger that keeps respond() on the legacy converter, and its guide says render value as JSON still uses the legacy converter, which stops being true once this PR merges. The two PRs also add two switches with different meanings, grails.converters.json.legacy here and grails.web.rendering.json.spring there, and after both merge the "legacy converter" in #16237 is this PR's mapper-backed converter unless both settings are on. Please settle one story across the two before either merges: one switch, one deprecation timeline, and registerObjectMarshaller either the migration tool or the thing being migrated away from.
There was a problem hiding this comment.
Agreed, the two PRs need one story before either merges. Here's what I propose. #16237 already matches most of it at its current head (c76fe76).
Two layers, one switch each, and each switch means one thing:
- What
grails.converters.JSONwrites (this PR). That coversrender ... as JSON, andrespond()whilerespond()uses the converter.- In 9 it writes through Boot's
JsonMapper, with Grails' marshallers on top. grails.converters.json.legacy: truegives Grails 8's text, byte for byte, for bothrenderandrespond.- The switch isn't deprecated in 9, and we don't name a removal release until we know how many applications use it.
- In 9 it writes through Boot's
- Which pipeline
respond()uses (Modernize MIME negotiation and serialization for Spring Boot 4.1 #16237), set bygrails.web.rendering.json.spring:- 9: the converter, by default.
- 10: Spring's message converters, by default.
- 11: the converter path for
respond()is removed.
So an application that wants exactly Grails 8's JSON in 9 sets grails.converters.json.legacy: true and nothing else. One that wants respond() to match a Spring Boot controller sets grails.web.rendering.json.spring: true. Neither switch changes what the other does.
registerObjectMarshaller is the migration tool, not the thing being migrated away from. Both PRs keep it supported and undeprecated, for the converter:
- Here, it's how to keep one type's Grails 8 format.
- In Modernize MIME negotiation and serialization for Spring Boot 4.1 #16237, it applies to
render ... as JSONand torespond()whilerespond()uses the converter. Registering one no longer movesrespond()to the converter: Modernize MIME negotiation and serialization for Spring Boot 4.1 #16237 dropped that trigger, and only the setting decides. - The Spring path is customized with Jackson serializers.
- Modernize MIME negotiation and serialization for Spring Boot 4.1 #16237 deprecates the named-configuration APIs (
JSON.use,createNamedConfig, …) for Grails 11, notregisterObjectMarshaller.
Whichever PR merges second updates its wording to match the other. If this one merges first, #16237 needs two changes:
- Its "the legacy converter" becomes "the JSON converter".
- Its §2 says that
respond()'s default output in 9 is this PR's §1, withgrails.converters.json.legacyfor Grails 8's text.
There was a problem hiding this comment.
Checked #16237 at 5ce132c against this: respond() stays on the converter in 9 and grails.web.rendering.json.spring flips in 10, the marshaller trigger is gone, and registerObjectMarshaller is not deprecated there either, so the two layers line up as you describe. Its guide still says render value as JSON "uses the legacy converter" and calls the converter "legacy" throughout; after this PR that converter writes through the mapper, so that wording needs the update you mention in whichever merges second.
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Reviewed at ebf5601. I left out everything already raised in jdaugherty's review and the earlier threads. On the default, I've replied in that thread. The comments below are new. Each one was reproduced with a probe spec against this head, except where a comment says otherwise.
Two of them affect grails.converters.json.legacy itself. Legacy mode still writes through a copy of the application's JsonMapper, so its generator settings and key serializers leak into what is meant to be Grails 8 text. Grails8JsonRenderingSpec only uses a default mapper, so it doesn't catch this. If the legacy path becomes the 9.x default, as requested, these become default-path regressions.
The domain module on Boot's JsonMapper also changes @RestController output beyond the annotation list already raised: @JsonValue, @JsonUnwrapped, the naming strategy, and subclasses of domain classes.
Smaller items:
ConvertersConfigurationHolder.setJsonMapper(JsonMapper)takes a JacksonJsonMapper, butgetJsonMapper()returnsJsonMapperSupport.JSON.getJsonMapper()also returnsJsonMapperSupport. Since all three are new 9.0 API, it would be good to make them symmetric, or name the gettersgetJsonMapperSupport(), before release.- With
spring.jackson.serialization.indent-output=true,JSON#toString(false)andgrails.converters.json.pretty.print: falsestill indent, because the generator inheritsINDENT_OUTPUTfrom the mapper. Either add.without(SerializationFeature.INDENT_OUTPUT)whenprettyPrintis false inJsonMapperSupport.createGenerator, or document thatspring.jacksonwins. - Performance: a domain instance now goes converter →
JsonMapperValue→DomainClassSerializer→ nested hand-off. Every property value then goes throughlookupObjectMarshallerandwriteNestedback into the converter. Hasrender list as JSONbeen measured for a few thousand instances against 9.0.x? It's the hottest path most apps have.
| ConvertersConfigurationHolder.setObservationRegistry( | ||
| applicationContext.getBeanProvider(ObservationRegistry.class).getIfAvailable(() -> ObservationRegistry.NOOP)); | ||
| // the application's JsonMapper, or a default one when there is none, or more than one and none is primary | ||
| ConvertersConfigurationHolder.setJsonMapper(applicationContext.getBeanProvider(JsonMapper.class).getIfUnique()); |
There was a problem hiding this comment.
The application's mapper is installed even when grails.converters.json.legacy is true. Legacy text is then written by a generator copied from it, so ordinary Boot write settings change the output that is meant to be Grails 8's. Legacy mode with a mapper that has INDENT_OUTPUT, WRITE_NUMBERS_AS_STRINGS and WRITE_BIGDECIMAL_AS_PLAIN enabled renders [n: 42, b: 1E+3] as:
{
"n" : "42",
"b" : "1000"
}
Grails 8 wrote {"n":42,"b":1E+3}. toString(false) is indented as well. These map to spring.jackson.serialization.indent-output, spring.jackson.json.write.* and spring.jackson.write.*, which an app may already have set for its @RestControllers. Could legacy mode use JsonMapperSupport.DEFAULT (or another fixed mapper) instead? A case with a non-default mapper in Grails8JsonRenderingSpec would cover it.
There was a problem hiding this comment.
Fixed in e41e7e4. With the switch on, the initializer installs a default JsonMapper rather than the application's. Grails8JsonRenderingSpec uses a mapper with:
INDENT_OUTPUT;WRITE_NUMBERS_AS_STRINGS;WRITE_BIGDECIMAL_AS_PLAIN;- a New York time zone;
- a module serializer.
With that mapper, every value, both pretty-print layouts and all 11 domain scenarios still match the 8.0.x text. toString(false) with pretty.print: true indents again, as 8.0.x ignored the argument.
| * @since 9.0 | ||
| */ | ||
| public String formatKey(Object key) { | ||
| return getJsonMapper().formatKey(key); |
There was a problem hiding this comment.
Map keys go through the application mapper's key serializers in legacy mode too. Grails 8 used JsonDateFormat.formatKey, which is toString() for everything except dates. With legacy on:
- a
Date(0)key, withspring.jackson.time-zone: America/New_York, renders"1969-12-31T19:00:00.000-05:00"; Grails 8 wrote"1970-01-01T00:00:00.000Z" - a
Classkey renders"java.lang.String"; Grails 8 wrote"class java.lang.String" - a key whose class has
@JsonValuerenders the@JsonValue; Grails 8 wrotetoString()
The Class and @JsonValue cases differ even with a default mapper. Branching here on ConvertersConfigurationHolder.isLegacyJson() to JsonDateFormat.formatKey would restore it. The Class key change in the new mode isn't in the upgrade guide's table either.
There was a problem hiding this comment.
Fixed in e41e7e4. In legacy mode JSON.formatKey uses JsonDateFormat.formatKey. One of the domain scenarios renders Class, Date and @JsonValue keys, and its expected text comes from 8.0.x: "class java.lang.String", the UTC instant, and the key's toString().
The Class key change in the new mode is now in the guide's map-key paragraph (1b644c8), along with @JsonValue keys, and ZonedDateTime/OffsetDateTime keys keeping their own offset.
| marshallers.add(domainClassMarshaller(grailsConfig, proxyHandler)); | ||
| // dates and times and the other single values the application's JsonMapper has a serializer for are written | ||
| // by the mapper, as Spring Boot writes them | ||
| marshallers.add(new org.grails.web.converters.marshaller.json.JsonMapperValueMarshaller()); |
There was a problem hiding this comment.
JsonMapperValueMarshaller sits ahead of SimpleEnumMarshaller. For an enum with a @JsonValue method, Jackson's serializer is a JsonValueSerializer, not an EnumSerializer, so rendersValue is true and the mapper renders it. enum Status { ACTIVE('A'); @JsonValue String getCode() { code } } renders "A", where Grails 8 and legacy render "ACTIVE".
Posting that back binds null without an error, because SimpleDataBinder.convertStringToEnum swallows the IllegalArgumentException. That contradicts upgrading90x.adoc:56 and defaultRenderers.adoc:93 ("Enums render by name() … so that the JSON binds back"). The @JsonValue row in the table says Grails 8 wrote "bean objects", but for an enum it wrote name().
Either let SimpleEnumMarshaller claim enums (other than Month) first, or give enums their own row and drop the binds-back sentence.
There was a problem hiding this comment.
Fixed in 512e8d9 by letting SimpleEnumMarshaller claim them. rendersValue is false for an enum whose serializer is a JsonValueSerializer, so your Status.ACTIVE renders "ACTIVE", as a map value and as a single value, and binds back.
Month still goes to the mapper, since its serializer isn't a JsonValueSerializer. The guide's "Enums render by name()" sentence now includes enums with a @JsonValue, and the @JsonValue row in the table says "other than an enum".
There was a problem hiding this comment.
This still misses an enum constant with a body. For ACTIVE('A') { … }, the runtime class is the constant's anonymous subclass. Jackson resolves a JsonValueSerializer for it, so the new check in rendersValue returns false, which is right. But SimpleEnumMarshaller.supports uses object.getClass().isEnum(), which is false for that subclass. It falls through to GroovyBeanMarshaller: [s: BodyStatus.ACTIVE] renders {"s":{"code":"A","declaringClass":"…BodyStatus","name":"ACTIVE","ordinal":0}}. At ebf5601 it rendered "A", and the guide promises "ACTIVE". Constants without a body are fine.
Using object instanceof Enum in supports for the new mode would fix it, since name() is already found through the superclass. Legacy can keep isEnum() to stay identical to Grails 8, where a body constant also rendered as a bean.
There was a problem hiding this comment.
Fixed in 26f1dda. SimpleEnumMarshaller has a new SimpleEnumMarshaller(boolean constantsWithBodies) constructor:
- The new mode registers
new SimpleEnumMarshaller(true), whosesupportsisobject instanceof Enum. - Legacy keeps
new SimpleEnumMarshaller()andisEnum(), so a body constant still renders as a bean there, as in 8.0.x.
The spec renders a body constant with a @JsonValue ("ACTIVE") and one without ("CIRCLE"). The guide's table has a row for body constants.
| public ValueSerializer<?> findSerializer(SerializationConfig config, JavaType type, | ||
| BeanDescription.Supplier beanDescRef, JsonFormat.Value formatOverrides) { | ||
| DomainClassSerializer domainClassSerializer = serializer(); | ||
| return domainClassSerializer.getRendering().isDomainClass(type.getRawClass()) ? domainClassSerializer : null; |
There was a problem hiding this comment.
Jackson asks module Serializers before it looks at @JsonValue and JacksonSerializable. In BeanSerializerFactory._createSerializer2, findSerializerByAnnotations comes after the module lookup, and only a class-level @JsonSerialize is checked earlier. So a domain class that uses either of them loses it on the application's mapper. @Entity class Code { String code; @JsonValue String jsonValue() { code } } returned from a @RestController renders "X1" without the module and {"id":2,"code":"X1"} with it. Returning null here when beanDescRef.get().findJsonValueAccessor() != null, or when the type implements JacksonSerializable, would leave those to Jackson. Otherwise they should go in the guide next to @JsonSerialize.
There was a problem hiding this comment.
Fixed in f520e7d. findSerializer returns null for a type that implements JacksonSerializable, or whose bean description has a @JsonValue accessor. Jackson then applies those as it does for any class. The spec checks your Code case: it renders "X1".
| * | ||
| * @since 9.0 | ||
| */ | ||
| public final class DomainClassSerializer extends StdSerializer<Object> { |
There was a problem hiding this comment.
This doesn't override unwrappingSerializer(NameTransformer) or isUnwrappingSerializer(), so @JsonUnwrapped on a property holding a domain instance stops unwrapping. UnwrappingBeanPropertyWriter writes the name and nests the object. class Wrapper { @JsonUnwrapped Book book; String note } renders {"bookTitle":"G","id":1,"version":null,"note":"n"} without the module and {"book":{"id":1,"bookTitle":"G"},"note":"n"} with it. I haven't checked Spring HATEOAS, but EntityModel<T> holds its content as @JsonUnwrapped, so EntityModel<Book> is likely to change shape too. An unwrapping variant would fix it: it skips the start and end of the object and passes property names through the NameTransformer.
There was a problem hiding this comment.
Fixed in f520e7d. DomainClassSerializer overrides unwrappingSerializer(NameTransformer) and isUnwrappingSerializer(). The unwrapping variant skips the start and end of the object and passes every name through the transformer.
The spec has @JsonUnwrapped with and without a prefix. Unwrapped properties land where the property is, in the order the mapper writes the enclosing bean.
I haven't checked Spring HATEOAS's EntityModel. Since its content is a plain @JsonUnwrapped property, it should take the same path.
| !rendering.includes(object, property.getName())) { | ||
| continue; | ||
| } | ||
| gen.writeName(property.getName()); |
There was a problem hiding this comment.
Property names (and class, the id name and version above) are written as they are, so the mapper's PropertyNamingStrategy no longer applies to domain classes on the mapper path. With spring.jackson.property-naming-strategy: SNAKE_CASE, a domain class renders {"book_title":"G","id":1,"version":null} without the module and {"id":1,"bookTitle":"G"} with it. The same goes for @JsonNaming and spring.jackson.mapper.sort-properties-alphabetically. This is a mapper setting rather than an annotation, so it isn't covered by the guide thread. The converter writes through a copy of the same mapper, so applying the strategy in serialize() only, not in the converter's value() path, would keep render as JSON unchanged.
There was a problem hiding this comment.
Fixed in f520e7d, as you suggested, in serialize() only. On the mapper path, property names come from Jackson's own introspection of the class (BeanPropertyDefinition.getName(), cached per type), so the naming strategy, @JsonNaming and @JsonProperty("rename") apply. The converter's value() path writes the names as they are, so render as JSON is unchanged. The spec covers SNAKE_CASE, @JsonNaming(KebabCaseStrategy) and a renamed property, and checks the converter alongside.
I left sorting out on purpose. Jackson 3 enables SORT_PROPERTIES_ALPHABETICALLY by default, so honouring it would reorder every domain class on the mapper away from the converter's order (id, version, then the persistent properties), and the module exists to match that order. The spec pins this with and without the feature, and the guide says properties keep the converter's order.
There was a problem hiding this comment.
The instance's own names follow the strategy now, but references don't do so consistently. Under UPPER_CAMEL_CASE, an instance writes {"Id":3,"Version":4,"Name":"top"}. An association reference writes "Shelf":{"id":3}, because it uses Names.AS_THEY_ARE. A cycle reference writes "Partner":{"Id":1}, with the instance's names. So the same reference shape is {"id":…} or {"Id":…} depending on how it was reached. Likewise, with @JsonProperty('key') Long id, the instance writes "key" but every reference to it writes "id". Either rule is fine, raw names for references or the referenced entity's mapped id name, but the association and cycle paths should use the same one.
There was a problem hiding this comment.
Fixed in 4a20e8d, with the referenced entity's mapped id name. On the mapper, a reference is named as the referenced class names its properties, on both the association and the cycle path. Under UPPER_CAMEL_CASE, both write {"Id":…}, and since the id goes through the same name map, @JsonProperty('key') on the id names it key in references too. The converter path still writes "id".
The spec covers UPPER_CAMEL_CASE through a to-one association, a list, a map association and a deep cycle.
| return false; | ||
| } | ||
| for (Class<?> current = type; current != null && current != Object.class; current = current.getSuperclass()) { | ||
| if (grailsApplication.isArtefactOfType(DomainClassArtefactHandler.TYPE, |
There was a problem hiding this comment.
isDomainClass walks superclasses, but DomainClassMarshaller.supports only matches the exact artefact name, so a non-artefact subclass of a domain class falls between them. That covers class BookSub extends Book, a Spock Spy(Book) or an anonymous subclass. DomainClassSerializer isn't in rendersValue's exclusion list, so JsonMapperValueMarshaller claims what DomainClassMarshaller declined. Both jsonMapper.writeValueAsString(sub) and new JSON(sub).toString() then fail with IllegalStateException: No GORM implementation configured for class [...BookSub]. Before this PR, the converter rendered it with GroovyBeanMarshaller and Boot's mapper as a bean. Matching only a name that trims to an artefact (the class itself or its proxy classes) would leave other subclasses to the bean serializer.
There was a problem hiding this comment.
Fixed in f520e7d. isDomainClass now matches the class itself, after trimProxySuffix, or a proxy class of a domain class: a GORM EntityProxy, or a Hibernate HibernateProxy, checked by interface name. A plain subclass, a Spock Spy or an anonymous subclass is left to the bean serializer.
The spec renders a VolumeSubclass through the mapper and the converter without the IllegalStateException. Its stand-in proxy now implements EntityProxy, as GORM's proxies do.
…Jackson module opt-in - grails.converters.json.legacy is no longer deprecated, does not log a warning, and names no release that removes it. - The JSON marshallers it registers, PrettyPrintJSONWriter and JSONParser stay deprecated, without a removal release. - grails.converters.json.domain.jackson.enabled defaults to false, so Spring Boot's JsonMapper renders domain classes as Jackson beans, as in Grails 8, unless an application opts in. It no longer depends on grails.converters.json.legacy. - Both settings have configuration metadata.
…ith grails.converters.json.legacy - DomainClassMarshaller renders an instance with the Grails 8 marshaller body when the switch is on, so entities with associations, proxies, embedded values, includes and excludes, deep, and the version and class name render as in Grails 8. A to-many Map association is one object of its entries, where Grails 8 failed unless it had exactly one. - The converter writes with a default JsonMapper when the switch is on, so the application's Jackson settings and modules do not change the text. - Map keys are formatted as Grails 8 formatted them: a date as a UTC instant and any other key with its toString(). - toString(false) indents when pretty printing is configured, as Grails 8 ignored the argument. - Grails8JsonRenderingSpec compares 11 domain class scenarios with text rendered by Grails 8.0.x (232bb34), and renders every value with a JsonMapper that indents, writes numbers as strings, writes plain BigDecimals, uses another time zone and has a module serializer.
…nums with a @jsonvalue by name - JSONWriter writes a Number it has no dedicated generator call for, such as an AtomicInteger, as its text, as Jackson's NumberSerializer does, rather than through the mapper, where an active nested write offered it back to a marshaller that wrote it to the writer again, without end. - JSON that is not pretty printed is not indented when the application's JsonMapper enables INDENT_OUTPUT. - An enum with a @jsonvalue renders by name(), as every enum does, so that it binds back. - RecordMarshaller reports an inaccessible record component as a ConverterException, and its javadoc no longer claims Jackson's rendering. - The getters that return a JsonMapperSupport are named getJsonMapperSupport(), next to setJsonMapper(JsonMapper).
… and with the mapper's names For an application that enables grails.converters.json.domain.jackson.enabled: - DomainClassSerializer implements serializeWithType, so a domain class with @JsonTypeInfo, or a property typed with one, is written with its type id rather than failing. - It provides an unwrapping serializer, so a @JsonUnwrapped property holding a domain instance writes its properties into the enclosing object. - Written by the mapper, property names are those Jackson gives the class's properties, so the naming strategy, @JsonNaming and @JsonProperty names apply. The converter writes the names as they are, and both keep the converter's property order. - The module leaves a domain class with a @jsonvalue, or that implements JacksonSerializable, to Jackson. - Only a domain class, or a GORM or Hibernate proxy class of one, is written as a domain class; another subclass of one is a Jackson bean. - A rendering supplied before the application exists is not kept, so the first type the mapper meets no longer fixes it for the mapper's lifetime. - The comment on a reference's id says how it matches Grails 8.
A Path is an Iterable of its names, so it was kept from the mapper as a container, and the bean marshallers failed to render it, as in Grails 8. It now renders as Jackson writes it, as its URI. JsonMapperRenderingSpec compares char[], File, Path, Pattern, Charset, InetAddress, ByteBuffer, a Class map key and an OffsetDateTime map key with the JsonMapper's text.
… review found Upgrade guide and REST guide: - grails.converters.json.legacy is not deprecated, and renders domain classes, map keys and the text as Grails 8 did, whatever the application's Jackson settings. - The domain module is opt-in, and the guides list exactly what it applies on the mapper and what it does not. - The table adds char[], File, Path, ByteBuffer, Pattern, Charset and InetAddress; map keys say how a Class key, a @jsonvalue key and an OffsetDateTime or ZonedDateTime key render; toString(false) and indent-output are described; an enum with a @jsonvalue renders by name. - No statement names a release that removes the legacy path. Tests: a registerer with an explicit negative priority, the validation errors marshaller over a module serializer for Errors, a Throwable, a record with Jackson annotations, JSON.use('deep') with domain classes, a composite key, and GString and StreamCharBuffer escaping through JSONWriter.
|
Thanks @jdaugherty and @matrei. I've replied in each thread. Here are the review-body items that have no thread, in six commits from 1b3a980 to 1b644c8. From @jdaugherty's minor items
From @matrei's smaller items
Verification: fresh runs, 0 failures: 3,766 tests in total, with no cache hits:
|
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Thanks, this addresses nearly everything. I re-checked the six commits at 1b644c8 with probe specs. These hold as described:
- The legacy switch writes with the default mapper, Grails 8 key formatting, the Grails 8 domain body and
toString(false). - The domain module is opt-in, and
@JsonValue/JacksonSerializableare left to Jackson. @JsonTypeInfoworks withPROPERTY,WRAPPER_OBJECTandWRAPPER_ARRAY, including cycle references and proxies.- Unwrapping works, with chained prefixes.
- Proxy and subclass detection, the metadata entries, and the compact generator are all correct.
The sort-order call is right too: SORT_PROPERTIES_ALPHABETICALLY is true by default in jackson-databind 3.x, and Boot only turns it off with use-jackson2-defaults.
Two of the fixes introduced regressions, which are in the threads they came from: an enum constant with a body, and a Number with a module serializer. Three new inline comments below cover the domain serializer.
Smaller items:
@JsonTypeInfo(include = EXISTING_PROPERTY)only works when the type property is persistent. With a transient or computed getter (static transients = ['type'],String getType()), the module writes no type id, where plain Jackson writes"type":…. A client that deserializes polymorphically then fails. Worth a line in section 2 of the guide.- The
Pathexclusion is a point fix. OtherIterablevalue types are still kept from the mapper before their serializer is looked at. Jackson 3'sJsonNodeisIterable, sonew JSON([n: mapper.readTree('{"a":1}')])renders{"n":{"array":false,…,"nodeType":"OBJECT",…}}. 8.0.x was just as broken, so this isn't a regression. But dropping theIterableclause fromisContainerwould let the serializer check decide: Jackson'sIterableSerializerandCollectionSerializerareStdContainerSerializers, and bean-like iterables get aBeanSerializerBase.PathandJsonNodewould then reach the mapper with no special case. - The guide doesn't say what
legacy: truewithdomain.jackson.enabled: truedoes. The converter is Grails 8, and Boot's mapper uses the module. That's sensible, but worth a sentence in the legacy section.
| Names names = names(object.getClass(), ctxt); | ||
| Set<Object> instances = renderingInstances(ctxt); | ||
| if (!instances.add(object)) { | ||
| writeReferenceProperties(object, entity(object, rendering), gen, rendering, names); |
There was a problem hiding this comment.
A custom DomainClassRendering.writeReference is no longer called for a cycle reference. At ebf5601 the cycle path went through writeReference(object, …), which asked rendering.writeReference(...) first. Now writeContents writes the reference properties directly, after serialize/serializeWithType has already written the start of the object or the type prefix, so the hook can't take over there. On a deep cycle with a rendering whose writeReference throws, the output is {"id":1,"name":"first","partner":{"id":2,"name":"second","partner":{"id":1}}} and the hook is never called. The shallow association path still calls it. Detecting the cycle in serialize/serializeWithType before anything is written would let rendering.writeReference(...) decide again, as it does for associations.
There was a problem hiding this comment.
Fixed in 4a20e8d. serialize and serializeWithType now detect the cycle before anything is written, and a cycle reference goes through writeReference(...). So rendering.writeReference(...) decides first, as it does for associations. On the typed path it's asked before the type prefix is written.
The spec renders your deep Partner cycle with a rendering whose writeReference records what it's offered, and checks that the hook gets the cycle reference.
Inside a @JsonUnwrapped property, a cycle still writes the reference's properties directly. The hook writes a whole value, and there's no place for one in the enclosing object.
|
|
||
| @Override | ||
| public void serializeWithType(Object value, JsonGenerator gen, SerializationContext ctxt, TypeSerializer typeSer) { | ||
| if (unwrapper != null) { |
There was a problem hiding this comment.
For the unwrapping variant this drops the type id silently. @JsonUnwrapped Dog dog with @JsonTypeInfo(use = NAME) on Animal renders {"id":2,"name":"Rex","note":"n"}. Jackson's own UnwrappingBeanSerializer.serializeWithType throws InvalidDefinitionException here, because SerializationFeature.FAIL_ON_UNWRAPPED_TYPE_IDENTIFIERS is on by default. I checked that with an equivalent plain bean. Matching it would be if (ctxt.isEnabled(SerializationFeature.FAIL_ON_UNWRAPPED_TYPE_IDENTIFIERS)) ctxt.reportBadDefinition(handledType(), …) before falling through to serialize.
There was a problem hiding this comment.
Fixed in 4a20e8d. With FAIL_ON_UNWRAPPED_TYPE_IDENTIFIERS on, which is the default, the unwrapping variant's serializeWithType now calls reportBadDefinition with UnwrappingBeanSerializer's message. With the feature disabled, it writes the properties without the type id. The spec covers both, with @JsonUnwrapped Dog dog.
…nd Iterable values as the mapper does - JSON.value writes only the JDK number types directly. Any other Number is rendered by its marshaller, so a Number subclass that a module serializes renders as the JsonMapper renders it, rather than as its toString(). With grails.converters.json.legacy every Number is written directly, as in Grails 8. - SimpleEnumMarshaller(true) marshals an enum constant with a body, whose class is a subclass of its enum, by name, as any other enum. The converter uses it unless grails.converters.json.legacy is set, where a body constant renders as a bean, as in Grails 8. - An Iterable is no longer kept from the mapper before its serializer is looked at: collections and bean-like iterables still go to the Grails marshallers, and a Path or a JsonNode renders as the mapper writes it.
…and name references as instances - Rendering deep, DomainClassSerializer detects an instance already being written before it writes anything, and writes the reference through DomainClassRendering.writeReference, as it does for associations. - An unwrapping serializer asked for a type id fails as Jackson's own unwrapping does, unless FAIL_ON_UNWRAPPED_TYPE_IDENTIFIERS is disabled. - On the mapper, a reference is named as the referenced class names its properties, through an association and through a cycle alike.
… ids and legacy with the domain module
|
@matrei Thanks for re-checking, and for confirming the sort order. The five threads have replies. Your smaller items are done:
Verification: 3,772 tests, 0 failures, all run fresh, across the same 12 suites as before, including the full |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Re-reviewed at 27a86d8: the nine commits from 1b3a980 to 5536958; the merges after them only bring in 9.0.x. The implementation asks are closed, and I've replied in each thread with what I checked. Beyond the threads:
DomainClassMarshaller.marshalObjectAsGrails8is the 8.0.xmarshalObjectbody line for line, apart from the documented change to a to-manyMapassociation (one object of its entries, identical text for one entry) and an unused local.- I ran
Grails8DomainRenderingon 8.0.x (232bb34) in a fresh checkout, in UTC: the 11 scenarios reproduce theGRAILS_8_DOMAINtext byte for byte. With that, the switch covers domain classes, map keys andtoString(false)as the guide says, and the spec holds against a mapper that indents, writes numbers as strings, uses another time zone and has a module serializer. - matrei's threads: the enum constant with a body,
Money extends Number, the cycle hook, the unwrapped type id and the reference naming behave as described.DomainClassJacksonModulecreates its serializers per mapper insetupModule, so the per-type name cache cannot carry one mapper's naming strategy into another; I probed two mappers sharing one module instance. grails-web-common(144) andgrails-converters(412) pass locally from a clean run, and CI is green at this head.
One cosmetic note that needs no change: on the mapper path id and version follow the naming strategy (Id, Version under UPPER_CAMEL_CASE) while the class property written with include.class stays class, as Jackson does not introspect it.
On the 9.x default I am -1 until it is inverted; see that thread.
| Grails marshallers still render domain classes, beans, enums, collections, maps and arrays, and now records (as an | ||
| object of their components) and `Optional` (as its value), so the values they contain are rendered as any other value | ||
| is. A marshaller that an application registers, with `JSON.registerObjectMarshaller(...)` or an | ||
| `ObjectMarshallerRegisterer`, still takes precedence over the mapper for the types it supports. |
There was a problem hiding this comment.
Checked #16237 at 5ce132c against this: respond() stays on the converter in 9 and grails.web.rendering.json.spring flips in 10, the marshaller trigger is gone, and registerObjectMarshaller is not deprecated there either, so the two layers line up as you describe. Its guide still says render value as JSON "uses the legacy converter" and calls the converter "legacy" throughout; after this PR that converter writes through the mapper, so that wording needs the update you mention in whichever merges second.
|
|
||
| Config grailsConfig = getGrailsConfig(); | ||
| ProxyHandler proxyHandler = getProxyHandler(); | ||
| boolean legacy = grailsConfig.getProperty(SETTING_CONVERTERS_JSON_LEGACY, Boolean.class, false); |
There was a problem hiding this comment.
I'm still asking for the inversion, and I'm -1 until the default changes.
The Grails 8 precedent doesn't carry here. 9.0 is being accelerated, with few milestones, so the new output gets little pre-release exposure before it ships, and this change is not being backported to 8.1.x, so there is no release in which an application can opt into the mapper-backed output before it becomes the default. The first release that has the new output is also the one that turns it on for everyone. For a part of the framework this central, where the text goes to clients outside the application, that is changing a default, not offering a migration.
So for 9.x: default to the Grails 8 marshallers and layout, let an application opt into the mapper with the setting, and flip the default in 10. The switch being complete and undeprecated is good, and the inversion doesn't lose any of that work.
…er as an opt-in grails.converters.json.legacy now defaults to true: the JSON converter renders JSON as Grails 8 did unless an application sets it to false to write through the JsonMapper, which Grails 10 makes the default. The setting stays undeprecated, and nothing is logged. - The initializer, ConvertersConfigurationHolder and the configuration metadata default to the Grails 8 rendering. - The specs of the JsonMapper path opt in, and Grails8JsonRenderingSpec checks that, without the setting, every value, both pretty print paths and the 11 domain class scenarios render as Grails 8.0.x rendered them. - Tests this change had edited only because the default had changed are back to their 9.0.x versions: JsonDateTimeRenderingSpec, the app1 and hibernate7 app1 date marshaller controllers and specs, and the comment in MonthBindingSpec. - The upgrade guide and the REST guide describe the JsonMapper as an opt-in that Grails 10 makes the default.
|
inversion done, although I agree with @matrei I think it would be better the other way since it is so easy to flip legacy behavior back on via |
This comment has been minimized.
This comment has been minimized.
This branch has not been deployed
4 participants
Description
grails.converters.JSONcan write JSON through the application's JacksonJsonMapper, the one Spring Boot auto-configures, and render domain classes with a Jackson serializer, which an application can also register with that mapper. With it, a Grails application and a Spring Boot application return the same JSON for the same values, andspring.jackson.*applies torender ... as JSON.In Grails 9 this is an opt-in:
grails.converters.json.legacydefaults totrue, and an application turns the mapper on withlegacy: false. Grails 10 makesfalsethe default. With #16237,respond()also stays on the converter in 9, so in 9 an application's JSON stays as Grails 8 rendered it unless it opts in, apart from what Grails 8 failed to render or rendered as invalid JSON (a single value,NaN), and both layers flip in 10.Parsing does not change:
JSON.parse,request.JSONandnew JSONObject(String)still parse with Grails'JSONTokener, with the same lenient syntax, number types andMap/Listcontainers.By default,
grails.converters.json.legacy: truerenders the text Grails 8 rendered, including domain classes and map keys, whatever the application's Jackson settings, except where Grails 8 failed or wrote invalid JSON. The setting is not deprecated.Usage
With the mapper turned on, Jackson configuration applies to the converter:
With
grails.converters.json.domain.jackson.enabled: true, the application'sJsonMapperrenders a domain class as the converter does, sojsonMapper.writeValueAsString(book), or a Spring MVC controller returningbook, writes the following. By default, the mapper renders domain classes as Jackson beans, as in Grails 8.{"id":1,"title":"Grails","publisher":{"id":3},"authors":[{"id":1},{"id":2}]}A Jackson module registered with the application (a
JacksonModulebean or a@JacksonComponent) renders its types inrender ... as JSON. The values its serializer writes are rendered by the converter, so registered marshallers still apply to them. A marshaller registered withJSON.registerObjectMarshalleror anObjectMarshallerRegistererstill takes precedence over the mapper:With the mapper turned on, to keep the Grails 8 format of a few types only:
Output with
legacy: false, compared with Grails 8Grails 8's output is also the 9.x default. Measured against text rendered by Grails 8.0.x for the same values. They are listed in the new 9.0 upgrade guide.
OffsetTime"03:00:00-03:00","03:00:00.5+05:30""03:00-03:00","03:00:00.500+05:30"Month"SEPTEMBER"9(binds back)Locale"zh_TW_#Hant""zh-Hant-TW"char{}"c"byte[][1,2,3]"AQID"UUID,URI,@JsonValuetypes other than enumsPattern,Charset,InetAddressFile,Path,ByteBuffer"/tmp/a.txt","file:///tmp/a.txt","AQID"char[][{},{}]"ab"JsonNodeClassmap key"class java.lang.String""java.lang.String"Optional{"empty":false,"present":true}nullNaNNaN(invalid JSON)"NaN"toString(false)withpretty.print: truerender Role.HEAD as JSON)ConverterException"HEAD"@JsonIgnoreor write-onlyDates, times, records, beans, collections, maps and domain classes through the converter render as before. Enums still render by
name(), and strings are still escaped for an HTML<script>element (</as<\u002f).Design
JSONWriterwrites through a JacksonJsonGenerator, and keeps its public methods and state machine, so existing marshallers andPathCapturingJSONWriterWrapperwork unchanged.JsonMapperValueMarshallerrenders a value with the mapper when the mapper has a dedicated serializer for a single value of its type (dates and times,Month,UUID,Locale,byte[],@JsonValuetypes other than enums, module types). Containers, beans, enums and domain classes stay with Grails marshallers.JSONWriter.writeNestedwrites such a value and returns the writer to its state. The application's own mapper is not changed.DomainClassSerializer(inorg.grails.web.converters.jackson) renders an instance asDomainClassMarshallerdid: id, version and class name when included, persistent properties, and associations as{"id":…}references or, deep, in full, with proxies unwrapped and references to proxies written from their identifier. Withgrails.converters.json.domain.jackson.enabled: true,DomainClassJacksonModuleis registered as thedomainClassJacksonModulebean, which Spring Boot adds to itsJsonMapper. On the mapper the serializer also applies the mapper's naming strategy,@JsonNamingand@JsonPropertynames, writes@JsonTypeInfotype ids and unwraps@JsonUnwrappedproperties. It leaves a domain class with a@JsonValueorJacksonSerializable, and a subclass that is neither a domain class nor a GORM or Hibernate proxy class, to Jackson.DomainClassMarshallerandDeepDomainClassMarshallerhand instances to the serializer with the converter's includes and excludes, and their protected methods (includesProperty,excludesProperty,extractValue,asShortObject,isRenderDomainClassRelations) still apply.RecordMarshallerandOptionalMarshallerrender a record as an object of its components (Jackson annotations on the components are not applied) andOptionalas its value, with their contents rendered by the converter.HtmlSafeJsonWriterkeeps Grails'</, U+2028 and U+2029 escaping for everything the generator writes.DomainClassMarshallerruns its Grails 8 body.grails.converters.json.pretty.printand fromtoString(true)andtoString(false).JsonMapper, so the application's Jackson settings and modules don't change the text.Grails8JsonRenderingSpeccompares it with Grails 8.0.x output (232bb34) for 111 values, both pretty print paths and 11 domain class scenarios, both with a default mapper and with one that indents, writes numbers as strings, uses another time zone and has a module serializer.Deprecated
Deprecated without a removal release, and still usable:
DateMarshaller,CalendarMarshaller,ToStringBeanMarshaller,ByteArrayMarshaller,InstantMarshaller,LocalDateMarshaller,LocalDateTimeMarshaller,OffsetDateTimeMarshallerandZonedDateTimeMarshaller.PrettyPrintJSONWriter, which still indents as Grails 8 did.org.grails.web.json.parser.JSONParser, which Grails does not use.Unchanged
JSON.parse,request.JSONandJSONObject/JSONArray(JsonParsingSpecpins their syntax, number types and containers), JSON views, the XML converter and the HAL renderer.Fixed
PathCapturingJSONWriterWrapper(circular.reference.behaviour: PATH) threw on an object with anullproperty, gave wrong paths after a number property, and failed on a single value.Mapfailed to render; it renders as an object of references.JsonMapperSupportno longer uses Jackson's test-onlyObjectMapper._serializationContext().Performance
render list as JSONfor 3,000 domain instances (528 KB of JSON). Each figure is the median of 60 renders after 40 warm-up renders, in separate JVMs:legacy: falseLimitations
@JsonSerialize(using = …)orconverteris written by that serializer even inside a module serializer, because Jackson builds class-annotated serializers without serializer modifiers. A Grails marshaller registered for such a type applies everywhere else.@JsonIgnorePropertiesapplies to the application'sJsonMapperonly, with the domain module enabled. GORM adds one to every domain class naming the version andtenantId, so the converter keeps rendering those as before.