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

Deserialization of enums with name defined with different cases leads to InvalidDefinitionException: Multiple fields representing property #4409

Closed
1 task done
sbailliez opened this issue Mar 3, 2024 · 21 comments
Labels
2.16 Issues planned for 2.16
Milestone

Comments

@sbailliez
Copy link

sbailliez commented Mar 3, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

I'm using a thirdparty client that is deserializing json where an enum is made of variations like RGBA and RGBa. When upgrading our dependencies from Jackson 2.14.1 to 2.16.1 this API started failing with

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Multiple fields representing property "rgba" ...

Version Information

2.16.1

Looks like the bug was introduced in 2.16.0 as it works with 2.15.4

Reproduction

    public static class Bug {
        public enum ColorMode {
            RGB, RGBa, RGBA
        }
        public ColorMode colorMode;
    }

    @Test
    public void enumFails() throws Exception {
        String json = "{ \"color_mode\": \"RGBa\"}";
        ObjectMapper mapper = new ObjectMapper();
        mapper.setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE);
        mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
        Bug bug = mapper.readValue(json, Bug.class);
    }

Expected behavior

This should succeed.

Additional context

For this particular, I worked around this issue right now by patching the objectmapper created within the client and adding a custom deserializer for that enum:

    public static class EnumDeserializer extends StdDeserializer<File.ColorMode> {
        public EnumDeserializer() {
            this(null);
        }

        public EnumDeserializer(Class<?> vc) {
            super(vc);
        }

        @Override
        public File.ColorMode deserialize(JsonParser p, DeserializationContext ctxt) throws IOException, JacksonException {
            JsonNode node = p.getCodec().readTree(p);
            String value = node.asText();
            return value != null ? File.ColorMode.valueOf(value) : null;
        }
    }
    
    // .... reflective access to find the objectmapper in the thirdparty client...
    mapper.registerModule(new SimpleModule("patch-enum")
         .addDeserializer(File.ColorMode.class, new EnumDeserializer()));
@sbailliez sbailliez added the to-evaluate Issue that has been received but not yet evaluated label Mar 3, 2024
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 3, 2024

Thank you for reporting this, @sbailliez ! It definitely looks like a bug.

Two immediate guesses for possible triggering part (will try to see if they are related).

  1. Use of naming strategy would trigger bogus conflict
  2. Case-insensitivity incorrectly enabled by some processing resulting in bogus conflict.

@cowtowncoder cowtowncoder changed the title REGRESSION: Deserialization of enums with name defined with different cases leads to com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Multiple fields representing property Deserialization of enums with name defined with different cases leads to InvalidDefinitionException: Multiple fields representing property Mar 4, 2024
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 4, 2024

Quick note: I haven't dig into why "RGBA" and '"RGBa"are translated by convention, but you can work around the issue by adding@JsonProperty` into one of conflicting values:

    enum ColorMode {
        RGB, RGBa,
        @JsonProperty("RGBA")
        RGBA
    }

@cowtowncoder
Copy link
Member

Ok the problem is that names do translate both to rgba as per SNAKE_CASE_STRATEGY:

https://github.com/FasterXML/jackson-databind/blob/2.17/src/main/java/com/fasterxml/jackson/databind/PropertyNamingStrategies.java#L171

So this is a problem that was not detected until 2.15 (I think) -- behavior in 2.16 is as expected going forward.

In case where naming strategy converts value into conflicts, annotation is needed, or possible @EnumNaming override: however, adding:

@EnumNaming(EnumNamingStrategies.CamelCaseStrategy.class)

does not seem to help in this case (assuming it will not translate relevant values into separate ones)

cowtowncoder added a commit that referenced this issue Mar 4, 2024
cowtowncoder added a commit that referenced this issue Mar 4, 2024
@cowtowncoder
Copy link
Member

Closing as wont-fix; solution is to apply annotation on one of conflicting values.

@cowtowncoder cowtowncoder added will-not-fix Closed as either non-issue or something not planned to be worked on 2.15 and removed to-evaluate Issue that has been received but not yet evaluated labels Mar 4, 2024
@cowtowncoder cowtowncoder added this to the 2.15.0 milestone Mar 4, 2024
@cowtowncoder
Copy link
Member

/cc @JooHyukKim you might be interested in this -- I had forgotten that the per-mapper default PropertyNamingStrategy is applied to Enum values.

@JooHyukKim
Copy link
Member

Right, from users could feel awkward who are used to having Enums unaffected by PropertyNamingStrategy.
I should've thought this through wrt PropertyNameStrategy 🥲.

WRT, keeping backward compatibility either...

  1. Keep the Enums behave as they are?
  2. Or have EnumFeature as....
    // name yet TBD
    EnumFeature.APPLY_PROPERTY_NAMING_STRATEGY_TO_ENUMS

...such to keep the PropertyNamingStrategy away from Enums unless configured so.

@sbailliez
Copy link
Author

This is probably the first time that I see that the naming strategy was "supposed" to be applied to enums values. I thought there were expected to be serialized/deserialized as is and you would use valueOf or fromString to impact it. So I would vote for keeping backward compatibility.

In my case this is a thirdparty library, so I cannot add annotations but will open a bug for trying to annotate it explicitly to make it compatible for all jackson versions.

@cowtowncoder
Copy link
Member

I must say I did not remember that PropertyNamingStrategy was to have worked on enum values. But at the same time, behavior that 2.16 has out is, by know, the behavioral baseline.

I think EnumFeature to control this would be useful; that'd need to go in 2.17, and should have default settings that reflect what 2.16 does.

This could mean that strategy should, by default, apply, but may be disabled by the new EnumFeature.

@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 5, 2024

Okay, so will modify PR #4414 to implement the new EnumFeature accordingly (probably later today).

But WRT version, shouldn't we have the feature provided in 2.16? So users upgrading from 2.15 can easily fix things, like case here.

@cowtowncoder
Copy link
Member

No, as per SemVer no features may be added in patch releases: all patch releases should have same API -- additions may be done in minor releases.

But let's think this through first, before providing more solutions before understanding the problem.

What I'd want to know is the actual scope of problem; both wrt version in which behavior changes (wrt PropertyNamingStrategy applying to Fields of Enum types), and whether it actually changes serialization of regular Enums -- or is it just some special case. Given that we haven't had many reports, I wonder how commonly this actually changes behavior.

@cowtowncoder
Copy link
Member

@sbailliez I may have spoken too soon on this change being intentional wrt PropertyNamingStrategy. It may well be unintentional change, due to somewhat aggressive rewriting of Enum handling in 2.16 (and some in 2.15 I think).

@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 5, 2024

Did some digging and made tests against 2.15 and 2.16

  • 2.15🔗
    • PropertyNamingStrategy NOT applied to both Serialization and Deserialization
  • 2.16 🔗
    • PropertyNamingStrategy applied to ONLY deserialization

@sbailliez
Copy link
Author

@JooHyukKim do you mean for 2.15 "NOT applied to both Serialization and Deserialization"?

@sbailliez
Copy link
Author

@cowtowncoder I believe the reason there has not been any report related to this behavior in 2.16 is that it is pretty unconventional to have enums with values that have a case difference. In the API I refer to it is probably there for backward compatibility.

@cowtowncoder
Copy link
Member

@sbailliez True, that makes it less common. Enums often have underscores and some naming conventions change those so... but either way, it is interesting how few problems have been reported

@cowtowncoder
Copy link
Member

@JooHyukKim Given this, I feel comfortable disabling application of PropertyNamingStrategy defaults for Enum types generally.

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 6, 2024

Ok so it's EnumFormatShapeTest that fails if skipping for all Enum types (wrt #2365 as per comment).
Everything else passes fwtw.

@cowtowncoder cowtowncoder removed the will-not-fix Closed as either non-issue or something not planned to be worked on label Mar 6, 2024
@cowtowncoder
Copy link
Member

Ok. The trick is to check what BasicBeanDescription.findExpectedFormat() does, but from POJOPropertiesCollector -- and if we do that, I think ideally it'd be collected by POJOPropertiesCollector and then handed to BasicBeanDescription (to avoid multiple introspection calls).

But in short term (2.17) duplication would be fine.
This lookup is needed in 2.17 just for enum types, so we'd have something like:

    protected void _renameUsing(Map<String, POJOPropertyBuilder> propMap,
            PropertyNamingStrategy naming)
    {
        // _findFormatShape() needs to what `BasicBeanDescription.findExpectedFormat()`
        // (but need not take or use "defValue"; there's no defaulting)
        if (_type.isEnumType() && _findFormatShape() != Shape.OBJECT) {
            return;
        }
        ...
}

I might have time to look into this tomorrow, but if not I hope above helps.

@cowtowncoder cowtowncoder added 2.16 Issues planned for 2.16 and removed 2.15 labels Mar 7, 2024
@cowtowncoder cowtowncoder modified the milestones: 2.15.0, 2.16.2 Mar 7, 2024
cowtowncoder added a commit that referenced this issue Mar 7, 2024
@JooHyukKim
Copy link
Member

Thanks @sbailliez for reproduction and all!

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 8, 2024

Fixed for 2.16.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issues planned for 2.16
Projects
None yet
Development

No branches or pull requests

3 participants