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

[kotlin] Fix NPE for POST/PUT/PATCH with empty request models. #7629

Merged
merged 2 commits into from
Feb 18, 2018

Conversation

kgilmer
Copy link
Contributor

@kgilmer kgilmer commented Feb 9, 2018

…agger-codegen generates clients from Api models without bodies for these methods.

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 and ./bin/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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • [X Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This change prevents an NPE in the case that a POST/PUT/PATCH request did not specify a model. The Java client allows empty models, so I believe this is a valid change.

See also: square/okhttp#751

This is a resubmission of #7626 due to an issue w/ email address associated with commit.

Testing:

I've consumed my forked repo branch via jitpack on an internal project and have used the feature successfully.
Unit tests pass.

CC @jimschubert

@kgilmer kgilmer mentioned this pull request Feb 9, 2018
3 tasks
@wing328 wing328 added this to the v2.4.0 milestone Feb 10, 2018
@jimschubert
Copy link
Contributor

@kgilmer Thanks. We obviously don't want to prevent people from using the client, so it makes sense to support this use case.

I have a question, though. What is the reason for dropping content-type in the fallback of the elvis operator? Is this required, or can it be RequestBody.create(null, contentType)? I ask because, similar to the discussion in the linked okhttp thread, there are plenty of APIs that rely on content negotiation and I'd worry that emptying the content type would result in a 404 or otherwise not touching the expected endpoint.

@kgilmer
Copy link
Contributor Author

kgilmer commented Feb 12, 2018

@jimschubert at the time it seemed that a message of 0 size would not have a content-type (by definition) but I see your point. I can update it to pass the content-type.

@jimschubert
Copy link
Contributor

@kgilmer yeah I don't think we can solve every edge case. And for those in which an empty content type is preferable, the template can always be customized. I know for the APIs that I regularly use (ASP.NET, Finatra, some Spring) the content types hello define our endpoints. The empty body parsing would be affected by the serializers in use on the server.

@kgilmer
Copy link
Contributor Author

kgilmer commented Feb 13, 2018

I am not sure about the ci/circleci check failing. Is this something I need to look into? Are there other items to be addressed?

@wing328
Copy link
Contributor

wing328 commented Feb 18, 2018

@kgilmer the CircleCI test failure was due to maven repo throttling the connection to download the dependencies (the project was too active :) ). I've restarted the job and the all tests look good now.

@wing328 wing328 merged commit 39fa375 into swagger-api:master Feb 18, 2018
@wing328
Copy link
Contributor

wing328 commented Feb 18, 2018

@kgilmer thanks for the PR, which has been merged into master. (sorry for the delay in merging due to Chinese New Year celebration)

@kgilmer
Copy link
Contributor Author

kgilmer commented Feb 18, 2018 via email

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