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] [Native] Add response body to exception message #9169

Merged
merged 26 commits into from
Apr 16, 2021

Conversation

MosheElisha
Copy link
Contributor

@MosheElisha MosheElisha commented Apr 4, 2021

Concatenate the response body to the exception message in case of unsuccessful response.

Implement the simple approach described in issue #8759

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

…ss only when generateSupportingFiles is true
…ss only when generateSupportingFiles is true
…ss only when generateSupportingFiles is true
…JSON class only when generateSupportingFiles is true"

This reverts commit 56e2b1f
…JSON class only when generateSupportingFiles is true"

This reverts commit 335c304
…ss only when generateSupportingFiles is true
@MosheElisha
Copy link
Contributor Author

@bgong-mdsol @bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @nmuesch @spacether @jimschubert mentioning the Java technical committee.

@wing328 I have implemented the simple approach as we discussed.

Please review. Thanks in advance.

@agilob
Copy link
Contributor

agilob commented Apr 5, 2021

Can you add a test for it?

@MosheElisha
Copy link
Contributor Author

Can you add a test for it?

@agilob Sure. I am familiar with tests that verify the template was generated as expected (e.g., JavaClientCodegenTest) but I felt that because there is no condition in the template, the generated samples are enough for that.

Are there tests that test the client functionality so I can test the thrown exception? I would be happy to add a test there.
If not, please let me know what kind of test you are looking for and where is best to add them.

Thanks!

@agilob
Copy link
Contributor

agilob commented Apr 5, 2021

I was thinking about test of createApiException to make sure it returns the format you expect with data you expect. Templates evolve quite fast and that behaviour you added here might change or seem irrelevant to future contributors.

@MosheElisha
Copy link
Contributor Author

I was thinking about test of createApiException to make sure it returns the format you expect with data you expect. Templates evolve quite fast and that behaviour you added here might change or seem irrelevant to future contributors.

@agilob Sure. Where can I add such a test? The code of createApiException is only in the template and auto generated in samples files so I'm not sure how to create such a test. Can you point me to a place where similar tests already exist?

@agilob
Copy link
Contributor

agilob commented Apr 5, 2021

Some of the samples will be generated to directories with pre-existing manually written code which simply imports generated code and tests it. Unfortunately it's not easy to find them (or I dont know how) so I delete all generated code eg. samples/ directory, regenerate all samples and check git status for which files were removed but not regenerated... If no such sample exists, you can add a new directory with tests inside generated sample project.

I've seen this approach to testing in typescript and we do it in Dart.

@MosheElisha
Copy link
Contributor Author

Thanks, @agilob . I was unable to find such tests for Java. To create such a test for Java, I believe I need to make the generated code available as a maven module before I can create a test that uses it.

@wing328 Please advise.

@wing328
Copy link
Member

wing328 commented Apr 16, 2021

Agreed it's not easy to test. Let's go with what you've so far.

@wing328 wing328 merged commit af992e4 into OpenAPITools:master Apr 16, 2021
@Gama11
Copy link
Contributor

Gama11 commented Jun 7, 2021

There's another method that creates API exceptions in this template (getApiException() within the asyncNative block). It seems like you forgot to update its message to the new format?

@MosheElisha
Copy link
Contributor Author

There's another method that creates API exceptions in this template (getApiException() within the asyncNative block). It seems like you forgot to update its message to the new format?

Yes, I missed that. Thank you. I don't have the time to do it now. If it is important for you, please go ahead. Thanks.

Gama11 added a commit to Gama11/openapi-generator that referenced this pull request Jun 22, 2021
The template has two methods for creating API exceptions, and the enhancements from OpenAPITools#9169 didn't make it to the async version.

- unify the signatures of the two methods (name, arguments)
- make sure the sync version is not generated with asyncNative
- extract the formatting logic into a common formatExceptionMessage() method
- add the status code to the exception message as well, not just the body
- shortened "call received non-success response" to a more concise "call failed with"
@Gama11
Copy link
Contributor

Gama11 commented Jun 22, 2021

I created a follow-up PR here, along with a bit of cleanup and also adding the status code to the exception message: #9825

wing328 pushed a commit that referenced this pull request Jul 5, 2021
…de (#9825)

* [Java] [Native] Unify exception messages for async, add the status code

The template has two methods for creating API exceptions, and the enhancements from #9169 didn't make it to the async version.

- unify the signatures of the two methods (name, arguments)
- make sure the sync version is not generated with asyncNative
- extract the formatting logic into a common formatExceptionMessage() method
- add the status code to the exception message as well, not just the body
- shortened "call received non-success response" to a more concise "call failed with"

* Treat an empty body the same as a null body

Co-authored-by: Jens Fischer <[email protected]>
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.

None yet

4 participants