Handle `Enum` introspection of values and aliases via `AnnotatedClass` instead of `Class<?>` #3832

JooHyukKim · 2023-03-18T13:32:41Z

Motivation

This PR provides more information to AnnotationIntrospector for Enum introspection by passing in MapperConfig and AnnotatedClass.

Prior to this PR, Enums have been directly handled from class declaration itself, using Class.getDeclaredFields()(🔗 check link for reference). Old way of handling Enums not only creates isolation from Jackson's rich functionality, but also down to the kind of basic functionalities such as simple mix-ins which created issues such as #2787.

Modifications

AnnotatedFieldCollector : consider enums as fields.
new API AnnotationIntrospector.findEnumValues(MapperConfig, Enum[], String[], AnnotatedClass)`.
new API AnnotationIntrospector.findEnumAlias(MapperConfig, Enum[], String[], AnnotatedClass)`.
BasicSerializerFactory : Remove enum self-rerence when handled via JsonFormat.Shape.Object
BasicDeserializerFactory : Pass in more information for EnumResolver construction

Original PR message (just for reference)

This pull request resolves issue #2787 by allowing mix-ins for Enum classes new 
EnumResolver.constructForMixIn(DeserializationConfig config, Class<?> enumCls, Class<?> mixInCls).

src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java

cowtowncoder · 2023-03-18T17:09:42Z

Ok, so I probably should have mentioned earlier that I do not want mix-in handling logic to be added outside of where it is done currently. So (de)serializers, factories that produce them should ideally not have to know anything about existence of mix-ins but would rather just look for annotations. Part of the challenge is that otherwise EnumResolver etc need to duplicate lookups for a growing set of annotations.

We can leave this PR open, just in case my thinking evolves. I realize that structure of Enum types as bytecode is kind of interesting and challenging to reconcile with POJOs, for example.

JooHyukKim · 2023-03-19T01:02:00Z

I realize that structure of Enum types as bytecode is kind of interesting and challenging to reconcile with POJOs,

@cowtowncoder Thinking further, I think what you saying is this -- Why @JsonProperty and @JsonAlias annotations from a mix-in class do not override those from the deserialization target class for Enum types, as Enum classes have a different nature than POJOs that prevents using POJO mix-in handling on them.

JooHyukKim · 2023-03-22T12:51:19Z

Would our issue be solved much easier in 3.x? Because, (3.x) Rewrite Annotation introspection methods for Enum handling #2550

…yukKim/jackson-databind into 2787-allow-mixins-for-enums

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

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

cowtowncoder · 2023-03-24T02:06:39Z

Ok aside from that @JsonFormat thing that needs to go elsewhere, this looks like it might work in general. I don't think we can get this in 2.15 (too risky at this point wrt API changes) but can continue work.
I'll add some smaller notes for changes.

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

JooHyukKim · 2023-03-24T05:07:31Z

in 2.15 (too risky at this point wrt API changes) but can continue work.

Great, sounds like a plan! 👍🏻 For the meantime, is there anything I can do to make our change more stable?

cowtowncoder · 2023-03-24T16:27:09Z

in 2.15 (too risky at this point wrt API changes) but can continue work.

Great, sounds like a plan! 👍🏻 For the meantime, is there anything I can do to make our change more stable?

No, just need to figure out how to remove special handling from AnnotatedFieldCollector (to resolve test failures some other way)

src/main/java/com/fasterxml/jackson/databind/ser/BeanSerializerFactory.java

JooHyukKim · 2023-05-28T05:49:50Z

I shifted direction of this PR and modified PR title and description accordingly, after studying this issue.

May I ask for your opinion, @cowtowncoder?

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

cowtowncoder · 2023-06-13T00:25:46Z

@JooHyukKim I hope to get back to this one Really Soon Now -- and I think your approach looks good from what I see.
But I do want to review this well.

Thank you once again for doing such a good job in taking feedback and finding ever improving solutions. I REALLY appreciate your diligence and persistence.

cowtowncoder · 2023-06-13T23:18:48Z

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

+    public String[] findEnumValues(MapperConfig<?> config, Enum<?>[] enumValues, String[] names,
+                                   AnnotatedClass annotatedClass) {
+        // First collect all JsonProperty.value()
+        HashMap<String, String> enumToPropertyMap = new HashMap<>();


Let's retain order by using LinkedHashMap

Done 👍🏻

cowtowncoder · 2023-06-13T23:20:45Z

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

+        for (AnnotatedField field : annotatedClass.fields()) {
+            JsonProperty property = field.getAnnotation(JsonProperty.class);
+            if (property != null) {
+                enumToPropertyMap.put(field.getName(), property.value());


Should this check for empty name ("")? If existing code doesn't check, that's fine I guess but I think often happens when just adding @JsonProperty as-is (useless annotation for Enums of course), or specifying other properties but not value.

Done 👍🏻 Revised(moved) this check to right before adding it to the Map.

Done, in accordance with your other comment 👍🏻

cowtowncoder · 2023-06-13T23:22:29Z

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

+        }
+        final Class<?> enumType = annotatedClass.getRawType();
+        HashMap<String,String> expl = null;
+        for (Field f : enumType.getDeclaredFields()) {


Why iterate over these again? Didn't AnnotatedClass.fields() have everything we need?

Oh, you are right about that AnnotatedClass.fields() does and "should" have everything we need. This one was from previous implementation and no longer needed.

Removed this whole iteration starting from....

final Class<?> enumType = annotatedClass.getRawType();

..... here ✌🏼

cowtowncoder · 2023-06-13T23:22:52Z

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

+                continue;
+            }
+            String n = enumToPropertyMap.get(f.getName());
+            if (n == null || n.isEmpty()) {


Ah ok, so empty String is handled here. Could have just not added it in Map earlier.

Done 👍🏻 Revised(moved) this check to right before adding it to the Map.

cowtowncoder

Looks pretty good, added a minor comment. Hoping to approve and merge tomorrow. Phew!

…or-enums

…ogic.

JooHyukKim · 2023-06-14T15:07:58Z

Looks pretty good, added a minor comment. Hoping to approve and merge tomorrow. Phew!

Thanks, 😆👍🏻 I applied all your comments (became much cleaner)

cowtowncoder

LGTM!

JooHyukKim · 2023-06-16T15:06:52Z

LGTM!

Thank you again for the thorough and solid insightful reviews, @cowtowncoder!

FYI, I plan on looking around in the project where we can leverage the new enum-related AnnotationIntrospector API. And also move away from using now-deprecated APIs.

Plz let me know if you need a hand merging this PR into master, or have any other suggestions or ideas. 🙏🏼🙏🏼

cowtowncoder · 2023-06-16T16:37:08Z

@JooHyukKim Agreed. I need to know merge 2.16->master, but I did mark old methods as deprecated and ideally should retrofit methods that call it to avoid them wherever possible.

cowtowncoder · 2023-06-16T18:15:41Z

@JooHyukKim Was able to merge it.

Noticed one other follow-up item: AnnotationIntrospector.findDefaultEnumValue() also takes Class, not AnnotatedClass.
So mix-ins wouldn't work.

But I guess it's not a big deal at this point.

JooHyukKim · 2023-06-17T11:15:05Z

Noticed one other follow-up item: AnnotationIntrospector.findDefaultEnumValue() also takes Class, not AnnotatedClass.

I wrote an issue #3990 to keep more PRs in one place 👍🏻. Will add this one also.

JooHyukKim added 3 commits March 18, 2023 21:38

Add test cases

0ff49d2

Allow construction of EnumResolver for mixin class

d9b70a9

Remove Pojo example tests

b11db13

JooHyukKim commented Mar 18, 2023

View reviewed changes

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

JooHyukKim added 5 commits March 18, 2023 22:56

improve JavaDoc

fbcc1c3

Clean up tests

3ebe045

Fix failing test

4cdef6c

Add test for mix-in of Wrapper field

bcd9025

Fix documentation

ed21cf3

cowtowncoder reviewed Mar 18, 2023

View reviewed changes

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

JooHyukKim added 5 commits March 24, 2023 00:58

Implement JacksonAnnotationIntrospector

e33619e

Merge branch '2787-allow-mixins-for-enums' of https://github.com/JooH…

39ff50d

…yukKim/jackson-databind into 2787-allow-mixins-for-enums

Merge branch '2787-allow-mixins-for-enums' of https://github.com/JooH…

51f923b

…yukKim/jackson-databind into 2787-allow-mixins-for-enums

Remove mixin from EnumResolver

1f7bbc1

Update BasicDeserializerFactory.java

9478871

JooHyukKim commented Mar 23, 2023

View reviewed changes

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

cowtowncoder reviewed Mar 24, 2023

View reviewed changes

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

cowtowncoder reviewed Mar 24, 2023

View reviewed changes

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

Add MapperConfig as first parameter

2a80825

JooHyukKim added 2 commits March 27, 2023 13:30

Filter self referencing properties with JsonFormat.Shape is OBJECT

b9a0677

Add more description

05d77bb

JooHyukKim commented Mar 27, 2023

View reviewed changes

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

JooHyukKim requested a review from cowtowncoder March 27, 2023 04:47

Merge branch '2.15' into 2787-allow-mixins-for-enums

ac04e46

JooHyukKim marked this pull request as draft May 22, 2023 15:06

JooHyukKim marked this pull request as ready for review May 26, 2023 15:02

JooHyukKim changed the title ~~Allow mixins for Enums~~ Handle Enum values and aliases with Annotated class May 28, 2023

JooHyukKim changed the title ~~Handle Enum values and aliases with Annotated class~~ Handle Enum values and aliases with AnnotatedClass May 28, 2023

JooHyukKim changed the title ~~Handle Enum values and aliases with AnnotatedClass~~ Handle Enum values and aliases via AnnotatedClass instead of Class May 28, 2023

JooHyukKim changed the title ~~Handle Enum values and aliases via AnnotatedClass instead of Class~~ Handle Enum values and aliases via AnnotatedClass instead of Class<?> May 28, 2023

JooHyukKim changed the title ~~Handle Enum values and aliases via AnnotatedClass instead of Class<?>~~ Handle Enum introspection of values and aliases via AnnotatedClass instead of Class<?> May 28, 2023

JooHyukKim commented May 28, 2023

View reviewed changes

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

JooHyukKim commented May 28, 2023

View reviewed changes

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

cowtowncoder reviewed Jun 13, 2023

View reviewed changes

JooHyukKim added 3 commits June 14, 2023 23:05

Merge remote-tracking branch 'upstream/2.16' into 2787-allow-mixins-f…

24f3f45

…or-enums

Apply review, improve JacksonAnnotationIntropector implementation l…

1f5baf6

…ogic.

Improve JavaDoc wrt FasterXML#2787

426fef5

cowtowncoder approved these changes Jun 16, 2023

View reviewed changes

cowtowncoder merged commit 2134584 into FasterXML:2.16 Jun 16, 2023

JooHyukKim deleted the 2787-allow-mixins-for-enums branch June 16, 2023 11:12

This was referenced Jun 17, 2023

Apply improved handling of Enum to all part #3990

Closed

Improve Enum serialization via AnnotatedClass #3991

Merged

JooHyukKim mentioned this pull request Jun 6, 2024

Possible 2.16.0 Enum-as-JSON-Object serialization regression #4564

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `Enum` introspection of values and aliases via `AnnotatedClass` instead of `Class<?>` #3832

Handle `Enum` introspection of values and aliases via `AnnotatedClass` instead of `Class<?>` #3832

JooHyukKim commented Mar 18, 2023 •

edited

Loading

cowtowncoder commented Mar 18, 2023

JooHyukKim commented Mar 19, 2023 •

edited

Loading

JooHyukKim commented Mar 22, 2023

cowtowncoder commented Mar 24, 2023

JooHyukKim commented Mar 24, 2023

cowtowncoder commented Mar 24, 2023

JooHyukKim commented May 28, 2023 •

edited

Loading

cowtowncoder commented Jun 13, 2023

cowtowncoder Jun 13, 2023

JooHyukKim Jun 14, 2023

cowtowncoder Jun 13, 2023

JooHyukKim Jun 14, 2023

cowtowncoder Jun 13, 2023

JooHyukKim Jun 14, 2023 •

edited

Loading

cowtowncoder Jun 13, 2023

JooHyukKim Jun 14, 2023

cowtowncoder left a comment

JooHyukKim commented Jun 14, 2023

cowtowncoder left a comment

JooHyukKim commented Jun 16, 2023 •

edited

Loading

cowtowncoder commented Jun 16, 2023

cowtowncoder commented Jun 16, 2023

JooHyukKim commented Jun 17, 2023

Handle Enum introspection of values and aliases via AnnotatedClass instead of Class<?> #3832

Handle Enum introspection of values and aliases via AnnotatedClass instead of Class<?> #3832

Conversation

JooHyukKim commented Mar 18, 2023 • edited Loading

Motivation

Modifications

Original PR message (just for reference)

cowtowncoder commented Mar 18, 2023

JooHyukKim commented Mar 19, 2023 • edited Loading

JooHyukKim commented Mar 22, 2023

cowtowncoder commented Mar 24, 2023

JooHyukKim commented Mar 24, 2023

cowtowncoder commented Mar 24, 2023

JooHyukKim commented May 28, 2023 • edited Loading

cowtowncoder commented Jun 13, 2023

cowtowncoder Jun 13, 2023

Choose a reason for hiding this comment

JooHyukKim Jun 14, 2023

Choose a reason for hiding this comment

cowtowncoder Jun 13, 2023

Choose a reason for hiding this comment

JooHyukKim Jun 14, 2023

Choose a reason for hiding this comment

cowtowncoder Jun 13, 2023

Choose a reason for hiding this comment

JooHyukKim Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

cowtowncoder Jun 13, 2023

Choose a reason for hiding this comment

JooHyukKim Jun 14, 2023

Choose a reason for hiding this comment

cowtowncoder left a comment

Choose a reason for hiding this comment

JooHyukKim commented Jun 14, 2023

cowtowncoder left a comment

Choose a reason for hiding this comment

JooHyukKim commented Jun 16, 2023 • edited Loading

cowtowncoder commented Jun 16, 2023

cowtowncoder commented Jun 16, 2023

JooHyukKim commented Jun 17, 2023

Handle `Enum` introspection of values and aliases via `AnnotatedClass` instead of `Class<?>` #3832

Handle `Enum` introspection of values and aliases via `AnnotatedClass` instead of `Class<?>` #3832

JooHyukKim commented Mar 18, 2023 •

edited

Loading

JooHyukKim commented Mar 19, 2023 •

edited

Loading

JooHyukKim commented May 28, 2023 •

edited

Loading

JooHyukKim Jun 14, 2023 •

edited

Loading

JooHyukKim commented Jun 16, 2023 •

edited

Loading