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

Fix: Duplicate keys in JSON produced for Catalog objects #915 #955

Merged
merged 27 commits into from
Mar 5, 2025
Merged

Fix: Duplicate keys in JSON produced for Catalog objects #915 #955

merged 27 commits into from
Mar 5, 2025

Conversation

vedansh-5
Copy link
Contributor

@vedansh-5 vedansh-5 commented Feb 6, 2025 •
edited
Loading

Title

fix: Remove duplicate type field in Catalog serialization

Description

Problem

The Catalog API responses were showing duplicate type fields due to Jackson's polymorphic type handling.

Solution

  • Created custom template for JsonTypeInfo annotation
  • Modified serialization to use EXISTING_PROPERTY instead of PROPERTY
  • Added comprehensive serialization tests

Files Changed

  1. api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java

    • Added tests for Unicode characters
    • Added tests for whitespace handling
    • Added tests for role ARN validation
    • Added tests for null/empty fields

  2. api/management-model/templates/typeInfoAnnotation.mustache (New)

    • Custom template for JsonTypeInfo annotation
    • Uses EXISTING_PROPERTY strategy

Testing

  • ✅ All unit tests passing
  • ✅ Verified with Postman
  • ✅ Tested edge cases:
    • Unicode characters
    • Whitespace handling
    • Role ARN validation
    • Null fields
    • Empty fields

Screenshots

Screenshot 2025-02-06 221503
Screenshot 2025-02-06 221447

Closes #915

eric-maynard
eric-maynard reviewed Feb 6, 2025
View reviewed changes
eric-maynard
eric-maynard reviewed Feb 6, 2025
View reviewed changes
Copy link
Contributor

@eric-maynard eric-maynard left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Left a few comments

dimas-b
dimas-b reviewed Feb 6, 2025
View reviewed changes
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, @vedansh-5 !

@vedansh-5
Assert exact JSON string, got rid of append calls, added comments bef…
b9128ad 
…ore each test and removed the readme.md file
dimas-b
dimas-b reviewed Feb 21, 2025
View reviewed changes
...gement-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java Outdated
private Catalog verifyRoundTrip(Catalog original) throws JsonProcessingException {
String json = mapper.writeValueAsString(original);
Catalog deserialized = mapper.readValue(json, Catalog.class);
assertThat(deserialized).usingRecursiveComparison().isEqualTo(original);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply assertThat(deserialized).isEqualTo(original)?

Do you think the .equals() method in Catalog does not take nested objects into account?

dimas-b
dimas-b approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Thanks for you patience @vedansh-5 !

The current state of this PR LGTM 👍

@flyrain @eric-maynard @dennishuo : Please double check mustache changes.

@dimas-b
Copy link
Contributor

dimas-b commented Feb 21, 2025

@snazy : Could you have another look too?

@vedansh-5
Copy link
Contributor Author

@snazy : I have worked on the proposed changes, could you please take another look at them. Thankyou for your feedback.

@vedansh-5
Copy link
Contributor Author

Hi @snazy,
I have worked on the requested changes based on your feedback. When you have some time, could you please take another look? Your review would be greatly appreciated.
Thanks!

@snazy
Copy link
Member

snazy commented Feb 27, 2025

Are there test cases ensuring that the type-field itself is only serialized once?

@vedansh-5
Copy link
Contributor Author

Are there test cases ensuring that the type-field itself is only serialized once?

I have added test case for the same.

@snazy
Copy link
Member

snazy commented Feb 27, 2025

Are there test cases ensuring that the type-field itself is only serialized once?

I have added test case for the same.

And where? I don't see any executed test case that checks it.

dimas-b
dimas-b reviewed Feb 27, 2025
View reviewed changes
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Good point by @snazy

@vedansh-5 : IIRC, we have a test validating the serialized JSON string at some point... I guess it was lost in the refactrorings. Cf. #955 (comment)

Please re-add a test method that asserts the exact JSON string form of a java Catalog object.

@vedansh-5
Copy link
Contributor Author

Good point by @snazy

@vedansh-5 : IIRC, we have a test validating the serialized JSON string at some point... I guess it was lost in the refactrorings. Cf. #955 (comment)

Please re-add a test method that asserts the exact JSON string form of a java Catalog object.

I have added the test method and using verifyRoundTrip as inline

@vedansh-5
Copy link
Contributor Author

@snazy: I made the requested changes. When you have some time, could you please take another look?
Thanks!

@vedansh-5
Copy link
Contributor Author

@snazy : Follow-up for the same message

@snazy: I made the requested changes. When you have some time, could you please take another look? Thanks!

@dimas-b dimas-b dismissed snazy’s stale review March 5, 2025 20:51

Previously flagged issues have been addressed

@dimas-b dimas-b merged commit 680f9b9 into apache:main Mar 5, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-maynard eric-maynard eric-maynard approved these changes

@dimas-b dimas-b dimas-b approved these changes

@gh-yzou gh-yzou gh-yzou approved these changes

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

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

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

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

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

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

@collado-mike collado-mike Awaiting requested review from collado-mike collado-mike is a code owner

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

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

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

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

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

@snazy snazy Awaiting requested review from snazy snazy 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.

Duplicate keys in JSON produced for Catalog objects
5 participants
@vedansh-5 @gh-yzou @dimas-b @snazy @eric-maynard