codeconsole commented on PR #16414:
URL: https://github.com/apache/grails-core/pull/16414#issuecomment-6049818183
Thanks @jdaugherty and @matrei. I've replied in each thread. Here are the
review-body items that have no thread, in six commits from 1b3a980f0d to
1b644c8973.
**From @jdaugherty's minor items**
- **`toString(false)` with `pretty.print: true`:** in the new mode it writes
compact JSON, and the guide now says so. With `grails.converters.json.legacy`
it indents, as 8.0.x ignored the argument, and `Grails8JsonRenderingSpec` pins
both.
- **`JsonMapperValueMarshaller` javadoc:** it now says the marshaller sits
after `ProxyUnwrappingMarshaller` and the domain class marshaller.
- **`isLegacyJson()`/`setLegacyJson()`:** the setting is no longer
deprecated, so these stay as plain 9.0 API.
- **Table:**
- `char[]` added.
- `Pattern`, `Charset` and `InetAddress` added: Grails 8 wrote bean
objects, the mapper writes strings.
- `File`, `Path` and `ByteBuffer` added. Grails 8 failed to render all
three, which I confirmed on 8.0.x.
- `Path` also failed on this branch, because it's an `Iterable` and was
kept away from the mapper. It now renders as Jackson writes it (18156ce6d0).
- Each new value is in `JsonMapperRenderingSpec`'s comparison with the
`JsonMapper`'s text.
- The map-key paragraph now limits the time-zone statement to
`Date`/`Calendar` keys, and says a `ZonedDateTime`/`OffsetDateTime` key keeps
its own offset.
- **ObjectId:** the comment on a reference's id now says it reproduces
Grails 8's split, rather than suggesting the split is intended.
- **`DomainClassJacksonModule.serializer()` ordering:** a rendering supplied
before the application exists is no longer cached. So the first type the mapper
meets can't fix an application-less rendering for the mapper's lifetime.
- I tried checking the type first, against GORM's `GormEntity`, but not
every domain class implements it (the converter specs' `@Entity` classes don't).
- **`HtmlSafeJsonWriter.close()`:** I kept it. The class is a `Writer`, and
closing it shouldn't drop a pending `<`, even though nothing closes it today.
The spec case documents that contract.
- **Two copies and `formatKey`:** `append(Writable)`/`value(JSONElement)`
copy the text once, and `formatKey` builds a context per non-`String` key.
- Jackson's raw-value API needs a `String`, and a fast path for numeric
keys would bypass a key serializer an application registers. So I've left both
as they are.
- **Test gaps:**
- Added: `@JsonTypeInfo`, a subclass through a superclass-typed property,
a composite key, `domain.jackson.enabled` default and `false`, `char[]`,
`Throwable`, `Errors` over a module serializer, a record with Jackson
annotations, a registerer with priority -100, `JSON.use('deep')` with domain
classes, and the legacy spec with a `@JsonValue` type and a module serializer.
- `GString`/`StreamCharBuffer` through `JSONWriter` showed they're escaped
by Grails' JavaScript encoder, `</` as `<\/`, exactly as `JSONWriter` quoted
them in Grails 8. A `String` gets `</`. Both are safe in a `<script>`, the test
pins both, and the guide mentions it.
**From @matrei's smaller items**
- **API symmetry:** the getters that return a `JsonMapperSupport` are now
`getJsonMapperSupport()` on `ConvertersConfigurationHolder` and `JSON`, next to
`setJsonMapper(JsonMapper)`. `JSON`'s protected field is renamed to match.
- **`indent-output`:** done as you suggested. The compact generator is
created `.without(SerializationFeature.INDENT_OUTPUT)`, so `toString(false)`
and `pretty.print: false` aren't indented whatever the mapper does. The guide
mentions it.
- **Performance:** I measured `render list as JSON` for 3,000 domain
instances (properties, a reference and a to-many association; 528 KB of JSON)
in the converters test JVM. Each figure is the median of 60 renders after 40
warm-up renders, in separate JVMs:
| | Median |
|---|---|
| 9.0.x (f5bbe69e75) | 4.6 ms, twice; a third run was disturbed at 11.3 ms
|
| This branch | 3.8, 4.0 and 4.2 ms |
| This branch, legacy | 3.5 ms |
All produce the same 527,752 characters. The nested hand-off doesn't slow
this path; the generator's buffered writing more than pays for it.
**Verification:** fresh runs, 0 failures:
3,766 tests in total, with no cache hits:
- `grails-web-common` 144 and `grails-converters` 406;
- `grails-rest-transforms` 47, `grails-controllers` 214, `grails-views-gson`
267, `grails-databinding` 49 and `grails-web-databinding` 67;
- `grails-test-suite-web` 442, `grails-test-suite-uber` 624 and
`grails-test-suite-persistence` 106;
- the full `app1` (711) and `hibernate7/app1` (689) functional suites.
`codeStyle` passes, and the guide builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
