-
Notifications
You must be signed in to change notification settings - Fork 769
Move to Jackson 3 #2173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Move to Jackson 3 #2173
Conversation
| <type>pom</type> | ||
| <scope>import</scope> | ||
| </dependency> | ||
| <!-- Jackson 3.0 requires jackson-annotations 2.20 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/FasterXML/jackson/blob/main/jackson3/MIGRATING_TO_JACKSON_3.md
Jackson 3.0 uses jackson-annotations 2.20
|
This is cool. I look forward to you fixing the build errors. |
|
Builds are green locally except for
which I believe is an expected breaking change. |
Any chance you can review this again? |
| */ | ||
| public <T extends GHEventPayload> T getPayload(Class<T> type) throws IOException { | ||
| T v = GitHubClient.getMappingObjectReader(root()).readValue(payload.traverse(), type); | ||
| T v = GitHubClient.getMappingObjectReader(root()).forType(type).readValue(payload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like ForType has been available since 2.5:
https://fasterxml.github.io/jackson-databind/javadoc/2.6/com/fasterxml/jackson/databind/ObjectWriter.html#forType-java.lang.Class-
| private Stream<String> readAotConfigToStreamOfClassNames(String reflectionConfig) throws IOException { | ||
| byte[] reflectionConfigFileAsBytes = Files.readAllBytes(Path.of(reflectionConfig)); | ||
| ArrayNode reflectConfigJsonArray = (ArrayNode) new ObjectMapper().readTree(reflectionConfigFileAsBytes); | ||
| ArrayNode reflectConfigJsonArray = (ArrayNode) JsonMapper.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonMapper has existed since 2.11:
https://fasterxml.github.io/jackson-databind/javadoc/2.11/com/fasterxml/jackson/databind/json/JsonMapper.html
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); | ||
| private GHGraphQLResponse<Object> convertJsonToGraphQLResponse(String json) throws JacksonException { | ||
| JsonMapper mapper = JsonMapper.builder().disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FAIL_ON_UNKNOWN_PROPERTIES has been around since 2.6 or earlier:
https://fasterxml.github.io/jackson-databind/javadoc/2.6/com/fasterxml/jackson/databind/DeserializationFeature.html
| private static final ObjectMapper MAPPER = new ObjectMapper(); | ||
| private static final JsonMapper MAPPER = JsonMapper.builder() | ||
| // Use annotations and enable access to private fields | ||
| .enable(MapperFeature.USE_ANNOTATIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the defaults have changed, I think all of these features and behavior were present in 2.x, correct?
| private static final int DEFAULT_MINIMUM_RETRY_MILLIS = DEFAULT_MAXIMUM_RETRY_MILLIS; | ||
| private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName()); | ||
| private static final ObjectMapper MAPPER = new ObjectMapper(); | ||
| private static final JsonMapper MAPPER = JsonMapper.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonMapper is a subclass of ObjectMapper correct?
| <scope>import</scope> | ||
| </dependency> | ||
| <!-- Jackson 3.0 requires jackson-annotations 2.20 --> | ||
| <dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this and keep the com.fasterxml.jackson.jackson-bom import. It is only dependency management, so it should generally be fine. There doesn't appear to be overlaping dependencies between com.fasterxml.jackson.jackson-bom and tools.jackson.jackson-bom.
|
@pvillard31 It looks like Jackson 2.20 and 3.x can exist side-by-side. As such, I'd like to ask you to go about this change a different way. First, it looks like many of the changes in this PR could be done while still depending on 2.20. Let's make all those changes first. Basically, start a new branch, take the changes from this PR, revert the Jackson version in the POM back to 2.20, then keep as many of the code changes as possible from this PR. I've noted at least a few of them. Second, take all the remaining places where the code would change between 2.20 and 3.x and refactor them into Similar to github-api/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java Lines 53 to 57 in 9dd29b8
This second part will probably include creating one or more new exception For testing, we can have a run similar to the okhttp in behavior, where the version used is controlled by a property. I know this is more work, but we need to continue to support 2.x for a while longer. |
Description
Following discussion in #2166.
Fixes #2166.
For reference: https://github.com/FasterXML/jackson/blob/main/jackson3/MIGRATING_TO_JACKSON_3.md
These changes are only targeting the 2.x line since it may be considered as containing breaking changes and requires a minimum java version of Java 17.
Before submitting a PR:
@linkJavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install site "-Dsurefire.argLine=--add-opens java.base/java.net=ALL-UNNAMED"locally. If this command doesn't succeed, your change will not pass CI.main. You will create your PR from that branch.When creating a PR: