-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[JAVA][NATIVE] Add gzip capability #22358
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
Conversation
|
thanks for the pr please follow step 3 to update the samples so that CI can verify the change. |
| protected Consumer<HttpRequest.Builder> interceptor; | ||
| protected Consumer<HttpResponse<InputStream>> responseInterceptor; | ||
| protected Consumer<HttpResponse<String>> asyncResponseInterceptor; | ||
| protected Consumer<HttpResponse<InputStream>> asyncResponseInterceptor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it correct to say that this is a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically yes, although I would expect most users to never use this method directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that responseInterceptor uses InputStream, we can consider this change a bug fix instead.
modules/openapi-generator/src/main/resources/Java/libraries/native/ApiClient.mustache
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/Java/libraries/native/ApiClient.mustache
Show resolved
Hide resolved
|
@martin-mfg @lwlee2608 @welandaz @joschi can you please review? |
|
Fixes issue #22326 |
| import java.util.StringJoiner; | ||
| import java.util.function.Consumer; | ||
| import java.util.Optional; | ||
| import java.util.zip.GZIPInputStream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this import for java.util.zip.GZIPInputStream only needed when useGzipFeature is enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll fix it with another PR.
thanks for the contribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now i see why as there's a function using it without gzip feature enabled.
public static InputStream getResponseBody(HttpResponse<InputStream> response) throws IOException {
if (response == null) {
return null;
}
InputStream body = response.body();
if (body == null) {
return null;
}
Optional<String> encoding = response.headers().firstValue("Content-Encoding");
if (encoding.isPresent()) {
for (String token : encoding.get().split(",")) {
if ("gzip".equalsIgnoreCase(token.trim())) {
return new GZIPInputStream(body);
}
}
}
return body;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I was also thinking of putting "java.util.zip.GZIPInputStream" behind a guard, but then I thought we should decompress gzip even if we dont request it because some servers respond with gzip compression by default.
|
thanks for the PR did a test with the feature enabled and the output compiles without issue |
|
FYI. Merged #22361 to add new samples (tested in CI) |
Perfect. Thank you! |
Currently, Java's native HttpClient template does not support gzipped responses from the server. This PR adds this capability and uses the existing flag "useGzipFeature" to guard it
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)