Use `@JsonProperty` for Enum values also when `READ_ENUMS_USING_TO_STRING` enabled #4036

iProdigy · 2023-07-13T22:29:23Z

When DeserializationFeature.READ_ENUMS_USING_TO_STRING is enabled, the enum deserializer neglects inspecting @JsonProperty; see the test case, it succeeds if READ_ENUMS_USING_TO_STRING is disabled and would fail with READ_ENUMS_USING_TO_STRING enabled (without this patch).

cowtowncoder · 2023-07-13T22:33:18Z

Hmmm. I guess this is a reasonable request as @JsonProperty should override not just Enum.name() provided value but also Enum.toString().

Can not fully validate suggested fix (although it makes sense); @JooHyukKim knows better I think having rewritten most of this code very recently.

iProdigy · 2023-07-13T23:22:14Z

When @JsonProperty is applied, WRITE_ENUMS_USING_TO_STRING ought to use the overriding string rather than Enum#toString, in my opinion (let me know if you disagree @cowtowncoder) - should I tackle this in another PR or here?

JooHyukKim · 2023-07-14T00:57:45Z

I guess this is a reasonable request as @JsonProperty should override not just Enum.name() provided value but also Enum.toString().

Can we use "improve" instead of "fix" in the title, @iProdigy ? Because some (bug) fixes can be merged back to 2.15, it's better not to look back and look for what bug we are trying to fix here. WDYT, @cowtowncoder ?

Can not fully validate suggested fix (although it makes sense); @JooHyukKim knows better I think having rewritten most of this code very recently.

I will take a look, seems straightforward though.

JooHyukKim · 2023-07-14T01:01:36Z

src/main/java/com/fasterxml/jackson/databind/util/EnumResolver.java

+            if (name == null) {
+                name = enumValue.toString();
+            }
+            map.put(name, enumValue);


As mentioend in the comment, we may not need to change behavior in this deprecated method

src/test/java/com/fasterxml/jackson/databind/deser/enums/EnumDeserializationTest.java

JooHyukKim · 2023-07-14T01:17:37Z

I guess this is a reasonable request as @JsonProperty should override not just Enum.name() provided value but also Enum.toString().

It's a straightforward, so I think it work just fine. @cowtowncoder. How about backporting to 2.15?

cowtowncoder · 2023-07-14T16:19:24Z

I think that for safety wrt avoiding regression, we should not try backporting improvements to behavior here.

@iProdigy yes I agree that name from @JsonProperty should override one from default/implicit, whether that is name() or toString(). That's the whole point here, right?

@JooHyukKim behavior of deprecated methods is something we could do either way, no strong opinion (that is; can fix, can leave without fix, as long as methods no longer called from within databind itself).

iProdigy · 2023-07-14T16:41:15Z

That's the whole point here, right?

Agreed, shall I update this PR with the serialization side or create a separate PR?

cowtowncoder · 2023-07-14T16:43:21Z

@iProdigy Ah! Sorry, missed the ser/deser part. Yeah, I think separate PR might make sense -- but either way is fine with me.

cowtowncoder · 2023-07-14T16:57:16Z

Ok one last thing @iProdigy -- if you havent been asked for (and sent) CLA, from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

we'd need that before merging the first contribution (once we have it, it's good for any number of contributions).
The usual way is to print, fill & sign, scan/photo, email to info at fasterxml dot com.
Once that's in, I can review and merge PRs. Apologies for this part of process but it's fortunately one-time thing (and if you already sent one just remind me of name/date and I'll check).

iProdigy · 2023-07-14T17:14:34Z

@cowtowncoder Just sent the signed CLA to that inbox!

JooHyukKim · 2023-07-15T01:22:17Z

LGTM!

JooHyukKim · 2023-07-15T01:40:35Z

@JooHyukKim behavior of deprecated methods is something we could do either way, no strong opinion (that is; can fix, can leave without fix, as long as methods no longer called from within databind itself).

If that's the case, may I suggest "not" to make modifications in deprecated methods? For following reasons...

Compliates going back and forth versions (like 2.15, 2.16, 3.0..). Too much attention spent.
To encourage newer version usage.

WDYT? /cc @cowtowncoder @iProdigy

iProdigy · 2023-07-15T01:57:15Z

It's worth noting that the deprecated EnumResolver.constructUsingToString is still called by:

StdKeyDeserializer IF _byToStringResolver is null
EnumDeserializer IF _lookupByToString is null

That said, these scenarios shouldn't (?) occur on 2.16, so updating the deprecated method may not be necessary

cowtowncoder · 2023-07-15T23:04:31Z

CLA received; will look over once more, then merge.

As to deprecated methods; yeah, need to be careful but at least existing unit test suite can catch some obvious issues.
But test coverage is not great for use of various types as Map keys, especially with "advanced" use cases (like use of annotations) so I guess it's hard choice. Adding handling for something not tested is not significantly better for long term than leaving things as are.

cowtowncoder · 2023-07-15T23:11:39Z

@JooHyukKim Good points in favor of NOT adding support, I concur.

JooHyukKim · 2023-07-16T06:29:02Z

@iProdigy Deleted earlier comment, because I thought we finished something before actually checking 😆. Anyways, thank you also!

fix(enum): use JsonProperty when READ_ENUMS_USING_TO_STRING

bc6c889

chore: also apply patch to deprecated factory method

afcdf13

iProdigy marked this pull request as ready for review July 13, 2023 23:02

iProdigy mentioned this pull request Jul 13, 2023

Fix Enum deserialization to use @JsonProperty, @JsonAlias even if EnumNamingStrategy used #4037

Merged

JooHyukKim reviewed Jul 14, 2023

View reviewed changes

src/test/java/com/fasterxml/jackson/databind/deser/enums/EnumDeserializationTest.java Show resolved Hide resolved

iProdigy changed the title ~~fix(enum): use JsonProperty when READ_ENUMS_USING_TO_STRING~~ feat(enum): use JsonProperty when READ_ENUMS_USING_TO_STRING Jul 14, 2023

iProdigy added 2 commits July 13, 2023 20:57

chore: add more test cases

798c350

chore: another test case

20b5cd7

cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jul 14, 2023

iProdigy mentioned this pull request Jul 14, 2023

Use @JsonProperty and lowercase feature when serializing Enums despite write using toString() #4039

Merged

iProdigy mentioned this pull request Jul 15, 2023

feat(helix): add content classification and branded content management twitch4j/twitch4j#811

Merged

2 tasks

chore: undo patch for deprecated method

2a17aa1

cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jul 15, 2023

cowtowncoder approved these changes Jul 15, 2023

View reviewed changes

cowtowncoder changed the title ~~feat(enum): use JsonProperty when READ_ENUMS_USING_TO_STRING~~ Use @JsonProperty for Enum values also when READ_ENUMS_USING_TO_STRING enabled Jul 15, 2023

cowtowncoder added the 2.16 Issues planned for 2.16 label Jul 15, 2023

cowtowncoder merged commit 059ccee into FasterXML:2.16 Jul 15, 2023

iProdigy deleted the fix/enum-deser-jsonproperty-tostring branch July 15, 2023 23:09

JooHyukKim mentioned this pull request Jul 14, 2023

Apply improved handling of Enum to all part #3990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `@JsonProperty` for Enum values also when `READ_ENUMS_USING_TO_STRING` enabled #4036

Use `@JsonProperty` for Enum values also when `READ_ENUMS_USING_TO_STRING` enabled #4036

iProdigy commented Jul 13, 2023

cowtowncoder commented Jul 13, 2023 •

edited

Loading

iProdigy commented Jul 13, 2023

JooHyukKim commented Jul 14, 2023

JooHyukKim Jul 14, 2023

JooHyukKim commented Jul 14, 2023 •

edited

Loading

cowtowncoder commented Jul 14, 2023

iProdigy commented Jul 14, 2023

cowtowncoder commented Jul 14, 2023

cowtowncoder commented Jul 14, 2023

iProdigy commented Jul 14, 2023

JooHyukKim commented Jul 15, 2023

JooHyukKim commented Jul 15, 2023

iProdigy commented Jul 15, 2023

cowtowncoder commented Jul 15, 2023

cowtowncoder commented Jul 15, 2023

JooHyukKim commented Jul 16, 2023

Use @JsonProperty for Enum values also when READ_ENUMS_USING_TO_STRING enabled #4036

Use @JsonProperty for Enum values also when READ_ENUMS_USING_TO_STRING enabled #4036

Conversation

iProdigy commented Jul 13, 2023

cowtowncoder commented Jul 13, 2023 • edited Loading

iProdigy commented Jul 13, 2023

JooHyukKim commented Jul 14, 2023

JooHyukKim Jul 14, 2023

Choose a reason for hiding this comment

JooHyukKim commented Jul 14, 2023 • edited Loading

cowtowncoder commented Jul 14, 2023

iProdigy commented Jul 14, 2023

cowtowncoder commented Jul 14, 2023

cowtowncoder commented Jul 14, 2023

iProdigy commented Jul 14, 2023

JooHyukKim commented Jul 15, 2023

JooHyukKim commented Jul 15, 2023

iProdigy commented Jul 15, 2023

cowtowncoder commented Jul 15, 2023

cowtowncoder commented Jul 15, 2023

JooHyukKim commented Jul 16, 2023

Use `@JsonProperty` for Enum values also when `READ_ENUMS_USING_TO_STRING` enabled #4036

Use `@JsonProperty` for Enum values also when `READ_ENUMS_USING_TO_STRING` enabled #4036

cowtowncoder commented Jul 13, 2023 •

edited

Loading

JooHyukKim commented Jul 14, 2023 •

edited

Loading