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

refactor(java): improve oneOf types DX #1990

Merged
merged 13 commits into from
Sep 11, 2023
Merged

refactor(java): improve oneOf types DX #1990

merged 13 commits into from
Sep 11, 2023

Conversation

aallam
Copy link
Member

@aallam aallam commented Sep 8, 2023 •
edited
Loading

🧭 What and Why

Currently, all oneOf types are built using a wrapper, even if no wrapper is needed. Also, generics are added for oneOf types too, although it doesn't make sense, especially if the underlying types are different (e.g., different records from each index).

Changes included:

  • Refactor the oneOf types to become interfaces, with concrete implementation when the oneOf type is a class or enum.
  • Use wrappers in the case of primitives and collections.
  • During deserialization, always start with types that have discriminator fields.
  • Move from jetbrains @NotNullto javax @Nonnull

@aallam aallam requested a review from a team as a code owner September 8, 2023 13:27
@netlify
Copy link

netlify bot commented Sep 8, 2023 •
edited
Loading

✅ Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit dd25195
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/64fed6bfb45c79000853745a
😎 Deploy Preview https://deploy-preview-1990--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@algolia-bot
Copy link
Collaborator

algolia-bot commented Sep 8, 2023 •
edited
Loading

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

@aallam aallam enabled auto-merge (squash) September 8, 2023 13:39
@aallam aallam changed the title rafactor(java): improve oneOf types DX refactor(java): improve oneOf types DX Sep 8, 2023
millotp
millotp reviewed Sep 8, 2023
View reviewed changes
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

small review, I'm waiting for the code to generate, but it looks super cool !

generators/src/main/java/com/algolia/codegen/GenericPropagator.java Show resolved Hide resolved
playground/java/src/main/java/com/algolia/playground/Insights.java Show resolved Hide resolved
generators/src/main/java/com/algolia/codegen/utils/OneOfUtils.java
markOneOfChildren(models, model);
generateSealedChildren(models, modelPackage, model);
model.vendorExtensions.put("x-is-one-of", true);
model.vendorExtensions.put("x-one-of-explicit-name", Utils.shouldUseExplicitOneOfName(model.oneOf));
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think you can move the function shouldUseExplicitOneOfName to this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be used also inside Utils

templates/java/api_helpers.mustache Show resolved Hide resolved
@aallam aallam requested a review from millotp September 11, 2023 09:18
@millotp
Copy link
Collaborator

millotp commented Sep 11, 2023 •
edited
Loading

generics are added for oneOf types too, although it doesn't make sense, especially if the underlying types are different

In the legacy clients it was done like this too, we have a ticket to make sure that Hits are generic, but with this PR it becomes any, is this wanted ?

Maybe we can have a generic SearchResult for single index search, and a not generic for multi index search ?

millotp
millotp reviewed Sep 11, 2023
View reviewed changes
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

nothing to add except for the generic SearchResult !

@aallam
Copy link
Member Author

aallam commented Sep 11, 2023 •
edited
Loading

Maybe we can have a generic SearchResult for single index search, and a not generic for multi index search ?

@millotp we have exactly that! in fact, there is SearchResponse<T> which remains generic, and there's SearchResult, which is the response used exclusively for multi-searches. In that context, generics don't make sense :D

millotp
millotp approved these changes Sep 11, 2023
View reviewed changes
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

oh okay then it's perfect !

@aallam aallam merged commit d18321c into main Sep 11, 2023
@aallam aallam deleted the refact/java-oneof branch September 11, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millotp millotp millotp approved these changes

@morganleroi morganleroi Awaiting requested review from morganleroi morganleroi is a code owner automatically assigned from algolia/api-clients-automation

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

Successfully merging this pull request may close these issues.
3 participants
@aallam @algolia-bot @millotp