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

Fix : Case insensitive and number based enum deserialization #3802

Conversation

JooHyukKim
Copy link
Member

Summary

Description

  • action : move if (!ctxt.isEnabled(DeserializationFeature.FAIL_ON_NUMBERS_FOR_ENUMS) && !_isFromIntValue) { part to outer.
  • pros : From code quality and future maintenance perspective, looks cleaner.
  • cons : Backward compatibility, we cannot assume how many systems rely on this issue-turned-into-specification.

@JooHyukKim
Copy link
Member Author

JooHyukKim commented Mar 2, 2023

cons : Backward compatibility, we cannot assume how many systems rely on this issue-turned-into-specification.

For above reason, if we are not confident about test cases, we might want to approach like below.

        } else {
            if (Boolean.TRUE.equals(_caseInsensitive)) {
                Object match = lookup.findCaseInsensitive(name);
                if (match != null) {
                    return match;
                }
                // newly added ----------------------------------------
                if (!ctxt.isEnabled(DeserializationFeature.FAIL_ON_NUMBERS_FOR_ENUMS)
                        && !_isFromIntValue) {
                    << same index parsing logic like below >>
                }
            } else if (!ctxt.isEnabled(DeserializationFeature.FAIL_ON_NUMBERS_FOR_ENUMS)
                    && !_isFromIntValue) {
                << index parsing logic that already existed >>
            }
        }

@pjfanning
Copy link
Member

If anyone upgrades jackson and have been inadvertently relying on the broken logic in the old versions, they can change their mapper config to fix things (by changing which DeserializationFeatures they set).

@JooHyukKim
Copy link
Member Author

If anyone upgrades jackson and have been inadvertently relying on the broken logic in the old versions, they can change their mapper config to fix things (by changing which DeserializationFeatures they set).

True true 👍🏻 Thanks, @pjfanning !

/**********************************************************
*/

public void testCaseSensitive() throws JsonProcessingException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going forward, let's just expose Exception -- this only because 3.0 (master) removes JsonProcessingException (and makes Jackson exceptions unchecked) and using Exception removes one incompatibility wrt merging of changes.
(I'll change this no need to modify PR)

@cowtowncoder
Copy link
Member

Thank you @JooHyukKim !

@cowtowncoder cowtowncoder merged commit ca38538 into FasterXML:2.15 Mar 3, 2023
@JooHyukKim JooHyukKim deleted the Fix-3638-Case-insensitive-and-number-based-enum-deserialization branch May 22, 2023 13:49
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 this pull request may close these issues.

Case-insensitive and number-based enum deserialization are (unnecessarily) mutually exclusive
3 participants