Skip to content
This repository was archived by the owner on Jun 30, 2023. It is now read-only.

check non-nullness of required parameters#148

Merged
tangiel merged 3 commits intomasterfrom
required-param
Jun 11, 2018
Merged

check non-nullness of required parameters#148
tangiel merged 3 commits intomasterfrom
required-param

Conversation

@tangiel
Copy link
Contributor

@tangiel tangiel commented Jun 8, 2018

@nAmed parameters that don't have @nullable parameters are deemed
required. Endpoints v2 lost this request validation, but this change
adds it back in. Previously, the framework would assume the validation
was done.

@nAmed parameters that don't have @nullable parameters are deemed
required. Endpoints v2 lost this request validation, but this change
adds it back in. Previously, the framework would assume the validation
was done.
@tangiel tangiel requested review from inklesspen and kryzthov June 8, 2018 23:04
@codecov-io
Copy link

codecov-io commented Jun 8, 2018

Codecov Report

Merging #148 into master will increase coverage by 0.06%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #148      +/-   ##
============================================
+ Coverage     80.01%   80.08%   +0.06%     
- Complexity     1678     1726      +48     
============================================
  Files           156      156              
  Lines          5595     5694      +99     
  Branches        730      766      +36     
============================================
+ Hits           4477     4560      +83     
- Misses          840      844       +4     
- Partials        278      290      +12
Impacted Files Coverage Δ Complexity Δ
.../server/spi/request/ServletRequestParamReader.java 83.59% <66.66%> (-0.41%) 28 <2> (+4)
...rver/spi/config/validation/ApiConfigValidator.java 79.33% <0%> (+2.76%) 109% <0%> (+44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d41d772...aab3f07. Read the comment docs.

Copy link

@kryzthov kryzthov left a comment

Choose a reason for hiding this comment

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

Looks good, just one nitpick question.

JsonNode nodeValue = node.get(name);
if (nodeValue == null) {
params[i] = null;
if (StandardParameters.isStandardParamName(name)) {

Choose a reason for hiding this comment

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

Any reason to turn the else if/else into a nested else { if/else }?
It looks strictly equivalent, am I misreading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just so that the null check can exist in the newly created else block, rather than re-checking for parameter name existence. Everything else is equivalent.

@tangiel tangiel merged commit b4984ab into master Jun 11, 2018
@tangiel tangiel deleted the required-param branch June 11, 2018 23:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants