Skip to content
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

Added support for java.time Instant instead of java.util.Date #311

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

azagniotov
Copy link

@azagniotov azagniotov commented Mar 13, 2018

This PR is trying to address what has been requested in #286 & indirectly in #291

Summary

  • Added support for java.time.Instant instead of java.util.Date
  • Made ObjectMapper static final in DefaultJwtParser.

Highlights

  • Added dependency on Jackson's jackson-datatype-jsr310, which brings in support for Java 8 date/time types (specified in JSR-310 specification)
  • All occurrences of java.util.Date have been replaced with java.time.Instant in prod & test code
  • In DefaultJwtParser, an instance of JavaTimeModule has been registered to bring in deserialization support for parsing timestamp into an Instant & the ObjectMapper has been made static final, since I already touched that code. This also addressed make_objectmapper_static #203 (https://github.com/FasterXML/jackson-docs/wiki/Presentation:-Jackson-Performance)
  • In DefaultJwtBuilder, a serializer for Instant has been registered with ObjectMapper to serialize Instant as a timestamp in seconds as per JWT RFC
  • JavaDocs have been updated where appropriate

Please note

This PR removes support for building the library against Java 7 as it leverages java.time.* classes which were added in Java 8. I do not know what the maintainers of jjwt have in mind in terms of sunsetting support for Java 7, so I am not sure if my PR is the right direction

@coveralls
Copy link

coveralls commented Mar 13, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7910865 on azagniotov:replacing_util_date_with_instant into 44faaca on jwtk:master.

@ghost
Copy link

ghost commented Mar 24, 2018

@lhazlewood this is a great addition, I hope to see it in a release soon.

@azagniotov azagniotov force-pushed the replacing_util_date_with_instant branch from ff97bc9 to 717689b Compare March 25, 2018 00:25
@azagniotov azagniotov force-pushed the replacing_util_date_with_instant branch from 717689b to 7910865 Compare March 25, 2018 01:01
@theencee
Copy link

@lhazlewood Any thoughts on this PR?

@bodawei
Copy link

bodawei commented Mar 28, 2018

This seems like a good idea to me. Given the comment on line 48 of DefaultJwtBuilder.java, should it just include a link to the jwt standard? I see a comment like that, and if I care, I'd like to actually be able to go see what the wording is.

@lhazlewood
Copy link
Contributor

JDK 7 support is still necessary - lots of frameworks and tools that use JJWT may not yet be on JDK 8. I don't know if/when we'll drop JDK 7, but if we do, it'll likely be on the 1.0 final release and not before then since such a change requires a semver major version increment.

@RDevet
Copy link

RDevet commented Oct 24, 2018

@lhazlewood Now that Java 11 is out, I strongly believe you should merge this PR. Java 7 is now legacy. Those still using Java 7 can continue to use the previous versions of JJWT.

@azagniotov
Copy link
Author

I am happy to resolve the conflicts

@devEffort
Copy link

👍

@aniket-patil
Copy link

This change is really important to have and I would strongly recommend merging this in.

@DanFTRX
Copy link

DanFTRX commented Mar 4, 2019

According to this: https://developer.android.com/about/dashboards/ more than half of all android devices still ran on Java 7 last year. (Java 8 is on devices with API 24+).

A lot of people seem to forget that Android Java has a different release schedule than regular Java. While regular Java may be at Version 11, Android Java is still barely jumping into Java 8 compatability.

Furthermore, 2018 Java 7 usage was still relatively among most Java developers according to https://www.jetbrains.com/research/devecosystem-2018/java/

@lhazlewood
Copy link
Contributor

As @DanFTRX has said, JDK 7 is still useful. We'll support it up to the 1.0 final release.

Is there a particular reason why this is important to people when Date.from(instant) will work? I'm just trying to understand if there are other issues beyond that.

@lhazlewood lhazlewood added this to the 1.0 milestone Mar 10, 2019
@RDevet
Copy link

RDevet commented Oct 21, 2019

@lhazlewood Now that Java 13 is out, I strongly believe you should merge this PR. Java 7 is now a billion years old. Those still using Java 7 can continue to use the previous versions of JJWT.

@lhazlewood
Copy link
Contributor

@RDevet as has been discussed in #308, per semantic versioning - which we absolutely will not break since thousands of projects rely on this determinism - this can only be done when we increment the major version number to 1.0. And for 1.0, we will not release it without the necessary JWE support. Feel free to follow along with #308 if you like, but this decision isn't being made lightly - believe me, I'd love to just use Java 8+ APIs.

Additionally, I would add a friendly caution to not get caught in the trap that Java 13 is oh-so-much-more advanced than JDK 8 or 11. That probably would have been true when JDK releases were feature-based. But they're not any longer - they're time-based - every 6 months, and IMO (and many others' opinions), time-based releases are misguided because what really matters is semantic versioning for deterministic software upgrades. Time-based releases just for the sake of meeting a time schedule - as opposed to something stable that people can depend on at a foundational level - is misguided at best IMO.

@lhazlewood lhazlewood added the jdk8 Changes related to migrating to JDK8 API features label Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jdk8 Changes related to migrating to JDK8 API features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants