Skip to content

Tag (Alias).valueOf(String) with JsonCreator #19

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

Closed
wants to merge 3 commits into from

Conversation

dansanduleac
Copy link
Contributor

@dansanduleac dansanduleac commented Jun 22, 2018

Otherwise the method won't be used by newer jackson-databind e.g. 2.9.6

Fixes #13

FLUPs:

@dansanduleac dansanduleac requested a review from iamdanfox June 22, 2018 15:15
@markelliot
Copy link
Contributor

this shouldn't be required

@dansanduleac
Copy link
Contributor Author

@melliot tested with jackson 2.9.6 and it doesn't work. Not sure if it's a bug in that version of jackson or a deliberate change (couldn't find it in the changelog) but shouldn't we be extra clear with our intention of what is a JsonCreator?

@dansanduleac
Copy link
Contributor Author

This is broken by FasterXML/jackson-databind@eb996aa as @robert3005 found.
I'll add a test that fails and the fix on top.

@dansanduleac dansanduleac force-pushed the ds/jsoncreator-alias branch from c48b6d0 to 997f06a Compare June 22, 2018 16:45
@dansanduleac dansanduleac requested a review from markelliot June 22, 2018 17:04
@dansanduleac
Copy link
Contributor Author

Acually, looking at https://github.com/palantir/conjure/blob/develop/wire.md, this maintains behaviour that used to work, but that we never committed to supporting.
Given that only one product internally seems to be relying on this, and the fix on their side would be easy, we can probably ask them to stop relying on unspecced behaviour.

@iamdanfox iamdanfox deleted the ds/jsoncreator-alias branch July 5, 2018 19:36
carterkozak pushed a commit to carterkozak/conjure-java that referenced this pull request Dec 24, 2018
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