-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added parser for Jackson3 #1032
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: master
Are you sure you want to change the base?
Conversation
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.
Thanks @glascaleia this looks great
I left a handful of comments, but they are all small nits.
extensions/jackson3/pom.xml
Outdated
| <plugins> | ||
| <!-- The following plugin section is used in jjwt-jackson and jjwt-orgjson, to repackage (and verify) | ||
| binary compatibility with previous versions. In v0.11.0 the implementations changed packages to | ||
| avoid split package issues with Java 9+ see: https://github.com/jwtk/jjwt/issues/399 --> | ||
| <!-- TODO: remove these deprecated packages and this config before v1.0 --> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <configuration> | ||
| <relocations> | ||
| <relocation> | ||
| <pattern>io.jsonwebtoken.jackson.io</pattern> | ||
| <shadedPattern>io.jsonwebtoken.io</shadedPattern> | ||
| <includes>io.jsonwebtoken.jackson.io.*</includes> | ||
| </relocation> | ||
| </relocations> | ||
| </configuration> | ||
| </plugin> |
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.
This looks like it was copypasta from the jackson2 module. It shouldn't be needed here as there is no backward compatibility concerns.
| /** | ||
| * Deserializer using a Jackson {@link ObjectMapper}. | ||
| * | ||
| * @since 0.10.0 |
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.
nit: use 0.14 (or {NEXT_VERSION})
| * @param objectMapper the objectMapper to modify by registering a custom type-converting | ||
| * {@link tools.jackson.databind.JacksonModule Module} | ||
| * @param claimTypeMap The claim name-to-class map used to deserialize claims into the given type | ||
| * @since 0.13.0 |
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.
| * @since 0.13.0 |
| /** | ||
| * Serializer using a Jackson {@link ObjectMapper}. | ||
| * | ||
| * @since 0.10.0 |
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.
See version comment above
| */ | ||
| public class Jackson3Serializer<T> extends AbstractSerializer<T> { | ||
|
|
||
| static final String MODULE_ID = "jjwt-jackson"; |
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.
This probably doesn't matter at runtime but should this be jjwt-jackson3 to reduce confusion
| @@ -0,0 +1,146 @@ | |||
| /* | |||
| * Copyright (C) 2019 jsonwebtoken.io | |||
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.
| * Copyright (C) 2019 jsonwebtoken.io | |
| * Copyright (C) 2025 jsonwebtoken.io |
| <!-- TODO: remove these deprecated packages and this config before v1.0 --> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> |
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.
Shouldn't need the shade plugin in this module (see comment above)
| <properties> | ||
| <maven.javadoc.additionalOptions>-html5</maven.javadoc.additionalOptions> | ||
| <surefire.argLine>${test.addOpens}</surefire.argLine> | ||
| <easymock.version>5.6.0</easymock.version> |
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.
Should this be moved to the parent pom?
extensions/jackson3/.mvn/jvm.config
Outdated
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.
This file should be removed (it would be in the root of the project if it needs to be added)
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.
This file should be removed (it would be in the root of the project if it needs to be added)
|
Before merging, we need to finalize a plan for multi-jvm testing, Jackson3 (and likely other libraries in the near future) depends on Java 17. We could build with 17+, and use multiple toolchains to test the other modules with older JVMs (e.g. Java 8). NOTE: This is NOT something @glascaleia needs to worry about |
|
I made the requested changes. Regards Giuseppe |
|
@bdemers when the new release with this ?? |
I created the new parser for jackson3. Regards Giuseppe