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

[FIX][CLI] handle null result messages #7472

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

codymikol
Copy link
Contributor

@codymikol codymikol commented Sep 21, 2020

swagger-parser has the potential to return null
for messages which will throw NPE on
initialization of the validationMessages hashset.
rather that allow this to happen we will assume
that a null message collection represents an empty
collection and continue.

Fixes #7453

This prevents a null pointer exception when the swagger-parser library returns a null on getMessages(). Rather than instantiate the validation HashSet with null, we will pass an empty list.

In my case I was experiencing this null pointer exception because swagger-parser has a bug which returns null for all fields when parameters are not passed in.

While this does not fix that issue, it allows a much more friendly run time exception to be thrown that states

Exception in thread "main" java.lang.RuntimeException: missing OpenAPI input!

There seems to be a swagger-parser issue for this over here ==> swagger-api/swagger-parser#1428

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.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

swagger-parser has the potential to return null
for messages which will throw NPE on
initialization of the  validationMessages hashset.
rather that allow this to happen we will assume
that a null message collection represents an empty
collection and continue.

Fixes OpenAPITools#7453
@codymikol
Copy link
Contributor Author

This part of the CLI is in java so I am adding the listed technical committee members listed in the contribution guidelines

@bkabrda @lwlee2608 @Zomzog @karismann @jeff9finger @cbornet @lukoyanov @jfiala @sreeshas @bbdouglas

Thank you for your time, your work is much appreciated!

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed your change will result in a better error message.

@wing328 wing328 merged commit 6db283c into OpenAPITools:master Sep 29, 2020
@wing328 wing328 added this to the 5.0.0 milestone Sep 29, 2020
@codymikol
Copy link
Contributor Author

codymikol commented Oct 4, 2020

I had some time to write tests for swagger-parser today to see if I could reproduce this on the swagger-parser end, but it looks like whatever the latest on their origin/master branch is handles these cases appropriately

image

EDIT: It looks like my tests fail on their latest release 2.0.22, but pass on the latest origin/master so this fix is not released yet, hopefully it will be in soon and we can bump to that version

EDIT: I was wrong, it looks like my tests pass in v2.0.22, I'm going to make a PR now to bump the parser version :D

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.

[BUG][CLI] crash when evaluating definitions without parameters
2 participants