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

Add support for custom type deserialization with Jackson #495

Merged
merged 1 commit into from
Sep 30, 2019
Merged

Conversation

bdemers
Copy link
Member

@bdemers bdemers commented Sep 19, 2019

Adds new constructor to JacksonDeserializer(Map<String, Class> claimTypeMap), which will enable later calls Claims.get("key", CustomType.class) to work as expectd

Fixes: #369

TODO:

@bdemers
Copy link
Member Author

bdemers commented Sep 19, 2019

NOTE: this sits on top of fix-split-package to avoid merge conflicts (as JacksonDeserializer was moved in that branch)

@bdemers bdemers self-assigned this Sep 19, 2019
@coveralls
Copy link

coveralls commented Sep 19, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling c0470e1 on issue-369 into e20883e on fix-split-package.

@bdemers bdemers added this to the 0.11.0 milestone Sep 20, 2019
CHANGELOG.md Outdated
@@ -12,6 +12,7 @@ This minor release:

A backward compatibility module has been created `io.jsonwebtoken:jjwt-deprecated`, if you are compiling against
these classes directly, otherwise you will be unaffected.
* Adds support for custom types when deserializing with Jackson. To use configure your parser with `Jwts.parserBuilder().deserializeJsonWith(new JacksonDeserializer(Maps.of("claimName", YourType.class))).build()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to create the Maps.of utility method? (would reside, presumably, in io.jsonwebtoken.lang.Maps).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, easy enough to add. I was thinking of recommending just using the Java 11 Map.of, but I know many of us will be on 8 for a while yet.

* Utility class to help with the manipulation of working with Maps.
* @since 0.11.0
*/
public final class Maps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a builder of sorts? Or in addition?

For example, I consider this a lot nicer than a bunch of parameters in a single method:

Maps.put(key1, value1).put(key2, value2).put(key3, value3).build()

(for example).

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not against it that 🤔 I just went the Java 11-ish route of Map.of(),
I've done something things with a .put() in the past, my concern is Map returns the value type on a put, so that might be confusing?

I will say, I like the chained method look better than the overloaded method, mostly because it seems DRYer?

I'm not sure if Map.of() will ever see common use (at least for a while)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, right - instead of put, then maybe of or with or and.

Copy link
Contributor

@lhazlewood lhazlewood Sep 25, 2019

Choose a reason for hiding this comment

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

I like this:

Maps.of(key1, value1).and(key2, value2).and(key3, value3)...

Maps.of creates a builder instance, puts the first key/value and returns the builder. And just adds the key/value and returns the builder. Looks nice and fluent.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that!

"Anything more complex is expected to be already converted to your desired type by the JSON Deserializer " +
"implementation. You may specify a custom Deserializer for a JwtParser with the desired conversion " +
"configuration via the JwtParserBuilder.deserializeJsonWith() method. " +
"See https://github.com/jwtk/jjwt#custom-json-processor for more information.";
Copy link
Contributor

@lhazlewood lhazlewood Sep 26, 2019

Choose a reason for hiding this comment

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

I think we should add one more sentence at the end of this paragraph:

If using Jackson, you can specify custom claim POJO types as described in <url-here>.

Where <url-here> is of course the section in the documentation that explains how to do this that you're presumably writing in #500

Since I would think Jackson will be the most widely used parser.

Thoughts?

@@ -35,6 +42,32 @@ public JacksonDeserializer() {
this(JacksonSerializer.DEFAULT_OBJECT_MAPPER);
}

/**
* Creates a new JacksonDeserializer where the values of the claims can be parsed into given types. A common usage
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some documentation here that it uses a new ObjectMapper instance?

For example, after the current documentation:

<p>
Because custom type parsing requires modifying Jackson {@code ObjectMapper} state, this 
constructor creates a new internal {@code ObjectMapper} instance and customizes it to support the 
specified {@code claimTypeMap}.  This ensures that JJWT parsing behavior does not unexpectedly 
modify the state of another application-specific {@code ObjectMapper}.
</p>
<p>
If you would like to use your own {@code ObjectMapper} instance that also supports custom types for 
JWT {@code Claims}, you will need to first customize your {@code ObjectMapper} instance by registering 
your custom types and then use the {@link JacksonDeserializer(ObjectMapper) } constructor instead.
</p>

Based on this, I wonder if we might want to make MappedTypeDeserializer a dedicated independent public class in the jjwt-jackson module so that users can use it on their own ObjectMapper instance (e.g. they might have customized pretty printing, but still want custom type registration, etc). That class would document how to use it in the class JavaDoc based on what we've done, e.g.:

MappedTypeDeserializer typeDeserializer = new MappedTypeDeserializer(typeMap);
SimpleModule module = new SimpleModule();
module.addDeserializer(Object.class, typeDeserializer);
objectMapper.registerModule(module);

All of this would help address the confusion I had (for example) around type customization on an application-wide deserializer, as well as provide an out-of-the-box solution for people that wanted to customize their own ObjectMapper.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great doc suggestion

I thought about extracting MappedTypeDeserializer out, but I worry it might cause a maintenance headache trying to cover more parsing edge cases (when combined with other Jackson configurations). My personal pref would be to hold off unless people ask for it (YAGNI?). It's really easy to extract that class later, but if we make invalid assumptions now, it is harder to retract them.

That was my thinking anyway, though I could be missing some of the benefits of sharing an ObjectMapper instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

I worry it might cause a maintenance headache trying to cover more parsing edge cases (when combined with other Jackson configurations)

I was thinking the current implementation would be the only one we'd support, and if that doesn't meet one's needs, they can just do it the manual approach or implement their own JsonSerializer. At least they'd see easily how to do it themselves since understanding how that works is most of the difficulty, and our example gets them out of the weeds.

That said, I see value with leaving it private and the YAGNI argument. I guess I'm on the fence 50/50 and would be happy with whatever you choose, so feel free ;)

 - Adds new constructor JacksonDeserializer(Map<String, Class> claimTypeMap), which will enable later calls Claims.get("key", CustomType.class) to work as expectd
 - Adds new Maps utility class to make map creation fluent

Fixes: #369
@bdemers bdemers changed the base branch from fix-split-package to master September 27, 2019 21:38
@bdemers
Copy link
Member Author

bdemers commented Sep 27, 2019

Rebased manually (to merge in the split package change)

@bdemers bdemers merged commit 7090bf3 into master Sep 30, 2019
@bdemers bdemers deleted the issue-369 branch September 30, 2019 21:24
bdemers added a commit that referenced this pull request Oct 3, 2019
- Adds new constructor JacksonDeserializer(Map<String, Class> claimTypeMap), which will enable later calls Claims.get("key", CustomType.class) to work as expectd
 - Adds new Maps utility class to make map creation fluent

Fixes: #369
lhazlewood pushed a commit that referenced this pull request Feb 4, 2020
- Docs: Adds section to README covering custom object parsing (#500)
- Docs: Add note about JwtParserBuilder creating an immutable JwtParser (#508)
Doc: #486
Fixes: #494
Doc: #495
Fixes: #171
lhazlewood pushed a commit that referenced this pull request Feb 5, 2020
- Docs: Adds section to README covering custom object parsing (#500)
- Docs: Add note about JwtParserBuilder creating an immutable JwtParser (#508)
Doc: #486
Fixes: #494
Doc: #495
Fixes: #171

Updated documentation and changelog to reflect the new Gson extension. Fixes #410. (#476)
lhazlewood added a commit that referenced this pull request Feb 5, 2020
…#559)

- Docs: Adds section to README covering custom object parsing (#500)
- Docs: Add note about JwtParserBuilder creating an immutable JwtParser (#508)
Doc: #486
Fixes: #494
Doc: #495
Fixes: #171

Updated documentation and changelog to reflect the new Gson extension. Fixes #410. (#476)

Co-authored-by: Brian Demers <[email protected]>
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.

JSON POJO unmarshalling
3 participants