-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
#235 Replace usage of java.util.Date #884
base: master
Are you sure you want to change the base?
Conversation
…T claims This commit replaces the usage of java.util.Date with java.time.Instant in handling JWT claims. This means upgrading from JDK7 to JDK8. This is done for better adherence to ISO 8601 standards and better precision in time-related operations. Classes working with claims have been updated to use the newer Instant class, providing better precision and cross-timezone compatibility. Additionally, overloading convenience methods for setting OffsetDateTime and ZonedDateTime on top of Instant have been added on the JwtBuilder and DefaultJwtBuilder.
There are 2 failing tests:
If a custom claim is added with type instant, then there are issues in A decision needs to be made how this needs to be handled. |
The JWT handling has been updated to default to Java's Instant class. This change removed support for various date and time classes such as ZonedDateTime, OffsetDateTime, Date, and Calendar in favor of an Instant-based approach throughout the codebase. The appropriate tests and documentation were updated to reflect this change.
Replaced all instances of 'Date' with 'Instant' in JwtParserTest to more accurately reflect variable usage. Also added a TODO in JwtDateConverter.java to clarify the handling of epochMillis vs epochSeconds.
This commit updates the version of the project across all pom.xml files from 0.12.4-SNAPSHOT to 1.0.0-SNAPSHOT as it prepares for a major release. Changes cover the core project and various extensions, signaling a coordinated upgrade across the codebase.
All instances of java.util.Date in code have been replaced with java.time.Instant for better time precision and timezone handling. This includes changes in variable names, method names, test cases, comments and documentation. The java.util.Date-based utilities have also been removed.
@pveeckhout Thanks for taking an interest in helping with this! However, we're not ready to force a JDK 8 upgrade until remaining JWT functionality is finished. #308 has some additional work in addition to the Java Date/Time changes for #235. #474 might also come into play instead of the existing builder utility methods as well. Some design work needs to be done before we can resolve this in earnest. I'm happy to leave this PR open of course, but you might have to deal with changes/rebases as we make other changes first. I hope that's ok! |
@lhazlewood, My pleasure to help! I am using jjwt in my own projects, and figured it might be time to try and contribute back to project. I will gladly keep this PR up to date with the other incoming changes. If there any other issue you figure I could help with, feel free to direct me to them. |
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!
I like this, I was playing around with something similar a little while ago.
I worry a little about folks migrating.
One option would be to add back the old setter methods with Date
parameters, potentially marking them as deprecated, though. As much as I prefer using Instant
lots of folks are probably still using Date
)
This doesn't work for the getters, but we could add deprecated getters, something like Date getExpirationDate() { return Date.from(expiration) }
It would still add a bit of burden on developers when updating, not sure if that's worth it. Or if we should do something else.
Thoughts?
* | ||
* @param exp the JWT {@code exp} value or {@code null} to remove the property from the Claims map. | ||
* @return the builder instance for method chaining. | ||
*/ | ||
@Override | ||
// for better/targeted JavaDoc | ||
JwtBuilder expiration(Date exp); |
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.
If adding these other wrappers, keeping the ability to wrap a Date
would help folks, and lessen the burden when migrating
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.
personally I would avoid keeping java.util.Date
around even as a convenience or temporary warpper
This would require a java version increase, and thus mean a major release. In my opinion it would be the ideal time to 'force' the devs to fix the breaking changes done in the major bump.
ofc, I am not the maintainer and will bend to the will of the code owner. just my 2cents
*/ | ||
@Deprecated | ||
T setExpiration(Date exp); | ||
T setExpiration(Instant exp); |
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.
Keep the old signatures for the deprecated methods
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 data change requires is a major release, I would actually opt to remove these deprecated methods (in another pull request)
I modified these just to ensure it all compiles and not longer requires java.time.Date
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.
I get that, but the method is marked as deprecated, so that shouldn't be changed.
(this is basically removing a old deprecated method, and adding a new deprecated method)
In general (and when possible) we want to provide an easy upgrade path for users, (e.g. adding an extra setter and deprecating the old one).
That's not possible with getters, (which would be a breaking change).
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.
the deprecated methods haven been reverted to use java.util.Date
Another potential addition related to using the newer dates api would be to add a convenance method to set the JwtBuilder validFor(Duration duration) {
Instant now = Instant.now(clock);
return this.expiration(now.plus(duration))
.notBefore(now)
.issuedAt(now);
} I'm not sure that name is clear, or even if this idea would provide enough value to folks. (This doesn't need to be in this PR, or implemented at all, just adding the thought to the discussion) |
fair point, these might be added in another PR that also add some convenience methods, like this one: #883 |
Replace usage of java.util.Date with java.time.Instant in JWT claims
This commit replaces the usage of java.util.Date with java.time.Instant in handling JWT claims. This means upgrading from JDK7 to JDK8. This is done for better adherence to ISO 8601 standards and better precision in time-related operations. Classes working with claims have been updated to use the newer Instant class, providing better precision and cross-timezone compatibility. Additionally, overloading convenience methods for setting OffsetDateTime and ZonedDateTime on top of Instant have been added on the JwtBuilder and DefaultJwtBuilder.
Closes #235