borinquenkid commented on PR #16414:
URL: https://github.com/apache/grails-core/pull/16414#issuecomment-5882291941
Reviewed at head `95066b7b83`. I compiled it and reran the claimed test
counts myself (`grails-web-common` 113, `grails-converters` 205,
`grails-views-gson` 185 — all green, matching the PR body), then tried to break
the design rather than just confirm it. Two findings.
### 1. `formatKey()` depends on a Jackson method documented as test-only
`JsonMapperSupport.formatKey()` calls `mapper._serializationContext()`. In
Jackson 3.1.6's own source (`ObjectMapper.java`):
```java
// NOTE: only public to allow for testing
public SerializationContextExt _serializationContext() {
```
and `SerializationContextExt`'s class Javadoc: "adds methods needed by
`ObjectMapper` ... but that are not to be exposed as general context during
serialization." That's Jackson's own maintainers saying this isn't meant to be
built on. It works today, but Jackson doesn't consider changing or removing it
a breaking change — there's no compatibility guarantee across even a patch
release.
### 2. A custom Jackson serializer can silently bypass a registered Grails
marshaller
`JsonMapperSupport.rendersValue()` decides Jackson "owns" a type by checking
its serializer isn't `instanceof` one of six internal classes
(`BeanSerializerBase`, `EnumSerializer`, `ReferenceTypeSerializer`,
`StdContainerSerializer`, `UnsupportedTypeSerializer`,
`ToEmptyObjectSerializer`). But Jackson's own docs recommend `StdSerializer` as
the base class for a custom serializer — and it extends `ValueSerializer`
directly, none of those six. So any textbook custom serializer an app or a
plugin registers passes the check and is treated as fully owned by Jackson.
Repro, against this branch:
```groovy
def module = new SimpleModule()
module.addSerializer(Wrapper, new StdSerializer<Wrapper>(Wrapper) {
void serialize(Wrapper value, JsonGenerator gen, SerializationContext
ctxt) {
gen.writeStartObject()
gen.writeName('inner')
gen.writePOJO(value.inner) // not converter.convertAnother()
gen.writeEndObject()
}
})
def mapper = JsonMapper.builder().addModule(module).build()
// ... registered as the app's JsonMapper bean ...
JSON.registerObjectMarshaller(Secret) { Secret s -> '***REDACTED***' }
new JSON([w: new Wrapper(inner: new Secret(value:
'super-secret'))]).toString()
```
Expected (what an app relying on `registerObjectMarshaller` to redact
`Secret` would expect):
```
{"w":{"inner":"***REDACTED***"}}
```
Actual:
```
{"w":{"inner":{"value":"super-secret"}}}
```
Once `JsonMapperValueMarshaller` hands `Wrapper` to `generator.writePOJO()`,
the custom serializer runs inside Jackson's own machinery. Its
`gen.writePOJO(value.inner)` call never comes back through
`converter.convertAnother()`, so Grails never gets the chance to apply the
`Secret` marshaller — Jackson's default bean serializer renders it raw instead.
This is new exposure, not a pre-existing gap: before this PR a bare Jackson
module had zero effect on `grails.converters.JSON` — `registerObjectMarshaller`
was the only thing that controlled rendering. Now a plugin's or an app's own
Jackson module can silently override an app's redaction/transform marshaller
for a nested type it has no idea about, with no error or warning. It needs both
ingredients (a custom serializer that embeds another value, and a separate
marshaller registered for that embedded type) so it won't hit every app, but
the failure mode is exactly the one that matters when it does.
### A question on the overall approach
Given `spring.jackson.*` and the auto-configured `JsonMapper` are meant to
be used directly (`jsonMapper.writeValueAsString(value)`), why keep this hybrid
split — Grails marshals containers/beans/domain objects, Jackson marshals
"simple values" it recognizes — instead of just handing the whole object graph
to the `JsonMapper` and letting Jackson serialize it end to end, the standard
documented pattern? I'd guess it's to keep `ObjectMarshaller<JSON>` /
`JSON.registerObjectMarshaller` working, and Grails' GORM-aware domain-class
rendering (proxies, circular references, includes/excludes, `deep` vs. shallow
associations) — none of which map onto plain Jackson bean serialization. If
that's the reasoning, it's worth saying explicitly in the PR description, since
it's exactly the seam the bypass above lives in.
--
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]
