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

Case-insensitive and number-based enum deserialization are (unnecessarily) mutually exclusive #3638

Closed
pgrefviau opened this issue Oct 19, 2022 · 2 comments · Fixed by #3802
Milestone

Comments

@pgrefviau
Copy link

Not sure if this should be filed as a bug or a feature request, since the "expected" behavior is mostly left to interpretation, but I went with a bug report in the end. Let me know if that makes sense.

Describe the bug
Deserializing an enum field that is marked for case-insensitive deserialization does not work if the source value happens to be shaped as the string representation of a number, whereas the same value will be deserialized properly if case-insensitive deserialization is disabled.

Version information
2.12.4

To Reproduce
If you have a way to reproduce this with:

enum MyEnum {
    FIRST_MEMBER(0),
    SECOND_MEMBER(1);
    
    private int index;

    private MyEnum(int index) {
        this.index = index;
    }
}

ACCEPT_CASE_INSENSITIVE_PROPERTIES: disabled

class MyClass {
    public MyEnum enumValue;
}

ACCEPT_CASE_INSENSITIVE_PROPERTIES: enabled

class MyClass {
    @JsonFormat(with = JsonFormat.Feature.ACCEPT_CASE_INSENSITIVE_PROPERTIES)
    public MyEnum enumValue;
}

Json input ACCEPT_CASE_INSENSITIVE_PROPERTIES: disabled ACCEPT_CASE_INSENSITIVE_PROPERTIES: enabled
{ "enumValue": "FIRST_MEMBER" }
{ "enumValue": "first_member" }
{ "enumValue": 0 }
{ "enumValue": "0" } ❌ 👈

Both failures (❌) are the same:

com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `MyClass` from String <X> not one of the values accepted for Enum class: ...

Expected behavior
I would expect the case annotated with 👈 in the table above to successfully deserialize the value. It seems as if the reason why this is not the case for now is that the conditional branches branches that handle the case-insensitive parsing and the number-based enum parsing are mutually exclusive:

// [databind#1313]: Case insensitive enum deserialization
if (Boolean.TRUE.equals(_caseInsensitive)) {
Object match = lookup.findCaseInsensitive(name);
if (match != null) {
return match;
}
} else if (!ctxt.isEnabled(DeserializationFeature.FAIL_ON_NUMBERS_FOR_ENUMS)) {
// [databind#149]: Allow use of 'String' indexes as well -- unless prohibited (as per above)
char c = name.charAt(0);
if (c >= '0' && c <= '9') {
try {
int index = Integer.parseInt(name);
if (!ctxt.isEnabled(MapperFeature.ALLOW_COERCION_OF_SCALARS)) {
return ctxt.handleWeirdStringValue(_enumClass(), name,
"value looks like quoted Enum index, but `MapperFeature.ALLOW_COERCION_OF_SCALARS` prevents use"
);
}
if (index >= 0 && index < _enumsByIndex.length) {
return _enumsByIndex[index];
}
} catch (NumberFormatException e) {
// fine, ignore, was not an integer
}
}
}

Not sure if this is by-design or if there is a historical precedent that lead to this, but I don't see why the deserialization process couldn't rely on attempting the number-based parsing in the case where _caseInsensitive is true and the subsequent lookup failed to return any results.

@pgrefviau pgrefviau added the to-evaluate Issue that has been received but not yet evaluated label Oct 19, 2022
@cowtowncoder cowtowncoder added 2.14 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue and removed to-evaluate Issue that has been received but not yet evaluated labels Oct 23, 2022
@cowtowncoder
Copy link
Member

From description I think you are right that this SHOULD work and is just an unfortunate implementation detail that it doesn't. I hope someone has time to look into this; I'd be happy to help with a PR, but do not have time right now to look into providing one myself (but may have in future, of course).

Thank you @pgrefviau for reporting this, adding reproductions.

@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 2, 2023

@cowtowncoder is this fixed yet? May I look into this?

@cowtowncoder cowtowncoder added 2.15 and removed has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue 2.14 labels Mar 3, 2023
@cowtowncoder cowtowncoder added this to the 2.15.0 milestone Mar 3, 2023
cowtowncoder added a commit that referenced this issue Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants