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

Support to add objects (primitive and collection) as claim in token pa… #289

Closed
wants to merge 0 commits into from

Conversation

complanboy2
Copy link

…yload

As requested @gustavoamigo we are giving support to add objects if primitive and collection types as claim in token payload.

Followed the suggestion by @lbalmaceda to make this change.
#164 (comment)

@complanboy2 complanboy2 mentioned this pull request Oct 10, 2018
4 tasks
@complanboy2
Copy link
Author

complanboy2 commented Oct 15, 2018

@lbalmaceda @martinoconnor Please review at your free time.
If there is a board or group of people that I should include, please let me know. thanks.

@ghost
Copy link

ghost commented Oct 17, 2018

The added withClaim() method is not type safe, and it is not possible to overload it due to type erasure. Also, nested collections would be raw types, so there would be no guarantee as to the types they would contain.

A reasonable compromise may be to accept only primitive wrapper types, Sets and Lists and to call toString() on anything else. That would require instanceof checks at runtime which may be ugly.

@complanboy2
Copy link
Author

complanboy2 commented Oct 26, 2018

The added withClaim() method is not type safe, and it is not possible to overload it due to type erasure. Also, nested collections would be raw types, so there would be no guarantee as to the types they would contain.

A reasonable compromise may be to accept only primitive wrapper types, Sets and Lists and to call toString() on anything else. That would require instanceof checks at runtime which may be ugly.

Hi @martinoconnor , thank you for the reivew. It helped fixing it better.

Agree, type safety would be an issue if consumer passes Map of custom objects. Updated the code to warn (throwing IllegalArugment) in that case. This change supports Map and/or Collection of objects of specified type/array type only. Restricted the checks to the minimal.

Please review the updated code when you have time.

@ornom0n
Copy link

ornom0n commented Nov 1, 2018

Hello,

Adding this feature would allow me to migrate from JWT 2 to JWT 3 as one of the claims we use is a Map and that cannot be changed. Is anything pending here?

@complanboy2
Copy link
Author

complanboy2 commented Nov 3, 2018

Hello,

Adding this feature would allow me to migrate from JWT 2 to JWT 3 as one of the claims we use is a Map and that cannot be changed. Is anything pending here?

Hi @ornom0n ,

I'm waiting on review, once the review is done, we are good to merge.
If it is helpful to you, please go ahead with the use. And please have a look at checks once, though major are passed.

@complanboy2
Copy link
Author

Any hope for getting this reviewed and also please check, #297 as well.

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.

2 participants