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

Fix #745 #746

Merged
merged 1 commit into from
Apr 1, 2015
Merged

Fix #745 #746

merged 1 commit into from
Apr 1, 2015

Conversation

jpmeyer
Copy link

@jpmeyer jpmeyer commented Mar 30, 2015

Allow EnumDeserializer.deserializerForCreator to deserialize map keys.

@cowtowncoder
Copy link
Member

Hmmh. I am not sure I follow the fix -- why would the change in EnumDeserializer change behavior?

@jpmeyer
Copy link
Author

jpmeyer commented Mar 30, 2015

The inner class EnumDeserializer.FactoryBasedDeserializer does not handle Map keys as the JsonParser is positioned on a FIELD_NAME rather than a json value during the call to deserialize. If a module registers a general EnumDeserializer that delegates to an instance of this class obtained via EnumDeserializer.deserializerForCreator, the static factory method of the target enum is called with a null value from JsonParser.getStringValue() instead of the field name from JsonParser.getText()

An example of where this happens is in the dropwizard FuzzyEnumModule (included by default). In dropwizard, simply creating a configuration with an EnumMap causes a cryptic exception: "Incorrect type of value at: myConfigurationProperty; is of type: String, expected: MyEnumType

https://github.com/dropwizard/dropwizard/blob/master/dropwizard-jackson/src/main/java/io/dropwizard/jackson/FuzzyEnumModule.java

@cowtowncoder
Copy link
Member

Thanks -- I'll see if handling of FIELD_NAME wrt "getValueAsString()" is what needs changing, however. It depends on what intended semantics should be (field names are not really values), but I'm pretty sure most users would expect to get name as is.

DropWizard's registration of Enum handling has been bit problematic over time, and maybe I should check into that part as well. Goal would just to be sure that core components would take care of things they should, so that external code need not work around deficiencies there.

Thank you for submitting this -- I will try to figure out combination of things to solve the problem, and I am not questioning the problem itself at all. It definitely should work like you suggest.

@jpmeyer
Copy link
Author

jpmeyer commented Mar 31, 2015

I looked at changing the behavior of getValueAsString initially, but that
would be a change in core and I had no way of knowing if any callers depend
on the result being null for non-value tokens.

Glad to finally contribute something for a project I've loved and used for
many years.

On Mon, Mar 30, 2015 at 7:51 PM, Tatu Saloranta notifications@github.com
wrote:

Thanks -- I'll see if handling of FIELD_NAME wrt "getValueAsString()" is
what needs changing, however. It depends on what intended semantics should
be (field names are not really values), but I'm pretty sure most users
would expect to get name as is.

DropWizard's registration of Enum handling has been bit problematic over
time, and maybe I should check into that part as well. Goal would just to
be sure that core components would take care of things they should, so that
external code need not work around deficiencies there.

Thank you for submitting this -- I will try to figure out combination of
things to solve the problem, and I am not questioning the problem itself at
all. It definitely should work like you suggest.


Reply to this email directly or view it on GitHub
#746 (comment)
.

@cowtowncoder
Copy link
Member

@jpmeyer Yes, I totally understand and appreciate your hesitation at core changes. That's partly why I propose it, as I know more about the history and intent. :)

But even with that fix there is need for more immediate change since behavior of 2.5 can't be changed. I will work on this later today or tomorrow -- thank you for the contribution, and thank you for sending CLA very fast as well (noticed it just came through, impressive!).

cowtowncoder added a commit that referenced this pull request Apr 1, 2015
@cowtowncoder cowtowncoder merged commit 0f7f583 into FasterXML:master Apr 1, 2015
@cowtowncoder
Copy link
Member

Thanks again! I'll merge this into 2.5 branch as well, will be in 2.5.3.

@jpmeyer jpmeyer deleted the issue745 branch April 1, 2015 03:15
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.

None yet

2 participants