feat(java): add `requestOptions` #487

shortcuts · 2022-05-10T12:29:36Z

🧭 What and Why

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

Changes included:

This PR adds the requestOptions parameter to every methods of the Java client.

The method is already present in the JavaScript client, but there's a small fix in this PR:

requestOptions headers and query parameters should always override default/method parameters.

The CTS did not changed, I'll add tests for this behavior in my next PR for all clients.

🧪 Test

CI :D

netlify · 2022-05-10T12:29:41Z

✅ Deploy Preview for api-clients-automation canceled.

Name	Link
🔨 Latest commit	`fc4ce88`
🔍 Latest deploy log	https://app.netlify.com/sites/api-clients-automation/deploys/627b853c5528e500087422df

algolia-bot · 2022-05-10T12:30:34Z

✗ The generated branch has been deleted.

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

shortcuts · 2022-05-10T12:31:04Z

...asearch-client-java-2/algoliasearch-core/src/main/java/com/algolia/utils/RequestOptions.java

@@ -0,0 +1,63 @@
+package com.algolia.utils;


This file copied from the current client implementation: https://github.com/algolia/algoliasearch-client-java-2/blob/master/algoliasearch-core/src/main/java/com/algolia/search/models/RequestOptions.java

shortcuts · 2022-05-10T12:31:35Z

.../algoliasearch-client-javascript/packages/client-common/src/transporter/createTransporter.ts

        ...methodOptions.queryParameters,
+        ...baseRequestOptions?.queryParameters,


This was wrong, parameters from requestOptions should always override defaults/methods parameters

shortcuts · 2022-05-10T12:32:07Z

templates/java/libraries/okhttp-gson/ApiClient.mustache

+     * @param queryParams The query parameters
+     * @return The URL
+     */
+    public StringBuilder parseQueryParameters(


As the logic was duplicated for both map I've used this, it seems to work but not a fan of the implem

shortcuts · 2022-05-10T12:32:41Z

tests/CTS/methods/requests/templates/java/requests.mustache

@@ -13,7 +13,6 @@ import org.junit.jupiter.api.BeforeAll;
 import com.google.gson.reflect.TypeToken;

 import com.algolia.JSON;
-import com.algolia.Pair;


I forgot to remove this one earlier

eunjae-lee · 2022-05-10T14:17:53Z

templates/java/libraries/okhttp-gson/ApiClient.mustache

+            if (prefix != null) {
+              url.append(prefix);
+              prefix = null;
+            } else {
+              url.append("&");
            }


What about something like...

Suggested change

if (prefix != null) {

url.append(prefix);

prefix = null;

} else {

url.append("&");

}

url.append(prefix);

if (prefix.equals("?")) {

prefix = "&";

}

I copied the actual logic to avoid introducing unrelated changes, I don't really know the edge cases possible here D:

eunjae-lee

This looks good to me. The suggestion in the parseQueryParameters is not a blocker.

millotp

Nice ! I have a few comments mostly about usage

...asearch-client-java-2/algoliasearch-core/src/main/java/com/algolia/utils/RequestOptions.java

templates/java/libraries/okhttp-gson/ApiClient.mustache

templates/java/libraries/okhttp-gson/api.mustache

millotp · 2022-05-11T07:58:48Z

templates/java/libraries/okhttp-gson/api.mustache

  }
  {{/optionalParams.0}}

+  {{! This case only sets `requestOptions` as optional }}


I don't think you need both, you can just keep the first one but always generate it, and if someone want's to provide requestOptions, they have to provide null for all the optional params before, I'll let you choose which one best suits the customer.

I think both with #487 (comment) are fine to keep, it's a bit verbose but a pretty good addition

millotp · 2022-05-11T08:00:47Z

templates/java/libraries/okhttp-gson/api.mustache

@@ -139,7 +148,7 @@ public class {{classname}} extends ApiClient {
  {{#isDeprecated}}
  @Deprecated
  {{/isDeprecated}}
-  public CompletableFuture<{{{returnType}}}> {{operationId}}Async({{#allParams}}{{{dataType}}} {{paramName}}{{^-last}},{{/-last}} {{/allParams}}) throws AlgoliaRuntimeException {
+  public CompletableFuture<{{{returnType}}}> {{operationId}}Async({{#allParams}}{{{dataType}}} {{paramName}}, {{/allParams}}RequestOptions requestOptions) throws AlgoliaRuntimeException {


If the async functions are widely used, maybe it would be worth it to also gave them alternative with optionals

Do you mean all optionals or just the requestOptions part?

for requestOptions here

The exact same as sync function, that mean:

all params

all params + requestOptions

only required params + requestOptions

Ah oki so I'm missing the last one for the async here

Maybe we could also have only required alone actually, it makes more sense

ok so sync and async function would need:

all params

all params - requestOptions

requiredParams + requestOptions

requiredParams

?

sounds good ! you thinks it's okay or too much ?

I think it's good, we could even have more cases but it should be enough

The cases above are reflected here: ad0ff62

…s async

millotp · 2022-05-11T09:09:26Z

templates/java/libraries/okhttp-gson/ApiClient.mustache

+          (byte[]) obj,
+          MediaType.parse(this.contentType)
+        );
+      } else if (isJsonMime(this.contentType)) {


this if is not relevant anymore as we already know the mime type, at least for the serialization

Sorry I did not saw that fc4ce88

millotp

Looks good ! Thanks !

shortcuts requested review from millotp and a team May 10, 2022 12:29

shortcuts self-assigned this May 10, 2022

shortcuts requested review from eunjae-lee and removed request for a team May 10, 2022 12:29

shortcuts commented May 10, 2022

View reviewed changes

shortcuts mentioned this pull request May 10, 2022

chore: fix specs run condition #488

Merged

eunjae-lee reviewed May 10, 2022

View reviewed changes

eunjae-lee previously approved these changes May 10, 2022

View reviewed changes

shortcuts mentioned this pull request May 10, 2022

chore: fix codegen restore #491

Merged

feat(java): add requestOptions

d416640

shortcuts dismissed eunjae-lee’s stale review via d416640 May 10, 2022 15:53

shortcuts force-pushed the feat/requestOptions branch from 52fc6e9 to d416640 Compare May 10, 2022 15:53

Merge branch 'main' into feat/requestOptions

90266ee

shortcuts requested a review from eunjae-lee May 11, 2022 08:01

millotp reviewed May 11, 2022

View reviewed changes

move redundant logic to API constructor, allow optional requestOption…

9345dd9

…s async

shortcuts requested a review from millotp May 11, 2022 08:47

millotp reviewed May 11, 2022

View reviewed changes

more optional cases

ad0ff62

shortcuts requested a review from millotp May 11, 2022 09:33

remove unused content type checking

fc4ce88

millotp approved these changes May 11, 2022

View reviewed changes

shortcuts merged commit c5ac185 into main May 11, 2022

shortcuts deleted the feat/requestOptions branch May 11, 2022 10:03

millotp pushed a commit that referenced this pull request May 11, 2022

feat(java): add requestOptions (#487)

8002674

This was referenced May 19, 2022

chore: release 2022-05-19 #518

Closed

chore: release 2022-05-19 #520

Closed

shortcuts mentioned this pull request May 23, 2022

chore: release 2022-05-23 #533

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(java): add `requestOptions` #487

feat(java): add `requestOptions` #487

shortcuts commented May 10, 2022

netlify bot commented May 10, 2022 •

edited

Loading

algolia-bot commented May 10, 2022 •

edited

Loading

shortcuts May 10, 2022 •

edited

Loading

shortcuts May 10, 2022

shortcuts May 10, 2022

shortcuts May 10, 2022

eunjae-lee May 10, 2022

shortcuts May 10, 2022

eunjae-lee left a comment

millotp left a comment

millotp May 11, 2022

shortcuts May 11, 2022

millotp May 11, 2022

shortcuts May 11, 2022

shortcuts May 11, 2022

millotp May 11, 2022

shortcuts May 11, 2022

millotp May 11, 2022

shortcuts May 11, 2022

millotp May 11, 2022 •

edited

Loading

shortcuts May 11, 2022

shortcuts May 11, 2022

millotp May 11, 2022

millotp May 11, 2022

shortcuts May 11, 2022

millotp left a comment

		...methodOptions.queryParameters,
		...baseRequestOptions?.queryParameters,

feat(java): add requestOptions #487

feat(java): add requestOptions #487

Conversation

shortcuts commented May 10, 2022

🧭 What and Why

Changes included:

🧪 Test

netlify bot commented May 10, 2022 • edited Loading

✅ Deploy Preview for api-clients-automation canceled.

algolia-bot commented May 10, 2022 • edited Loading

✗ The generated branch has been deleted.

shortcuts May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunjae-lee left a comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

feat(java): add `requestOptions` #487

feat(java): add `requestOptions` #487

netlify bot commented May 10, 2022 •

edited

Loading

algolia-bot commented May 10, 2022 •

edited

Loading

shortcuts May 10, 2022 •

edited

Loading

millotp May 11, 2022 •

edited

Loading