add support of Jackson 3 #736
Conversation
Reviewer's GuideAdds parallel support for Jackson 3.x JSON verification by introducing a new JsonJackson3Approvals helper, wiring in Jackson 3 dependencies, and duplicating the existing Jackson 2 tests for the new implementation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- JsonJackson3Approvals closely mirrors JsonJacksonApprovals; consider extracting a shared internal helper (e.g., a generic
JsonApprovalsthat takes mapper types) to avoid duplication and keep the two implementations behaviorally aligned over time. - You’ve added a Jackson 3 BOM and explicit
jackson-databindversions in bothapprovaltestsandapprovaltests-testsPOMs; consider centralizing the BOM in a parent or one module and relying on dependencyManagement to avoid duplicating version declarations. - In
JsonJackson3Approvals.verifyAsJson, you always force the.jsonextension viaoptions.forFile().withExtension(".json"); if callers might want to customize the extension, consider only setting a default when no extension has been provided.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- JsonJackson3Approvals closely mirrors JsonJacksonApprovals; consider extracting a shared internal helper (e.g., a generic `JsonApprovals` that takes mapper types) to avoid duplication and keep the two implementations behaviorally aligned over time.
- You’ve added a Jackson 3 BOM and explicit `jackson-databind` versions in both `approvaltests` and `approvaltests-tests` POMs; consider centralizing the BOM in a parent or one module and relying on dependencyManagement to avoid duplicating version declarations.
- In `JsonJackson3Approvals.verifyAsJson`, you always force the `.json` extension via `options.forFile().withExtension(".json")`; if callers might want to customize the extension, consider only setting a default when no extension has been provided.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds support for Jackson 3.x by creating a new JsonJackson3Approvals class alongside the existing JsonJacksonApprovals class for Jackson 2.x. Jackson 3.x introduced breaking API changes including a package rename from com.fasterxml.jackson to tools.jackson, requiring a separate implementation.
Key changes:
- New
JsonJackson3Approvalsutility class using Jackson 3.x'sJsonMapper.BuilderAPI - Jackson 3.x BOM and dependencies added to both main and test projects
- Test suite mirroring the existing Jackson 2.x tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| approvaltests/src/main/java/org/approvaltests/JsonJackson3Approvals.java | New utility class providing JSON approval testing with Jackson 3.x, using the new tools.jackson package and JsonMapper.Builder API |
| approvaltests/pom.xml | Adds Jackson 3.x BOM in dependency management and jackson-databind dependency |
| approvaltests-tests/src/test/java/org/approvaltests/JsonJackson3ApprovalsTest.java | Test suite for Jackson 3.x functionality with ObjectMapper override and duplicate fields tests |
| approvaltests-tests/src/test/java/org/approvaltests/JsonJackson3ApprovalsTest.testObjectMapperOverride.approved.json | Expected output for ObjectMapper override test |
| approvaltests-tests/src/test/java/org/approvaltests/JsonJackson3ApprovalsTest.testDuplicateFields.approved.json | Expected output for duplicate fields test |
| approvaltests-tests/pom.xml | Adds Jackson 3.x BOM and jackson-databind dependency for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import com.fasterxml.jackson.annotation.JsonIgnore; | ||
| import com.fasterxml.jackson.annotation.JsonInclude; |
There was a problem hiding this comment.
The test is importing Jackson 2.x annotations (com.fasterxml.jackson.*) instead of Jackson 3.x annotations (tools.jackson.*). For Jackson 3.x, the imports should be:
import tools.jackson.annotation.JsonIgnore;import tools.jackson.annotation.JsonInclude;
This will cause compilation errors or runtime issues when trying to use Jackson 3.x specific features.
| import com.fasterxml.jackson.annotation.JsonIgnore; | |
| import com.fasterxml.jackson.annotation.JsonInclude; | |
| import tools.jackson.annotation.JsonIgnore; | |
| import tools.jackson.annotation.JsonInclude; |
There was a problem hiding this comment.
There is no jackson-annotations dep for jackson 3.
Please read the note :
NOTE: jackson-annotations not affected as Jackson 3.0 uses 2.x variant (2.20) of jackson-annotations.
(https://github.com/FasterXML/jackson/wiki/Jackson-Release-3.0)
| <dependency> | ||
| <groupId>tools.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| <version>3.0.2</version> | ||
| </dependency> |
There was a problem hiding this comment.
Missing Jackson 3.x annotations dependency. Since the test uses Jackson 3.x annotations like @JsonIgnore and @JsonInclude, the following dependency should be added:
<dependency>
<groupId>tools.jackson.core</groupId>
<artifactId>jackson-annotations</artifactId>
</dependency>This dependency will be managed by the jackson-bom declared in the dependencyManagement section.
There was a problem hiding this comment.
There is no jackson-annotations dep for jackson 3.
Please read the note :
NOTE: jackson-annotations not affected as Jackson 3.0 uses 2.x variant (2.20) of jackson-annotations.
(https://github.com/FasterXML/jackson/wiki/Jackson-Release-3.0)
approvaltests-tests/pom.xml
Outdated
| <dependency> | ||
| <groupId>tools.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| <version>3.0.2</version> |
There was a problem hiding this comment.
The version should be removed from this dependency declaration since it's already managed by the jackson-bom in the dependencyManagement section (lines 20-26). This ensures version consistency across all Jackson 3.x artifacts and simplifies maintenance.
| <version>3.0.2</version> |
| </dependency> | ||
| <dependency> | ||
| <groupId>tools.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> |
There was a problem hiding this comment.
The Jackson 3.x dependency should be marked as <optional>true</optional> to maintain consistency with the Jackson 2.x dependency (line 93) and other similar optional dependencies like gson, xstream, etc. This ensures that users of the library aren't forced to include Jackson 3.x if they don't use this feature.
| <artifactId>jackson-databind</artifactId> | |
| <artifactId>jackson-databind</artifactId> | |
| <optional>true</optional> |
lbar
force-pushed
the
feat/jackson3-support
branch
from
8695361 to
6767dae
Compare
|
hey, thank you for this contribution. We'll get together on Monday 18 o'clock Central European Time and will have a look. If you also want to join, to say hello, I can forward you an invite. |
|
Thanks for the invitation, but unfortunately, I am not available this time. |
2 participants
Description
The library currently supports using Jackson to verify JSON objects. A new version of Jackson (3.x) has been released with API changes. The current
JsonJacksonApprovalsclass cannot be used.The solution
I created a new class,
JsonJackson3Approvals, which has the same API as the currentJsonJacksonApprovalsclass, in order to provide support for this new version. The same tests are also provided.Summary by Sourcery
Add Jackson 3 JSON verification support alongside existing Jackson 2 integration.
New Features:
Enhancements:
Build:
Tests: