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(java): use Map for query parameters #484

Merged
merged 5 commits into from
May 10, 2022
Merged

Conversation

shortcuts
Copy link
Member

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-471

Changes included:

While implementing the requestOptions for Java, I've found that the type of the Query Params were different than the current clients.

This PR updates the type of the query params and their related utils/methods/tests.

Also in this PR:

  • Add missing requestOption JSDoc description for the JS client to the template
  • Move headers that are common to all methods to the transporter

🧪 Test

CI :D

@shortcuts shortcuts requested a review from a team May 9, 2022 14:26
@shortcuts shortcuts self-assigned this May 9, 2022
@shortcuts shortcuts requested review from eunjae-lee and damcou and removed request for a team May 9, 2022 14:26
@netlify
Copy link

netlify bot commented May 9, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 3f2680c
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/627a35cccde7050008f96eed

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 9, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

Copy link
Member Author

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Comment on lines +558 to +559
headerParams.put("Accept", "application/json");
headerParams.put("Content-Type", "application/json");
Copy link
Member Author

Choose a reason for hiding this comment

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

Those were in every methods so I've moved them here

Comment on lines -564 to -567
// ensuring a default content type
if (contentType == null) {
contentType = "application/json";
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we set it right above, it can't be null

@@ -149,6 +149,7 @@ export function create{{capitalizedApiName}}(options: CreateClientOptions{{#hasR
{{/queryParams.0}}
{{/bodyParams.0}}
{{/allParams.0}}
* @param requestOptions - The requestOptions to send along with the query, they will be merged with the transporter requestOptions.
Copy link
Member Author

Choose a reason for hiding this comment

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

This was missing in the JS client

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.

Cool ! Looks perfect, you can definitely remove the unused methods in ApiClient.java and the Pair.java file (either with the custom generator or with openapi-generator-ignore

damcou
damcou previously approved these changes May 10, 2022
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Was the use of Pairs a choice from us or was it by default in the Java template ?

(I trust your judgment for this change since I didn't even know about this type in Java)

@shortcuts
Copy link
Member Author

Was the use of Pairs a choice from us or was it by default in the Java template ?

IIRC yes it was like this in the base template, we've iterate on that but after discussing with Pierre we thought it made more sense to make it match today's type

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.

Love it !

@shortcuts shortcuts merged commit 949c3f2 into main May 10, 2022
@shortcuts shortcuts deleted the fix/java-query-parameters branch May 10, 2022 10:08
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.

4 participants