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 KeyDeserializer to WireSafeEnum #39

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Add KeyDeserializer to WireSafeEnum #39

merged 5 commits into from
Oct 31, 2023

Conversation

jhaber
Copy link
Member

@jhaber jhaber commented Oct 20, 2023

So that Jackson can deserialize maps with WireSafeEnum as keys (which has nicer ergonomics and reduces friction to using WireSafeEnum, since plain enums support this already).

This test is probably most illustrative of the new functionality:
464f920#diff-8095941f2209daf7eb627be2a5d6be0953a3073a8a4e4e91d239abbe945fcadbR537-R549

I thought we might need a KeySerializer as well, but it doesn't seem to be necessary.

Unfortunately this PR requires upgrading Jackson in order to pick up this feature: FasterXML/jackson-databind#2503

We're already using 2.12.6 internally, so this doesn't actually represent a change for us. However upgrading Jackson caused a few test failures that I need to investigate.

@stevie400 @Xcelled @suruuK @kmclarnon

@jhaber
Copy link
Member Author

jhaber commented Oct 20, 2023

Unfortunately JsonDeserializer and KeyDeserializer are both abstract classes, so we can't implement both in a single class. This required a bit of refactoring to extract some shared methods from the existing deserializer class, which makes the diff look a lot bigger than it actually is. I also refactored the tests so that all of the json reading/writing tests will cover plain objects and maps at the same time (to get better test coverage without having to duplicate all the tests).

I did this over 3 commits to make it a bit easier to review. First commit just moved some methods out of the inner class to the top level class:
2e0e3fa

Second commit was just test refactoring so that we're asserting on a stream of objects rather than a single object:
1efdd24

Third commit was actually adding the KeyDeserializer, wiring it into the tests, and upgrading Jackson so that the tests pass:
464f920

@jhaber
Copy link
Member Author

jhaber commented Oct 20, 2023

Tests fixed in 6fcb8dc, they just needed to be updated to handle some minor changes to exception messages in the new Jackson version.

@@ -22,6 +22,10 @@

<properties>
<project.build.targetJdk>1.8</project.build.targetJdk>
<dep.assertj.version>3.17.1</dep.assertj.version>
<dep.plugin.duplicate-finder.version>1.5.1</dep.plugin.duplicate-finder.version>
Copy link
Member Author

Choose a reason for hiding this comment

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

Some of these version overrides can go away once we upgrade basepom to 25.8. I tried doing that in this PR, but it automatically enabled prettier, which I didn't want to do as part of this PR (it reformatted all the code)

@kmclarnon
Copy link
Member

LGTM 👍

@jhaber
Copy link
Member Author

jhaber commented Oct 31, 2023

This should be pretty safe because deserialize to a map key would've just failed before. And if anyone was registering a custom key deserializer, that would still take precedence.

@jhaber jhaber merged commit dbf9d5b into master Oct 31, 2023
@jhaber jhaber deleted the jh/key-deserializer2 branch October 31, 2023 16:27
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