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

feat(java): add requestOptions #487

Merged
merged 5 commits into from
May 11, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package com.algolia.utils;
Copy link
Member Author

@shortcuts shortcuts May 10, 2022

Choose a reason for hiding this comment

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


import java.io.Serializable;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nonnull;

/**
* Request options are used to pass extra parameters, headers, timeout to the request. Parameters
* set in the request option will override default parameter.
*/
public class RequestOptions implements Serializable {
shortcuts marked this conversation as resolved.
Show resolved Hide resolved

private final Map<String, String> headers = new HashMap<String, String>();
private final Map<String, String> queryParams = new HashMap<String, String>();
private Integer timeout = null;

public RequestOptions addExtraHeader(
@Nonnull String key,
@Nonnull String value
) {
headers.put(key, value);
return this;
}

public RequestOptions addExtraQueryParameters(
@Nonnull String key,
@Nonnull String value
) {
queryParams.put(key, value);
return this;
}

public Map<String, String> getExtraHeaders() {
return headers;
}

public Map<String, String> getExtraQueryParams() {
return queryParams;
}

public Integer getTimeout() {
return timeout;
}

public RequestOptions setTimeout(Integer timeout) {
this.timeout = timeout;
return this;
}

@Override
public String toString() {
return (
"RequestOptions{" +
"headers=" +
headers +
", queryParams=" +
queryParams +
'\'' +
'}'
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,13 @@ export function createTransporter({
cacheable: baseRequestOptions?.cacheable,
timeout: baseRequestOptions?.timeout,
queryParameters: {
...baseRequestOptions?.queryParameters,
...methodOptions.queryParameters,
...baseRequestOptions?.queryParameters,
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 wrong, parameters from requestOptions should always override defaults/methods parameters

},
headers: {
Accept: 'application/json',
...baseRequestOptions?.headers,
...methodOptions.headers,
...baseRequestOptions?.headers,
},
};

Expand Down
103 changes: 73 additions & 30 deletions templates/java/libraries/okhttp-gson/ApiClient.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.algolia.utils.Requester;
import com.algolia.exceptions.*;
import com.algolia.utils.UserAgent;
import com.algolia.utils.JSON;
import com.algolia.utils.RequestOptions;

import okhttp3.*;
import okhttp3.internal.http.HttpMethod;
Expand Down Expand Up @@ -286,11 +287,12 @@ public class ApiClient {
* @param queryParams The query parameters
* @param body The request body object
* @param headerParams The header parameters
* @param requestOptions The requestOptions to send along with the query, they will be merged with the transporter requestOptions.
* @return The HTTP call
* @throws AlgoliaRuntimeException If fail to serialize the request body object
*/
public Call buildCall(String path, String method, Map<String, String> queryParams, Object body, Map<String, String> headerParams) throws AlgoliaRuntimeException {
Request request = buildRequest(path, method, queryParams, body, headerParams);
public Call buildCall(String path, String method, Map<String, String> queryParams, Object body, Map<String, String> headerParams, RequestOptions requestOptions) throws AlgoliaRuntimeException {
Request request = buildRequest(path, method, queryParams, body, headerParams, requestOptions);

return requester.newCall(request);
}
Expand All @@ -303,23 +305,32 @@ public class ApiClient {
* @param queryParams The query parameters
* @param body The request body object
* @param headerParams The header parameters
* @param requestOptions The requestOptions to send along with the query, they will be merged with the transporter requestOptions.
* @return The HTTP request
* @throws AlgoliaRuntimeException If fail to serialize the request body object
*/
public Request buildRequest(String path, String method, Map<String, String> queryParams, Object body, Map<String, String> headerParams) throws AlgoliaRuntimeException {
public Request buildRequest(String path, String method, Map<String, String> queryParams, Object body, Map<String, String> headerParams, RequestOptions requestOptions) throws AlgoliaRuntimeException {
headerParams.put("X-Algolia-Application-Id", this.appId);
headerParams.put("X-Algolia-API-Key", this.apiKey);
headerParams.put("Accept", "application/json");
headerParams.put("Content-Type", "application/json");

String contentType = "application/json";
headerParams.put("Accept", contentType);
headerParams.put("Content-Type", contentType);
shortcuts marked this conversation as resolved.
Show resolved Hide resolved

final String url = buildUrl(path, queryParams);

boolean hasRequestOptions = requestOptions != null;
final String url = buildUrl(
path,
queryParams,
hasRequestOptions ? requestOptions.getExtraQueryParams() : null
);
final Request.Builder reqBuilder = new Request.Builder().url(url);
processHeaderParams(headerParams, reqBuilder);

processHeaderParams(
headerParams,
hasRequestOptions ? requestOptions.getExtraHeaders() : null,
reqBuilder
);

RequestBody reqBody;
if (!HttpMethod.permitsRequestBody(method)) {
reqBody = null;
Expand All @@ -343,48 +354,80 @@ public class ApiClient {
*
* @param path The sub path
* @param queryParams The query parameters
* @param extraQueryParams The query parameters, coming from the requestOptions
* @return The full URL
*/
public String buildUrl(String path, Map<String, String> queryParams) {
final StringBuilder url = new StringBuilder();
public String buildUrl(String path, Map<String, String> queryParams, Map<String, String> extraQueryParams) {
StringBuilder url = new StringBuilder();

//The real host will be assigned by the retry strategy
url.append("http://temp.path").append(path);

if (queryParams != null && !queryParams.isEmpty()) {
// support (constant) query string in `path`, e.g. "/posts?draft=1"
String prefix = path.contains("?") ? "&" : "?";
for (Entry<String, String> param : queryParams.entrySet()) {
if (param.getValue() != null) {
if (prefix != null) {
url.append(prefix);
prefix = null;
} else {
url.append("&");
}
String value = parameterToString(param.getValue());
url.append(escapeString(param.getKey())).append("=").append(escapeString(value));
}
url = parseQueryParameters(path, url, queryParams);
url = parseQueryParameters(path, url, extraQueryParams);

return url.toString();
}

/**
* Parses the given map of Query Parameters to a given URL.
*
* @param path The sub path
* @param url The url to add queryParams to
* @param queryParams The query parameters
* @return The URL
*/
public StringBuilder parseQueryParameters(
Copy link
Member Author

Choose a reason for hiding this comment

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

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

String path,
StringBuilder url,
Map<String, String> queryParams
) {
if (queryParams != null && !queryParams.isEmpty()) {
// support (constant) query string in `path`, e.g. "/posts?draft=1"
String prefix = path.contains("?") ? "&" : "?";
for (Entry<String, String> param : queryParams.entrySet()) {
if (param.getValue() != null) {
if (prefix != null) {
url.append(prefix);
prefix = null;
} else {
url.append("&");
}
Comment on lines +364 to 369
Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like...

Suggested change
if (prefix != null) {
url.append(prefix);
prefix = null;
} else {
url.append("&");
}
url.append(prefix);
if (prefix.equals("?")) {
prefix = "&";
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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

String value = parameterToString(param.getValue());
url
.append(escapeString(param.getKey()))
.append("=")
.append(escapeString(value));
}
}
}

return url.toString();
return url;
}

/**
* Set header parameters to the request builder, including default headers.
*
* @param headerParams Header parameters in the form of Map
* @param extraHeaderParams Header parameters in the form of Map, coming from RequestOptions
* @param reqBuilder Request.Builder
*/
public void processHeaderParams(Map<String, String> headerParams, Request.Builder reqBuilder) {
public void processHeaderParams(Map<String, String> headerParams, Map<String, String> extraHeaderParams, Request.Builder reqBuilder) {
for (Entry<String, String> param : headerParams.entrySet()) {
reqBuilder.header(param.getKey(), parameterToString(param.getValue()));
reqBuilder.header(param.getKey(), parameterToString(param.getValue()));
}
for (Entry<String, String> header : defaultHeaderMap.entrySet()) {
if (!headerParams.containsKey(header.getKey())) {
reqBuilder.header(header.getKey(), parameterToString(header.getValue()));
}
if (!headerParams.containsKey(header.getKey())) {
reqBuilder.header(header.getKey(), parameterToString(header.getValue()));
}
}
if (extraHeaderParams != null) {
for (Entry<String, String> header : extraHeaderParams.entrySet()) {
reqBuilder.header(
header.getKey(),
parameterToString(header.getValue())
);
}
}
}

Expand Down
19 changes: 14 additions & 5 deletions templates/java/libraries/okhttp-gson/api.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {{modelPackage}}.*;
import com.algolia.exceptions.*;
import com.algolia.utils.retry.CallType;
import com.algolia.utils.retry.StatefulHost;
import com.algolia.utils.RequestOptions;

import java.util.EnumSet;
import java.util.Random;
Expand Down Expand Up @@ -99,6 +100,7 @@ public class {{classname}} extends ApiClient {
/**
* {{&notes}}{{#allParams}}
* @param {{paramName}} {{&description}}{{#required}} (required){{/required}}{{^required}} (optional{{^isContainer}}{{#defaultValue}}, default to {{.}}{{/defaultValue}}{{/isContainer}}){{/required}}{{/allParams}}{{#returnType}}
* @param requestOptions The requestOptions to send along with the query, they will be merged with the transporter requestOptions.
* @return {{.}}{{/returnType}}
* @throws AlgoliaRuntimeException If fail to call the API, e.g. server error or cannot deserialize the response body
{{#isDeprecated}}
Expand All @@ -112,20 +114,27 @@ public class {{classname}} extends ApiClient {
{{#isDeprecated}}
@Deprecated
{{/isDeprecated}}
public {{#returnType}}{{{.}}} {{/returnType}}{{^returnType}}void {{/returnType}}{{operationId}}({{#allParams}}{{{dataType}}} {{paramName}}{{^-last}}, {{/-last}}{{/allParams}}) throws AlgoliaRuntimeException {
return LaunderThrowable.await({{operationId}}Async({{#allParams}}{{paramName}}{{^-last}}, {{/-last}}{{/allParams}}));
public {{#returnType}}{{{.}}} {{/returnType}}{{^returnType}}void {{/returnType}}{{operationId}}({{#allParams}}{{{dataType}}} {{paramName}}, {{/allParams}}RequestOptions requestOptions) throws AlgoliaRuntimeException {
return LaunderThrowable.await({{operationId}}Async({{#allParams}}{{paramName}}, {{/allParams}}requestOptions));
}

{{! this case handle all the both optional parameters and `requestOptions` }}
{{#optionalParams.0}}
public {{#returnType}}{{{.}}} {{/returnType}}{{^returnType}}void {{/returnType}}{{operationId}}({{#requiredParams}}{{{dataType}}} {{paramName}}{{^-last}}, {{/-last}}{{/requiredParams}}) throws AlgoliaRuntimeException {
{{#returnType}}return {{/returnType}}this.{{operationId}}({{#requiredParams}}{{paramName}}{{^-last}},{{/-last}}{{/requiredParams}}{{#requiredParams.0}},{{/requiredParams.0}}{{#optionalParams}}null{{^-last}},{{/-last}}{{/optionalParams}});
{{#returnType}}return {{/returnType}}this.{{operationId}}({{#requiredParams}}{{paramName}}{{^-last}}, {{/-last}}{{/requiredParams}}{{#requiredParams.0}},{{/requiredParams.0}}{{#optionalParams}}null{{^-last}},{{/-last}}{{/optionalParams}}{{#optionalParams.0}},{{/optionalParams.0}} null);
shortcuts marked this conversation as resolved.
Show resolved Hide resolved
}
{{/optionalParams.0}}

{{! This case only sets `requestOptions` as optional }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

public {{#returnType}}{{{.}}} {{/returnType}}{{^returnType}}void {{/returnType}}{{operationId}}({{#allParams}}{{{dataType}}} {{paramName}}{{^-last}}, {{/-last}}{{/allParams}}) throws AlgoliaRuntimeException {
{{#returnType}}return {{/returnType}}this.{{operationId}}({{#allParams}}{{paramName}}, {{/allParams}}null);
}

/**
* (asynchronously)
* {{notes}}{{#allParams}}
* @param {{paramName}} {{{description}}}{{#required}} (required){{/required}}{{^required}} (optional{{^isContainer}}{{#defaultValue}}, default to {{.}}{{/defaultValue}}{{/isContainer}}){{/required}}{{/allParams}}
* @param requestOptions The requestOptions to send along with the query, they will be merged with the transporter requestOptions.
* @return The awaitable future
* @throws AlgoliaRuntimeException If fail to process the API call, e.g. serializing the request body object
{{#isDeprecated}}
Expand All @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean all optionals or just the requestOptions part?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

The exact same as sync function, that mean:

  • all params
  • all params + requestOptions
  • only required params + 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.

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ok so sync and async function would need:

  • all params
  • all params - requestOptions
  • requiredParams + requestOptions
  • requiredParams

?

Copy link
Collaborator

@millotp millotp May 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The cases above are reflected here: ad0ff62

{{#allParams}}{{#required}}
if ({{paramName}} == null) {
throw new AlgoliaRuntimeException("Missing the required parameter '{{paramName}}' when calling {{operationId}}(Async)");
Expand Down Expand Up @@ -176,7 +185,7 @@ public class {{classname}} extends ApiClient {
}

{{/headerParams}}
Call call = this.buildCall(requestPath, "{{httpMethod}}", queryParams, bodyObj, headers);
Call call = this.buildCall(requestPath, "{{httpMethod}}", queryParams, bodyObj, headers, requestOptions);
Type returnType = new TypeToken<{{{returnType}}}>() {}.getType();
return this.executeAsync(call, returnType);
}
Expand Down