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.

Sign up for GitHub

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

Jump to bottom

Add jsonValue check on Enum deserializer, default to checking name() function of enum #5103

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Add jsonValue check on Enum deserializer, default to checking name() function of enum #5103

merged 3 commits into from
Oct 25, 2024

Conversation

graytaylor0
Copy link
Member

@graytaylor0 graytaylor0 commented Oct 24, 2024 •
edited
Loading

Description

The new EnumDeserializer defaults to checking for the lower case enum value when there is no @JsonCreator on the enum class, and this broke basic enums like

enum MyEnum {
  ENUM_ONE,
  ENUM_TWO
}

Since we don't have consistency in enums as lower case we have to default to checking both for lowercase and uppercase values.

New behavior for deserializing enums

  1. Check for JsonCreator and compare using that
  2. Check for JsonValue and compare using that
  3. Compare to the enum.name()

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dlvenable
dlvenable previously requested changes Oct 24, 2024
View reviewed changes
Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

Which enums are supporting upper case presently? Those should be updated with @JsonCreator and we should move away from having these inconsistencies.

Supporting this will allow the inconsistencies to grow, whereas we should be working to remove them.

@graytaylor0 graytaylor0 changed the title Ignore case sensitivity when deserializing enums Add jsonValue check on Enum deserializer, default to checking name() function of enum Oct 24, 2024
@graytaylor0
Ignore case sensitivity when deserializing enums
4153147 
Signed-off-by: Taylor Gray <tylgry@amazon.com>
@graytaylor0
Add jsonValue check on Enum deserializer, default to checking name() …
6522706 
…function of enum

Signed-off-by: Taylor Gray <tylgry@amazon.com>
sb2k16
sb2k16 previously approved these changes Oct 24, 2024
View reviewed changes
chenqi0805
chenqi0805 previously approved these changes Oct 25, 2024
View reviewed changes
dlvenable
dlvenable requested changes Oct 25, 2024
View reviewed changes
...ne-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java Outdated
final DeserializationContext deserializationContext = mock(DeserializationContext.class);
when(jsonParser.getCodec()).thenReturn(objectMapper);

when(objectMapper.readTree(jsonParser)).thenReturn(new TextNode(testEnumOption.toString()));
Copy link
Member

Choose a reason for hiding this comment

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

I think this should return using .name() instead of .toString().

Copy link
Member Author

@graytaylor0 graytaylor0 Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No because in this case the value that users have to pass to the config is lowercase, not the enum value. Passing enum.name() will lead to exceptions since the JsonCreator returns the lowercase value

Copy link
Member

Choose a reason for hiding this comment

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

This testing is using the TestEnumOnlyUppercase enum. This does not have a @JsonCreator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see

...ne-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/EnumDeserializerTest.java
Enum<?> result = objectUnderTest.deserialize(jsonParser, deserializationContext);

assertThat(result, equalTo(testEnumOption));
}
Copy link
Member

Choose a reason for hiding this comment

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

We may also benefit from a test where the enum has a toString() which returns a different value, but is not marked with @JsonValue. In this case, we should be using the enum's name(). So we can verify this situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that already covered with this test void enum_class_without_json_creator_or_json_value_annotation_returns_expected_enum_constant

Copy link
Member

Choose a reason for hiding this comment

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

For that test, toString() returns the name, so you can't be sure if the code is using name() or toString().

Can you change that enum's toString() to return a random UUID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure will make that change

@graytaylor0
Address comments on test cases
7c61cf5 
Signed-off-by: Taylor Gray <tylgry@amazon.com>
@graytaylor0 graytaylor0 dismissed stale reviews from chenqi0805 and sb2k16 via 7c61cf5 October 25, 2024 15:57
@graytaylor0 graytaylor0 merged commit 9065acb into opensearch-project:main Oct 25, 2024
46 of 47 checks passed
@graytaylor0 graytaylor0 deleted the EnumFix branch October 25, 2024 16:33
graytaylor0 added a commit to graytaylor0/data-prepper that referenced this pull request Oct 25, 2024
@graytaylor0
Revert "Add jsonValue check on Enum deserializer, default to checking…
dd2dba9 
… name() function of enum (opensearch-project#5103)"

This reverts commit 9065acb.
graytaylor0 added a commit to graytaylor0/data-prepper that referenced this pull request Oct 25, 2024
@graytaylor0
Revert "Add jsonValue check on Enum deserializer, default to checking…
c29f002 
… name() function of enum (opensearch-project#5103)"

This reverts commit 9065acb.
graytaylor0 added a commit to graytaylor0/data-prepper that referenced this pull request Oct 25, 2024
@graytaylor0
Revert "Add jsonValue check on Enum deserializer, default to checking…
00cefb2 
… name() function of enum (opensearch-project#5103)"

This reverts commit 9065acb.

Signed-off-by: Taylor Gray <tylgry@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlvenable dlvenable dlvenable approved these changes

@sb2k16 sb2k16 sb2k16 left review comments

@chenqi0805 chenqi0805 chenqi0805 left review comments

@engechas engechas Awaiting requested review from engechas engechas is a code owner

@dinujoh dinujoh Awaiting requested review from dinujoh dinujoh is a code owner

@kkondaka kkondaka Awaiting requested review from kkondaka kkondaka is a code owner

@KarstenSchnitter KarstenSchnitter Awaiting requested review from KarstenSchnitter KarstenSchnitter is a code owner

@oeyh oeyh Awaiting requested review from oeyh oeyh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@graytaylor0 @dlvenable @chenqi0805 @sb2k16