Repository navigation
Conversation
Signed-off-by: Michael Edgar <michael@xlate.io>
|
phillip-kruger
approved these changes
Oct 6, 2026
phillip-kruger
left a comment
Member
There was a problem hiding this comment.
LGTM. I built the branch and ran the core tests (410 run, 0 failures). I also compared the output against main for a few variations:
| Scenario | main |
PR |
|---|---|---|
Record from the issue: record Colliding(@JsonUnwrapped Inner foo) |
only bar |
bar, foo ✅ |
Bean with private @JsonUnwrapped PInner foo and public getFoo()/setFoo() |
only bar |
bar, foo ✅ |
@JsonUnwrapped(prefix = "x") PInner xfoo (collision after the prefix) |
only xbar |
xbar, xfoo ✅ |
@JsonUnwrapped @Schema(description = "outer") PInner foo |
only bar |
bar, foo ✅ |
I first suspected the outer accessor (foo()/getFoo() returning Inner) would attach to the inner foo resolver now that it's no longer ignored. updateTypeResolvers only stores the method when the types match, though, so it's fine.
Some minor, non-blocking comments:
- Edge case (FYI). If the outer class already has a property serialized as
foo, for example@JsonProperty("foo") Integer otheralongside@JsonUnwrapped PInner foo, the result is nowfoo: {type: [integer, string], format: int32}.maingave justinteger, though it dropped the unwrapped property. Jackson would write a duplicate key here anyway, so this is a user error, but the merged schema is a bit odd. It could be handled in a follow-up, or maybe a warning. - Comment wording. "Since the field will be ignored" could say more about why the early return matters, e.g. "...returning here avoids marking the unwrapped property as ignored."
- Javadoc placement in the test. The
/** Issue ... */block sits between the annotations and the method, so it isn't treated as Javadoc. Move it above@Testor make it a line comment. - Test coverage. The issue's reproducer is a record, which can't be written in
core's Java 11 tests. A record case intestsuite/data(next toRecordSchemaTest) and a prefix-collision case incorewould round out the coverage.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes #2687