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

Fixes deserialization of enums when using booleans instead of strings #482

Merged
merged 4 commits into from
May 18, 2023
Merged

Fixes deserialization of enums when using booleans instead of strings #482

merged 4 commits into from
May 18, 2023

Conversation

patschl
Copy link
Contributor

@patschl patschl commented May 16, 2023

Description

Allows for boolean values for enums which support those, eg. DynamicMapping and Refresh.

Issues Resolved

Fixes #256

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.

@patschl
Fixes CHANGELOG.md
5b1c990 
Signed-off-by: Patrick Schlindwein <[email protected]>
@dblock
Copy link
Member

dblock commented May 16, 2023

Before I merge, what tests would fail if DeserializerSupportingBooleans was actually implemented as part of Deserializer? And what would be the user impact?

@patschl
Fixes checkstyle failure
f630192 
Signed-off-by: Patrick Schlindwein <[email protected]>
@patschl
Copy link
Contributor Author

patschl commented May 16, 2023

I just tried it and actually no test fails, it's just an unexpected behaviour to implicitly support boolean deserialization with the Enum Deserializer.

If you'd like to I could move the new deserialization logic to the existing Deserializer.

@dblock
Copy link
Member

dblock commented May 16, 2023

I just tried it and actually no test fails, it's just an unexpected behaviour to implicitly support boolean deserialization with the Enum Deserializer.

If you'd like to I could move the new deserialization logic to the existing Deserializer.

Yes yes!

@dblock
Copy link
Member

dblock commented May 16, 2023

Also please update documentation/README.

@patschl patschl force-pushed the fix_enum_boolean_deserialization branch from d79afc5 to b372d24 Compare May 17, 2023 11:20
@patschl
Copy link
Contributor Author

patschl commented May 17, 2023

I made the recommended changes and updated the CHANGELOG.md along with the Javadoc.
Which documentation/README are you referring to?

@patschl patschl force-pushed the fix_enum_boolean_deserialization branch from b372d24 to 125b78f Compare May 17, 2023 12:08
dblock
dblock approved these changes May 18, 2023
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

LGTM! No need to change anything in README.

@reta reta merged commit 5af5674 into opensearch-project:main May 18, 2023
@reta reta added the backport 2.x Backport to 2.x branch label May 18, 2023
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request May 18, 2023
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 18, 2023
@github-actions
Fixes deserialization of enums when using booleans instead of strings (…
657b7ed 
…#482)

* Fixes deserialization of enums when using booleans instead of strings

Signed-off-by: Patrick Schlindwein <[email protected]>

* Fixes CHANGELOG.md

Signed-off-by: Patrick Schlindwein <[email protected]>

* Fixes checkstyle failure

Signed-off-by: Patrick Schlindwein <[email protected]>

* Include boolean support in existing JsonEnum.Deserializer

Signed-off-by: Patrick Schlindwein <[email protected]>

---------

Signed-off-by: Patrick Schlindwein <[email protected]>
(cherry picked from commit 5af5674)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@patschl patschl deleted the fix_enum_boolean_deserialization branch May 18, 2023 21:37
dblock pushed a commit that referenced this pull request May 21, 2023
@opensearch-trigger-bot @github-actions
Fixes deserialization of enums when using booleans instead of strings (…
4294495 
…#482) (#488)

* Fixes deserialization of enums when using booleans instead of strings



* Fixes CHANGELOG.md



* Fixes checkstyle failure



* Include boolean support in existing JsonEnum.Deserializer



---------


(cherry picked from commit 5af5674)

Signed-off-by: Patrick Schlindwein <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@ls-urs-keller ls-urs-keller mentioned this pull request Jan 19, 2024
Closed
@BrendonFaleiro BrendonFaleiro mentioned this pull request Jun 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

@reta reta reta approved these changes

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

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

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

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

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

Assignees
No one assigned
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Serialize DynamicMapping boolean type
3 participants
@patschl @dblock @reta