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

[java-client][okhttp-gson] fixes for interceptors in ApiClient.java #3502

Merged
merged 2 commits into from Aug 1, 2019

Conversation

@lostiniceland
Copy link
Contributor

commented Jul 30, 2019

ApiClient for Okhttp3 must not copy the interceptors when setting the HttpClient.

fixes #3497

@@ -335,7 +335,7 @@ public void testInterceptorCleanupWithNewClient() {
OkHttpClient oldClient = apiClient.getHttpClient();
assertEquals(1, oldClient.networkInterceptors().size());

OkHttpClient newClient = new OkHttpClient();
OkHttpClient newClient = apiClient.getHttpClient().newBuilder().build();

This comment has been minimized.

Copy link
@jmini

jmini Jul 31, 2019

Member

CircleCI reports that this change gets overridden when the samples are updated:

diff --git a/samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/ApiClientTest.java b/samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/ApiClientTest.java
index 3675d8645a..6181e8afa2 100644
--- a/samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/ApiClientTest.java
+++ b/samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/ApiClientTest.java
@@ -335,7 +335,7 @@ public class ApiClientTest {
         OkHttpClient oldClient = apiClient.getHttpClient();
         assertEquals(1, oldClient.networkInterceptors().size());
 
-        OkHttpClient newClient = apiClient.getHttpClient().newBuilder().build();
+        OkHttpClient newClient = new OkHttpClient();
         apiClient.setHttpClient(newClient);
         assertEquals(1, apiClient.getHttpClient().networkInterceptors().size());
         apiClient.setHttpClient(newClient);

This is because the source for this file is in the CI/samples.ci folder. Can you update this file:

@jmini jmini changed the title fixes OpenAPITools/openapi-generator#3497 [java-client][okhttp-gson] fixes for interceptors in ApiClient.java Jul 31, 2019

@jmini
Copy link
Member

left a comment

@lostiniceland: Thank you a lot for this PR. It looks great.

Can you also change the mentioned file?

fixes #3497
ApiClient for Okhttp3 must not copy the interceptors when setting the HttpClient.
@lostiniceland

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Updated as requested by review.

I would argue though that the Tests I've fixed are not very usefull at all. They might have been with the old "logic" but now the tested method is a plain setter, except the check for equality, which could also be removed.
A usefull invariant would be that the passed client must not be null, because the ApiClient cannot function without a httpClient.

@lostiniceland

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@jmini I've added another commit which contains the mentioned invariant and removed the unnecessary tests

fixes #3497
Enforce invariant that the HttpClient must never be null.
@wing328

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@lostiniceland

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@wing328 thanks for pointing that out. I've added my work-email to Github so it should match now.

@jmini

jmini approved these changes Aug 1, 2019

Copy link
Member

left a comment

LGTM. Thank you for the change.

@jmini jmini merged commit 0a7527b into OpenAPITools:master Aug 1, 2019

5 checks passed

Shippable Run 9547 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wing328 wing328 added the Issue: Bug label Aug 1, 2019

@wing328 wing328 added this to the 4.1.0 milestone Aug 1, 2019

@lostiniceland lostiniceland deleted the lostiniceland:bugfix/issue-3497 branch Aug 1, 2019

smasala added a commit to smasala/openapi-generator that referenced this pull request Aug 1, 2019

[java-client][okhttp-gson] fixes for interceptors in ApiClient.java (O…
…penAPITools#3502)

ApiClient for Okhttp3 must not copy the interceptors when setting the HttpClient.
Enforce invariant that the HttpClient must never be null.
@wing328

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

@lostiniceland thanks for the PR, which has been included in the 4.1.0 release: https://twitter.com/oas_generator/status/1160000504455319553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.