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

Use warning instead of throwing exceptions #9188

Merged
merged 2 commits into from
Apr 13, 2021
Merged

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Apr 6, 2021

Currently, showing warnings instead of throwing exceptions in case the spec is not perfect.

We're already doing something similar in other part of the codegen, e.g. https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L2025

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.

cc @OpenAPITools/generator-core-team

@wing328 wing328 marked this pull request as draft April 6, 2021 16:17
@wing328
Copy link
Member Author

wing328 commented Apr 6, 2021

@spacether please review when you've time and let me know if you're ok with this change.

The goal is to prevent one little issue from stopping the code generation to proceed with the rest of the spec.

@spacether
Copy link
Contributor

spacether commented Apr 6, 2021

Adding this can help people debug their specs.
My main concern is that these warnings indicate that their discriminators will definitely not work if these warning are logged.
Why not log all warnings then stop generation before model/api/docs generation?

If we definitely want to do warnings only and ignore that these specs will have malfunctioning discriminators
are there validation implications to this change?
If possible it would be great if the spec did not pass validation if any of these warnings were thrown. What do you think?

@wing328
Copy link
Member Author

wing328 commented Apr 7, 2021

Why not log all warnings then stop generation before model/api/docs generation?

Consider the use case in which the spec has thousand of endpoints and models. One (and only one) of the model has an incorrect discriminator value in the spec. The current behaviour is that an exception is thrown while this PR proposes logging a warning instead of throwing runtime exception.

Imagine a user (not the owner of the spec) takes the spec and try to generate a client. He/She is only interested in 1 endpoint which references one model that has nothing to do with discriminator. Currently, he/she will get an exception while this PR will let him/her at least get something that hopefully still works (or may need some minor tweaks to remove errors related to the incorrect discriminator).

Try to search for logger.warn in the code base and you will see us show a lot warnings instead of throwing an exception. We do encourage users to fix the incorrect input.

If possible it would be great if the spec did not pass validation if any of these warnings were thrown. What do you think?

Currently, we already allow invalid spec to complete code generation to at least output something (sometimes people just want to try to see what OpenAPI Generator can produce)

@spacether
Copy link
Contributor

Thank you for sharing that context. The warnings sound good to me.

@wing328 wing328 marked this pull request as ready for review April 13, 2021 06:08
@wing328 wing328 merged commit 13c0b2c into master Apr 13, 2021
@wing328 wing328 deleted the discriminator-warning branch April 13, 2021 06:09
@wing328 wing328 added this to the 5.1.1 milestone Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants