Skip to content

JWTCreator for basic types - #282

Merged
lbalmaceda merged 5 commits into
auth0:masterfrom
skjolber:jwtCreaterForBasicTypes
Feb 13, 2020
Merged

JWTCreator for basic types#282
lbalmaceda merged 5 commits into
auth0:masterfrom
skjolber:jwtCreaterForBasicTypes

Conversation

@skjolber

@skjolber skjolber commented Sep 4, 2018

Copy link
Copy Markdown
Contributor

Support for creating tokens using Maps / Lists which must drill down to the base types Boolean, Integer, Long, Double, String and Date.

@lbalmaceda

Copy link
Copy Markdown
Contributor

This is one of the ideas we proposed back when all this discussion started. I really like what you came up with since it's not exposing any external dependencies in the public API 🎉 Unfortunately, I won't be able to dedicate time to this repo for at least 2 more weeks. I'll come back later

@skjolber

Copy link
Copy Markdown
Contributor Author

Our use-case is that we are currently migrating to Auth0 from Keycloak and would like to use this library for validation of tokens. So then it is also very convenient to use this library for creating mock tokens in our testing libraries as well - which requires a few deeper structures in the JWT. We are running both providers in parallel so that migration can be done service per service.

@skjolber

Copy link
Copy Markdown
Contributor Author

Any hope for getting this reviewed in 2018?

@skjolber

Copy link
Copy Markdown
Contributor Author

See also #278

@skjolber
skjolber requested a review from a team May 9, 2019 07:52
@skjolber
skjolber force-pushed the jwtCreaterForBasicTypes branch 4 times, most recently from 508d243 to 3758bb7 Compare May 9, 2019 11:04
@skjolber

skjolber commented May 9, 2019

Copy link
Copy Markdown
Contributor Author

@lbalmaceda bump. I've added code coverage, the build now passes the codecov quality checks.

Null keys and values in maps are disallowed, but null items in lists (and arrays) are allowed - fair?

If this gets merged, it would let us remove a test dependency (your competitor's library!) from our projects.

@damieng damieng added the medium This PR may require moderate effort to action, or contains many changes to review label May 10, 2019
@luisrudge
luisrudge requested review from lbalmaceda and removed request for a team June 19, 2019 17:06
@stale

stale Bot commented Jan 6, 2020

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

@stale stale Bot added the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@skjolber

skjolber commented Jan 6, 2020

Copy link
Copy Markdown
Contributor Author

Was any of the alternative PRs merged? If not then do not close.

@stale stale Bot removed the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@lbalmaceda

Copy link
Copy Markdown
Contributor

Hi @skjolber. Apologies for the delay here! We're planning to review this soon. If we happen to request changes, will you be able to address them?

@skjolber

skjolber commented Feb 5, 2020

Copy link
Copy Markdown
Contributor Author

@lbalmaceda yes

Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why would we want to accept null values in a list?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The corresponding withArrayClaim(..) does now, so basically it is just the same behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But are null values so bad? Could leave it up to the caller for value of Map as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checking again, it makes sense the way you did it since you're keeping the behavior of the current withArrayClaim implementation that accepts null, and you're not accepting nulls for maps. Let's keep it that way for now.

Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
@skjolber
skjolber force-pushed the jwtCreaterForBasicTypes branch from 06cfddd to f85c6e1 Compare February 11, 2020 20:43
@skjolber
skjolber requested a review from a team February 11, 2020 20:43
Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checking again, it makes sense the way you did it since you're keeping the behavior of the current withArrayClaim implementation that accepts null, and you're not accepting nulls for maps. Let's keep it that way for now.

Comment thread lib/src/main/java/com/auth0/jwt/JWTCreator.java Outdated
Comment thread lib/src/test/java/com/auth0/jwt/JWTCreatorTest.java Outdated
Comment thread lib/src/test/java/com/auth0/jwt/JWTCreatorTest.java Outdated
@lbalmaceda
lbalmaceda self-requested a review February 13, 2020 18:11
@lbalmaceda
lbalmaceda merged commit 0130a26 into auth0:master Feb 13, 2020
@lbalmaceda lbalmaceda added this to the v3-Next milestone Feb 13, 2020
@skjolber
skjolber deleted the jwtCreaterForBasicTypes branch February 13, 2020 20:28
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.10.0 Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CH: Added medium This PR may require moderate effort to action, or contains many changes to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants