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

Mix-ins do not work for Enums #2787

Closed
kostousov-ds opened this issue Jul 6, 2020 · 17 comments
Closed

Mix-ins do not work for Enums #2787

kostousov-ds opened this issue Jul 6, 2020 · 17 comments
Labels
2.15 Issues planned for 2.15 enum Related to handling of Enum values has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue

Comments

@kostousov-ds
Copy link

kostousov-ds commented Jul 6, 2020

  1. create java enum like this
public enum SomeEnum {
    none,
    tax10,
    tax20
}
  1. create mixin for enum
public enum  SomeEnumMixin {
    @JsonProperty("zero")
    none,
    @JsonProperty("TypTyp")
    tax10,
    @JsonProperty("PytPyt")
    tax20
}
  1. register mixin via .addMixIn(SomeEnum.class, SomeEnumMixin.class)

  2. try to deserialize sometithing

ObjectMapper throws NullPointerException

java.lang.NullPointerException
	at com.fasterxml.jackson.databind.introspect.AnnotatedFieldCollector._addFieldMixIns(AnnotatedFieldCollector.java:117)
	at com.fasterxml.jackson.databind.introspect.AnnotatedFieldCollector._findFields(AnnotatedFieldCollector.java:94)
	at com.fasterxml.jackson.databind.introspect.AnnotatedFieldCollector.collect(AnnotatedFieldCollector.java:48)
	at com.fasterxml.jackson.databind.introspect.AnnotatedFieldCollector.collectFields(AnnotatedFieldCollector.java:43)
	at com.fasterxml.jackson.databind.introspect.AnnotatedClass._fields(AnnotatedClass.java:366)
	at com.fasterxml.jackson.databind.introspect.AnnotatedClass.fields(AnnotatedClass.java:338)
	at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector._addFields(POJOPropertiesCollector.java:393)
	at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector.collectAll(POJOPropertiesCollector.java:322)
	at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector.getPropertyMap(POJOPropertiesCollector.java:287)
	at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector.getProperties(POJOPropertiesCollector.java:186)
	at com.fasterxml.jackson.databind.introspect.BasicBeanDescription._properties(BasicBeanDescription.java:164)
	at com.fasterxml.jackson.databind.introspect.BasicBeanDescription.findProperties(BasicBeanDescription.java:239)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._findCreatorsFromProperties(BasicDeserializerFactory.java:292)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._constructDefaultValueInstantiator(BasicDeserializerFactory.java:276)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createEnumDeserializer(BasicDeserializerFactory.java:1472)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer2(DeserializerCache.java:371)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:349)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:264)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
	at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
	at com.fasterxml.jackson.databind.DeserializationContext.findNonContextualValueDeserializer(DeserializationContext.java:481)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.resolve(BeanDeserializerBase.java:497)
	at com.fasterxml.jackson.databind.deser.std.DelegatingDeserializer.resolve(DelegatingDeserializer.java:58)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:293)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
	at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
	at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:491)
	at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:4669)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4478)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3434)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3402)

I've tested on 2.11.0 and 2.11.1

@cowtowncoder
Copy link
Member

cowtowncoder commented Jul 8, 2020

Ok I can reproduce this.

Also checked same failure occurs on 2.10 at least.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jul 8, 2020

So, as per commit message, added failing test; and fixed NPE part.

However, functionality will not work as expected, most likely because of the way JDK implements Enums under the hood; entries may look like fields or perhaps instance methods but are neither if I remember correctly -- so mix-in handling functionality may not be able to attach annotations as expected to them yet (class annotations work fine and are tested).

I hope to resolve the second problem too, hence leaving this issue open.

And the problem itself is that when buffering content that can not yet be used (both for polymorphic subtype handling and for dealing with unwrapped content), it is not known that type will be needed as BigDecimal -- so it will be buffered as Double (with somewhat lower overhead).
While it would be possible to force storage of all floating-point values as BigDecimal, in theory, one nasty consequence but that binary formats with efficient storage format for 32- and 64-bit values would be heavily penalized by conversions between 2- and 10-based FP numbers. So it would be good to figure out something better; in case of textual format it might even make sense to defer number parsing.

But I do not know a good way yet; and changes likely need to go in a new minor version anyway (2.12.0 at earliest).

@cowtowncoder
Copy link
Member

cowtowncoder commented Jul 11, 2020

Ah. The problem is that unlike POJO properties that are discovered using AnnotatedField / -Method and so on -- on which mix-ins are applied -- enum names are detected directly from fields that Enum declares, by JacksonAnnotationIntrospector. This means mix-ins are not indeed applied.

This should be fixed but will be bit bigger undertaking as it requires changes to AnnotationIntrospector interface, for one.
That means it can not be fixed for versions earlier than 2.12.0.

@cowtowncoder cowtowncoder added 2.12 and removed 2.11 labels Jul 11, 2020
@cowtowncoder cowtowncoder changed the title NPE after add mixin for enum Field mix-ins do not work for Enums Jul 11, 2020
@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Oct 27, 2020
@cowtowncoder cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label Feb 20, 2021
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jul 15, 2021
@cowtowncoder cowtowncoder added the enum Related to handling of Enum values label Aug 3, 2022
@ianbrandt
Copy link

It looks like 2.15 will bring support for lowercasing of serialized enums per #3053, which is one use case I have for enum mix-ins. I'd rather configure the output case on a per-mix-in basis, so I'd still very much appreciate this feature.

@cowtowncoder cowtowncoder added 2.15 Issues planned for 2.15 and removed 2.14 labels Mar 14, 2023
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 14, 2023

It would be great to get this fixed but right now there is no good plan to do that, unfortunately.

But if anyone wants to tackle it, I'd be happy to help get PR ready.

/cc @JooHyukKim this would be another Enum-related challenge. :)

@JooHyukKim
Copy link
Member

@cowtowncoder thankssss for the mention! Like you thought, I did try tackling already 🤣.

That time I got caught up with other PR, but now that you mention it, I will go back to it now. Do you think below method would be the right place to start?

public JsonDeserializer<?> createEnumDeserializer(DeserializationContext ctxt,

And possibly somewhere in that method, do introspection like...

Class<?> mixInClass = ctxt.getConfig().findMixInClassFor(type.getRawClass());
JavaType mixInType = ctxt.constructType(mixInClass);

then pass in additional lookup for the construction of an EnumDeserializer?

@cowtowncoder
Copy link
Member

@JooHyukKim Ideally it would all work through standard mix-in handling and not enum-specific handling. But then again, that may be difficult in its own way.
FWTW with POJOs AnnotatedClass already has all mix-ins mixed in.

I think a starting challenge is figuring out what Enum values look like, to find how mix-ins apply.
I don't remember exactly what entries are (methods?), esp. in case of method overrides but they were not quite what I expected.

@ianbrandt
Copy link

ianbrandt commented Mar 21, 2023

I realized I have a need to sometimes de-snake-case and rename in addition to lowercase my enum values, so it's really exciting to see the pending PR for this. Mix-ins should prove a lot nicer than writing custom serializers and deserializers. 🎉

@JooHyukKim
Copy link
Member

@ianbrandt Thank you for the support! 🙏🏼 Though probably we might have to write code over again, you could say it's in progress.

@cowtowncoder
Copy link
Member

If we could figure out how to match Enum class structure for annotation flattening, that'd be great. I forget exact way Enum values map to "regular" class constructs but it was somewhat non-intuitive (i.e. they had to sort of hack it back in Java .... 1.4? or whenever they were added)

@JooHyukKim
Copy link
Member

I will go check. 🙆🏽‍♂️🙆🏽‍♂️ As far as I know,

  1. Each Enums values are compiled as ‘public static final SOME_VALUE’.
  2. Since they are fields, annotation flattening should be done in ‘AnnotatedFieldCollector’.
  3. “Annotation Flattening” means apply all annotations of matching fields from mixin class to target class. This is what you mean right? @cowtowncoder

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 23, 2023

@JooHyukKim correct, that's the idea. Change existing machinery to support mix-ins for Enums in general way (both for Enum class and enum values).

Main concerns/questions are just that:

  1. When using overrides in enum definition, do definitions change (there's some sub-classing involved as I recall, but maybe it won't affect annotation handling)
  2. From users POV, how should mix-ins look like? Do they have to create throw-away Enum types; or just know to specify static Fields with matching names? Basically, not trying to specify, say, Methods as those would not match (or should they? I guess with enough work it's possible but could get ugly).

@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 24, 2023

@jinwookh correct, that's t

I think someone else is tagged 😅here.

@cowtowncoder
Copy link
Member

... correct, that's t

I think someone else is tagged 😅here.

Sorry. Auto-completion for the win. :-/

@JooHyukKim
Copy link
Member

JooHyukKim commented Apr 9, 2023

I realized I have a need to sometimes de-snake-case and rename in addition to lowercase my enum values, so it's really exciting to see the pending PR for this. Mix-ins should prove a lot nicer than writing custom serializers and deserializers. 🎉

I just realized there might be a feature that might satisfy your needs, @ianbrandt ! 👍🏻

There is a recent PR #3792 that features naming strategy for enum's. Check below code for example usage. If your desired naming conversion is not supported, but is general enough, I think we can discuss a new EnumNamingStrategy in a new issue.

@EnumNaming(EnumNamingStrategies.CamelCaseStrategy.class)
    static enum EnumFlavorA {
        PEANUT_BUTTER, // handled as peanutButter
        SALTED_CARAMEL, // as saltedCaramel
        @JsonEnumDefaultValue
        VANILLA; // 
    }

JooHyukKim added a commit to JooHyukKim/jackson-databind that referenced this issue Jun 14, 2023
@cowtowncoder cowtowncoder changed the title Field mix-ins do not work for Enums Mix-ins do not work for Enums Jun 16, 2023
cowtowncoder added a commit that referenced this issue Jun 16, 2023
@JooHyukKim
Copy link
Member

It seems this issue can be closed now, addressed by #3990 ?

@cowtowncoder
Copy link
Member

Yes, included as fixed in 2.16.0-rc1 release notes, just forgot to close issue itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.15 Issues planned for 2.15 enum Related to handling of Enum values has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

4 participants