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: optional empty validation #7003

Merged
merged 3 commits into from Mar 3, 2021

Conversation

mathis-m
Copy link
Contributor

@mathis-m mathis-m commented Feb 26, 2021

Description

internal logic does send null to validation in case of empty parameter input. This should be included in the validation, This was introduced with:
5c4dfc2 Mahtis Michel 02/03/2021 09:29 PM feat: enhance parameter validation (#6878)

I decided to refactor the validation condition to more readable state.
I think this is even more readable then the comment before.

Motivation and Context

Fixes #6998

How Has This Been Tested?

Screenshots (if appropriate):

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

mathis-m and others added 3 commits February 26, 2021 01:29
I decided to refactor the validation condition to more readable state.
I think this is even more readable then the comment before

Signed-off-by: mathis-m <mathis.michel@outlook.de>
internal logic does send null to validation in case of empty parameter input.

Signed-off-by: mathis-m <mathis.michel@outlook.de>
@tim-lai tim-lai merged commit d32bd1a into swagger-api:master Mar 3, 2021
@tim-lai
Copy link
Contributor

tim-lai commented Mar 3, 2021

@mathis-m PR merged! Thanks for the contribution!

@dferretti
Copy link

I know I'm late to the party, but I think this is relevant to this PR. I believe you can have a non-required string array param in the query, with minItems: 1 to represent "either don't pass it at all, or if you do it must include at least 1 item". Well, I think so anyway..
I know you can have a non-required, non-nullable parameter meaning "either don't provide it, or provide a non-null value, but never provide x: null". So I believe you can additionally constrain the array with minItems: 1 IIRC.

I am trying this in https://editor.swagger.io :

openapi: 3.0.3
info:
  title: Test
  version: 1.0.0
paths:
 /test:
   get:
     parameters: 
       - name: names
         in: query
         required: false
         schema:
           type: array
           minItems: 1
           items:
             type: string
     responses:
       200:
         description: OK

I would like to be able to test omitting the names parameter by not providing any elements, but swagger-ui tells me I must provide at least 1 value.

Should these minItems validation checks include whether or not the param is required? Or am I way off here and the spec doesn't allow that?

@dferretti
Copy link

Actually I may be off on what I'm talking about anyways - I was initially reading this older PR #6878, but poking through this PR I do see some null/required checking going on. Maybe editor.swagger.io is not running this code yet (is there an easy way to see what version it is running?).
Tomorrow I'll try just running swagger-ui locally on this version and see if it still is happening.

@mathis-m
Copy link
Contributor Author

mathis-m commented Mar 24, 2021

@dferretti you are right when there is no value provided and the param is optional there should not be any error.
I had a look at how it is handled for type: string minLength:
https://editor.swagger.io/?url=https://gist.githubusercontent.com/mathis-m/416a5796511d27cc7a6160bc9f9866fa/raw/4c8968e09386b67318c29827831c84994cf86d03/WZOi9WH0TP.txt

openapi: 3.0.3
info:
  title: Test
  version: 1.0.0
paths:
 /test:
   get:
     parameters: 
       - name: names
         in: query
         required: false
         schema:
           type: string
           minLength: 2
     responses:
       200:
         description: OK

It allows optional or minLength of 2 both is valid.

Have moved this to issue #7111.
#7112 will fix this.

@dferretti
Copy link

Awesome thank you!

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.

Query with example turns field into required
3 participants