Feature : Support naming strategy for enums #3792

JooHyukKim · 2023-02-22T12:26:45Z

Description

part of Add @EnumNaming, EnumNamingStrategy to allow use of naming strategies for Enums #2667
NEEDS lots of improvements at the moment...

JooHyukKim · 2023-02-22T12:50:42Z

Can I get some test case ideas on EnumNamingStrategies.CamelCaseStrategy? Currently the its implementation is exactly same as PropertyNamingStrategies.UpperSnakeCaseStrategy.translate()(🔗Link) and it does too much....

cowtowncoder · 2023-02-24T03:59:14Z

src/main/java/com/fasterxml/jackson/databind/EnumNamingStrategy.java

+
+public interface EnumNamingStrategy {
+
+    public String translate(String value);


Would be good to add Javadocs to explain direction, and name argument as something more specific than value.
I think that value would be Enum.name() to translate, and result is the "external" value used in JSON, right?

src/main/java/com/fasterxml/jackson/databind/EnumNamingStrategies.java

cowtowncoder · 2023-02-24T04:01:40Z

src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java

+            synchronized (this) {
+                namingStrategy = _namingStrategy;
+                if (namingStrategy == null) {
+                    EnumNaming enumNamingAnnotation = _valueClass.getAnnotation(EnumNaming.class);


No; all access to annotations should go via AnnotationIntrospector, not check directly.

cowtowncoder · 2023-02-24T04:05:18Z

Ok looks like a good starting point. Some suggestions:

All annotation access needs to be done via AnnotationIntrospector (and JacksonAnnotationIntrospector implementation), not directly. (I think there may be one or two exceptions but no new ones should be added). This is partly so that mix-in annotations work
All translations should be done when constructing EnumDeserializer and EnumSerializer -- not during reads/writes. Part of this has to do with performance, but it also keeps deserializer/serializer implementations simpler: they still just deal with a static list of values. This is how property naming strategies work too.

JooHyukKim · 2023-02-24T04:19:18Z

Thanks you for the review! 🙏🏼
I will get back with your reviews applied

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

src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java

src/main/java/com/fasterxml/jackson/databind/ser/std/EnumSerializer.java

JooHyukKim · 2023-02-28T03:09:24Z

src/main/java/com/fasterxml/jackson/databind/EnumNamingStrategies.java

+     *
+     * @since 2.15
+     */
+    public static class CamelCaseStrategy implements EnumNamingStrategy {


Implementation of convertEnumToExternalName here is from Guava's com.google.base.CaseFormat utility class.

cowtowncoder · 2023-03-11T03:41:10Z

src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java

@@ -253,6 +272,8 @@ protected Object _fromString(JsonParser p, DeserializationContext ctxt,
    {
        CompactStringObjectMap lookup = ctxt.isEnabled(DeserializationFeature.READ_ENUMS_USING_TO_STRING)
                ? _getToStringLookup(ctxt) : _lookupByName;
+        lookup = _hasEnumNaming(ctxt) ? _getEnumNamingLookup(ctxt) : lookup;


Ok, getting better, but I think all of introspection really should be done when constructing EnumDeserializer, and not dynamically. That avoids having to use synchronization and we just replace names used when constructing/initializing deserializer.

cowtowncoder · 2023-03-11T03:45:32Z

src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java

+            synchronized (this) {
+                lookup = _lookupByEnumNaming;
+                if (lookup == null) {
+                    EnumNamingStrategy namingStrategy = EnumPropertiesCollector


I think this should be fetched via AnnotationIntrospector, directly. Or, if we prefer, via DeserializationContext.

Agreed. Running introspection during construction like you suggested will doors to more information, so the helper class EnumPropertiesCollector will not be needed.

src/main/java/com/fasterxml/jackson/databind/introspect/EnumPropertiesCollector.java

cowtowncoder · 2023-03-11T03:49:44Z

Ok good, I can see this forming: functionality is there just need to (I think) rearrange things slightly -- mostly to handle all renaming, I think, when constructing EnumSerializer/EnumDeserializer. This allows keeping things immutable, reduce synchronized access and simplifies code.

I added some notes but probably not enough. I'll try to give some more feedback when I have time.

JooHyukKim · 2023-03-11T13:44:10Z

src/main/java/com/fasterxml/jackson/databind/ser/std/StdKeySerializers.java

+        // 19-Oct-2016, tatu: Used to just return DEFAULT_KEY_SERIALIZER but why not:
+        return new Default(Default.TYPE_TO_STRING, rawKeyType);
+    }
+


Right above this method, there is exact same method without EnumSerializer.constructEnumNamingStrategyValues(config, (Class<Enum<?>>) rawKeyType, annotatedClass)).

I think method above should be marked as @deprecated unless it is actually called from somewhere?

True. Added @deprecated annotation and documented it

JooHyukKim · 2023-03-11T14:01:47Z

Suggested changes based on the feedback provided by @cowtowncoder seem have been implemented. Changes made include:

Targeting the EnumDeserializer, StdKeyDeserializer, EnumSerializer, StdKeySerializer classes.
Arranging all introspection of the @EnumNaming annotation to be done during construction phase.
Making use of the AnnotationIntrospector for introspection wherever possible, while encapsulating the implementation contained in methods that create EnumValues and EnumResolver.

src/main/java/com/fasterxml/jackson/databind/AnnotationIntrospector.java

src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java

cowtowncoder · 2023-03-12T04:30:51Z

Ok this looks pretty good. I wish there was a way to combine 2 instances of CompactStringObjectMap (and matching thing for serializers), but that is probably not possible due to dynamic nature of DeserializationFeature.READ_ENUMS_USING_TO_STRING.

I think I'll want to go over the code one more time but maybe get it merged tomorrow.

Thank you very much @JooHyukKim for all the improvements here... this should be a nice new feature once we get it merged in for 2.15.0.

JooHyukKim · 2023-03-12T04:44:14Z

I think I'll want to go over the code one more time but maybe get it merged tomorrow.

Thank you for for yall's reviews also ✌️
Okay, I will pause searching for improvements now and wait for your review @cowtowncoder.

src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java

cowtowncoder · 2023-03-14T00:30:34Z

src/main/java/com/fasterxml/jackson/databind/introspect/EnumPropertiesCollector.java

+ *
+ * @since 2.15
+ */
+public class EnumPropertiesCollector {


Let's rename as... maybe EnumNamingStrategyFactory?

True, I was expecting this class would be come Enum version of Pojo's POJOPropertiesCollector. But I guess time will tell. I changed the name.

I had expected that this class would become the Enum version of POJOPropertiesCollector for Pojos, but it is not yet clear and you are right. I renamed the class accordingly. 🙏🏼

cowtowncoder · 2023-03-16T04:11:25Z

Whoa. This was gnarly to merge into main/3.0. :)
(but it's done now)

JooHyukKim · 2023-03-16T04:25:53Z

Whoa. This was gnarly to merge into main/3.0. :)
(but it's done now)

Niceee

JooHyukKim changed the title ~~Support naming strategy for enums~~ Feature : Support naming strategy for enums Feb 23, 2023

cowtowncoder reviewed Feb 24, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/EnumNamingStrategies.java Show resolved Hide resolved

cowtowncoder reviewed Feb 24, 2023

View reviewed changes

JooHyukKim commented Feb 27, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/util/EnumResolver.java Show resolved Hide resolved

JooHyukKim commented Feb 27, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java Outdated Show resolved Hide resolved

JooHyukKim commented Feb 27, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/ser/std/EnumSerializer.java Outdated Show resolved Hide resolved

JooHyukKim requested a review from cowtowncoder February 27, 2023 04:04

JooHyukKim commented Feb 28, 2023

View reviewed changes

JooHyukKim force-pushed the Naming-strategy-for-enums- branch 2 times, most recently from e1f405f to 3556f6e Compare March 5, 2023 06:25

JooHyukKim added 3 commits March 6, 2023 22:03

initial commit

5460b9b

Apply changes after first review

1d336f3

Fix all conflicts from rebase.

c67804f

JooHyukKim force-pushed the Naming-strategy-for-enums- branch from 3556f6e to c67804f Compare March 6, 2023 13:03

cowtowncoder reviewed Mar 11, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/introspect/EnumPropertiesCollector.java Outdated Show resolved Hide resolved

Apply review 2023.03.11

2e9a4b0

JooHyukKim commented Mar 11, 2023

View reviewed changes

Add more deser tests

2a6b999

JooHyukKim requested a review from cowtowncoder March 11, 2023 23:11

JooHyukKim added 3 commits March 12, 2023 08:27

Make JavaDoc more descriptive

248af1d

Improve JavaDoc of EnumPropertiessCollector

4ecda3c

Clean EnumPropertiesCollector

6799d2d

cowtowncoder reviewed Mar 12, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/AnnotationIntrospector.java Show resolved Hide resolved

cowtowncoder reviewed Mar 12, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java Outdated Show resolved Hide resolved

JooHyukKim added 2 commits March 12, 2023 13:29

reuse EnumDeserializer constructor

9473b1b

OverridefindEnumNamingStrategy() in AnnotationIntrospectorPair

c2f3699

pjfanning reviewed Mar 12, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java Outdated Show resolved Hide resolved

Improve decision-making on deserialization lookup and resolver

e855829

cowtowncoder reviewed Mar 14, 2023

View reviewed changes

JooHyukKim added 3 commits March 14, 2023 12:20

Deprecate StdKeySerializers.getFallbackKeySerializer() method

9526d3a

Change EnumPropertiesCollector name to EnumNamingStrategyFactory

f005b7f

Improve JavaDoc

4b783c7

JooHyukKim requested a review from cowtowncoder March 14, 2023 22:37

cowtowncoder approved these changes Mar 15, 2023

View reviewed changes

cowtowncoder merged commit 5343866 into FasterXML:2.15 Mar 15, 2023

JooHyukKim mentioned this pull request Apr 9, 2023

Mix-ins do not work for Enums #2787

Closed

JooHyukKim deleted the Naming-strategy-for-enums- branch May 22, 2023 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature : Support naming strategy for enums #3792

Feature : Support naming strategy for enums #3792

JooHyukKim commented Feb 22, 2023 •

edited

Loading

JooHyukKim commented Feb 22, 2023

cowtowncoder Feb 24, 2023

cowtowncoder Feb 24, 2023

cowtowncoder commented Feb 24, 2023

JooHyukKim commented Feb 24, 2023

JooHyukKim Feb 28, 2023

cowtowncoder Mar 11, 2023

cowtowncoder Mar 11, 2023

JooHyukKim Mar 11, 2023

cowtowncoder commented Mar 11, 2023

JooHyukKim Mar 11, 2023 •

edited

Loading

cowtowncoder Mar 14, 2023

JooHyukKim Mar 14, 2023

JooHyukKim commented Mar 11, 2023 •

edited

Loading

cowtowncoder commented Mar 12, 2023

JooHyukKim commented Mar 12, 2023

cowtowncoder Mar 14, 2023

JooHyukKim Mar 14, 2023

JooHyukKim Mar 14, 2023

cowtowncoder commented Mar 16, 2023

JooHyukKim commented Mar 16, 2023


		public interface EnumNamingStrategy {

		public String translate(String value);

Feature : Support naming strategy for enums #3792

Feature : Support naming strategy for enums #3792

Conversation

JooHyukKim commented Feb 22, 2023 • edited Loading

Description

JooHyukKim commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Feb 24, 2023

JooHyukKim commented Feb 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Mar 11, 2023

JooHyukKim Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim commented Mar 11, 2023 • edited Loading

cowtowncoder commented Mar 12, 2023

JooHyukKim commented Mar 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Mar 16, 2023

JooHyukKim commented Mar 16, 2023

JooHyukKim commented Feb 22, 2023 •

edited

Loading

JooHyukKim Mar 11, 2023 •

edited

Loading

JooHyukKim commented Mar 11, 2023 •

edited

Loading