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][okhttp-gson] Fix incorrect use of OkHttp interceptors #2356

Merged
merged 1 commit into from
Mar 12, 2019
Merged

[Java][okhttp-gson] Fix incorrect use of OkHttp interceptors #2356

merged 1 commit into from
Mar 12, 2019

Conversation

burberius
Copy link
Contributor

@burberius burberius commented Mar 10, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Port of @ngaya-ll 's pull request from swagger to openapi.
See swagger-api/swagger-codegen#8053

Currently, the generated Java okhttp-gson client adds an interceptor to the underlying OkHttpClient for each async call. The purpose of the interceptor is to wrap the response body to track download progress. This implementation doesn't work correctly, for multiple reasons:

The interceptor intercepts all requests to the client, not just the one it's trying to track.
Interceptors are never removed from the client, so each async request adds another layer of interception for all subsequent requests. Since the interceptor chain is invoked recursively, at some point all requests will start failing with a StackOverflowError.
With this code change, the client uses a single interceptor to decorate all async requests and send updates to the relevant listener only.

I also added a test case to demonstrate the issue, which fails on the current master and passes on this branch.

On master the petshop generation is broken, so I couldn't produce the samples and run the tests!

Reviewers: @bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger

Thanks @ngaya-ll for the original work.

GoldenGnu added a commit to GoldenGnu/eve-esi that referenced this pull request Mar 11, 2019
Generated with Burberius's openapi-generator
OpenAPITools/openapi-generator#2356
burberius pushed a commit to burberius/eve-esi that referenced this pull request Mar 11, 2019
* Generated with openapi-gnerator 3.3.4

Now include meta specs on generation

* Fixed verify not adding auth

* Update SsoApi.java

* user-agent

* Added CharacterInfo.getExpiresOnDate()

* Updated openapi-generator

Generated with Burberius's openapi-generator
OpenAPITools/openapi-generator#2356
@wing328 wing328 added this to the 4.0.0 milestone Mar 12, 2019
@wing328
Copy link
Member

wing328 commented Mar 12, 2019

@burberius thanks for the PR. I notice that the additional tests added by @burberius are not yet included in this PR. I'll submit another PR to add the tests instead.

I tested locally with the new test cases and all tests passed.

Also thanks @ngaya-ll for the original work.

@wing328 wing328 merged commit fde3252 into OpenAPITools:master Mar 12, 2019
@wing328 wing328 changed the title Port of @ngaya-ll 's pull request from swagger to openapi. [Java][okhttp-gson] Fix incorrect use of OkHttp interceptors Mar 12, 2019
burberius added a commit to burberius/eve-esi that referenced this pull request Mar 17, 2019
* Line ending changes

Always use lf line ending for *.sed and *.sh files. Use default for everything else.

* More line ending stuff

* Final line ending change

* Started on issue 78

* Better tests

less failing

* Better tests

less failing (again)

* More work on issue 78

* Issue 78

Better error handling

* Issue 78 formatting

* Less sync blocks

* First commit switching to okhttp

Status
-Compiles and no test failures

Known Issues:
-soApi.revokeRefreshToken() and SsoApi.revokeAccessToken() does nothing.
-SsoApi.getCharacterInfo() is just a wrapper for MetaApi.getVerify()

* More work on okhttp

Known issues:
VerifyResponse.getExpiresOn() return a String should be Java8 Date format?

* swagger codegen 2.3.1

* formatting

* Bug Fix for OAuth

Bug: Code allow you to match a refresh token with just a clientID

* Ok http (#83)

* Generated with openapi-gnerator 3.3.4

Now include meta specs on generation

* Fixed verify not adding auth

* Update SsoApi.java

* user-agent

* Added CharacterInfo.getExpiresOnDate()

* Updated openapi-generator

Generated with Burberius's openapi-generator
OpenAPITools/openapi-generator#2356

* Added fixed openapi-generator.

* Added new openapi generator and made script executable.

* Update ApiClient.java

Use the latest version of Burberius openapi-generator-cli

* 2019-03-11 esi release

2019-03-11 esi release

Promoted
/v3/universe/names/ (now resolve faction ids)

* ApiClientBuilder (#85)

* ApiClientBuilder

* little more work on the builder

* Okhttp fixes (#89)

* Fixed SsoAuth main

* Updated CharacterInfo  and SsoApi

-Updated CharacterInfo with better method names + tests
-SsoApi commen

* Update README.md  (#88)

* ApiClientBuilder

* little more work on the builder

* Update README.md

-Removed client secret (no longer used)
-Updated link to MarketApiTest, ESI, and SSO

* Formated pom.xml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants