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

Fix Jwts usage to get rid of the deprecation warning #11500

Merged

Conversation

dschulz
Copy link
Contributor

@dschulz dschulz commented Mar 23, 2020

jjwt project has deprecated the Jwts.parser() method in favor of Jwts.parserBuilder() some months ago. This patch corrects the relevant code to use the updated versions and avoids the deprecation warning.

More info: jwtk/jjwt#499

@dschulz
Copy link
Contributor Author

dschulz commented Mar 23, 2020

The changes are trivial but I'm afraid I'll need some guidance to comply with the requirements.

@@ -100,8 +100,9 @@ public class TokenProvider {
}

public Authentication getAuthentication(String token) {
Claims claims = Jwts.parser()
Claims claims = Jwts.parserBuilder()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, you want to reuse the parser parserBuilder()...build() when possible. (and only incur the cost of construction once)
Maybe stick it in the PostConstruct above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right @bdemers, I guess I was too focused in the deprecation warning and absolutely not in the performance.

@bdemers
Copy link
Contributor

bdemers commented Mar 23, 2020

nit:
Note, reusing the parser could be done before this change too, but you could mutate the Parser when not using the parserBuilder() (the mutable methods will be removed in future versions of JJWT)

@CLAassistant
Copy link

CLAassistant commented Apr 20, 2020

CLA assistant check
All committers have signed the CLA.

@mshima mshima mentioned this pull request Apr 20, 2020
4 tasks
@jdubois
Copy link
Member

jdubois commented Apr 20, 2020

The performance point is very interesting! Indeed JwtParser is supposed to be immutable, so we could reuse it, which would be a very good improvement

@jdubois
Copy link
Member

jdubois commented Apr 20, 2020

@dschulz is that fine to make this PR "ready for review"? It's currently in draft mode.

@dschulz dschulz marked this pull request as ready for review April 20, 2020 21:21
@dschulz
Copy link
Contributor Author

dschulz commented Apr 20, 2020

@dschulz is that fine to make this PR "ready for review"? It's currently in draft mode.

@jdubois, absolutely. I'll make it ready for review and try to come up with another, better patch after that. I need to take some time examining the jjwt code to be more confident.

@mraible mraible closed this Apr 26, 2020
@mraible mraible reopened this Apr 26, 2020
@DanielFran DanielFran closed this May 7, 2020
@DanielFran DanielFran reopened this May 7, 2020
@DanielFran
Copy link
Member

@pascalgrimaud We should consider this PR for this build?

@pascalgrimaud
Copy link
Member

@DanielFran : not for this build, but for this release, you mean ? In this case, yes, I'm short on time to test everything but I'll trust on the daily builds this night

@DanielFran
Copy link
Member

Yes, for the release of course 😴😂

@DanielFran
Copy link
Member

@jdubois Did you test this changes?

@DanielFran DanielFran merged commit c949b1e into jhipster:master May 7, 2020
@pascalgrimaud pascalgrimaud added this to the 6.9.0 milestone May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants