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

Add x-nullable support for body validators #642

Merged
merged 7 commits into from Sep 17, 2018

Conversation

dmksnnk
Copy link
Contributor

@dmksnnk dmksnnk commented Jul 26, 2018

Closes #439

Add x-nullable support in properties by adding property validator to default Draft4Validator. It skips null-property with "x-nullable": true and then adds it back.

@coveralls
Copy link

coveralls commented Jul 26, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling d248434 on dmksnnk:nullable-in-definitions into 79e0e3d on zalando:master.


def nullable_support(validator, properties, instance, schema):
null_properties = {}
for property_, subschema in six.iteritems(properties):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is breaking the build by not throwing the right exception when the function is given an integer as input.
See the travis build for the failure.

@dtkav
Copy link
Collaborator

dtkav commented Aug 7, 2018

This looks really close - there are some minor changes needed to fix the build, plus a rebase on master. Thanks for putting this together!

from werkzeug.datastructures import MultiDict

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this breaks the isort step in the build. See the end of the python 3.6 travis run for details.

@dmksnnk
Copy link
Contributor Author

dmksnnk commented Aug 7, 2018

Can you please check for Pypy build? It can't download Pypy binaries. Otherwise all other builds are passed.

@dtkav
Copy link
Collaborator

dtkav commented Aug 29, 2018

hey @dmksnnk , sorry this fell off my radar. I kicked the build - seems like the problem was with travis-ci.
You code looks great to me, and I'd love to use this in the dev-2.0 branch for nullable support in openapi 3 validation.

Copy link
Collaborator

@dtkav dtkav left a comment

Choose a reason for hiding this comment

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

Looks great.
Did you consider pulling this into a "connexion/validators.py" file?

@dmksnnk
Copy link
Contributor Author

dmksnnk commented Aug 29, 2018

@dtkav thank you for checking this. What should be my next step? Move it to connexion/validators.py? Or you are just going to use it in next 2.0 branch?

@dtkav
Copy link
Collaborator

dtkav commented Aug 29, 2018

Only zalando employees can merge PRs into master, so we'll have to wait for either @jmcs or @hjacobs to have a look.

I've merged it into dev-2.0 for my own purposes, but I think it would make sense to merge it into master too (so it can get out sooner). I can sort out any rebase/merge conflicts that may come up on the dev-2.0 branch.

Moving the code into another file is totally optional, I just wanted to raise the idea. I don't have a strong preference.

@dtkav
Copy link
Collaborator

dtkav commented Aug 29, 2018

actually, @dmksnnk if you can rebase on master, that would help.

@dtkav
Copy link
Collaborator

dtkav commented Sep 11, 2018

Hey @jmcs - This is looking good from my perspective. I recommend we merge this into master, what do you think?

@jmcs
Copy link
Contributor

jmcs commented Sep 17, 2018

👍

@jmcs jmcs merged commit 1c0bd7d into spec-first:master Sep 17, 2018
@kinokrt
Copy link

kinokrt commented Oct 5, 2018

Isn't this a bug ? Why x-nullable does not work on $ref ? For example:

changes:
      type: object
      properties:
        Bla:
          $ref: "#/definitions/Muhehe"
          x-nullable: true
      additionalProperties:
        $ref: "#/definitions/Muhehe"
        x-nullable: true

@dmksnnk
Copy link
Contributor Author

dmksnnk commented Oct 5, 2018

@jankontrik you need to set x-nullable: true inside referenced object:

Muhehe:
    title: Muhehe
    type: object
    x-nullable: true

@kinokrt
Copy link

kinokrt commented Oct 5, 2018

@dmksnnk Indeed I had that this way (x-nullable: true inside referenced object) but always ends with:
validation error: None is not of type 'object'
So that's why I've tried to define x-nullable alongside $ref.

@kinokrt
Copy link

kinokrt commented Oct 5, 2018

I'm using python 3.6, connexion==1.5.2.

@dmksnnk
Copy link
Contributor Author

dmksnnk commented Oct 5, 2018

if you check releases, this feature was added for version 1.5.3

@kinokrt
Copy link

kinokrt commented Oct 5, 2018

@dmksnnk Yep. Sorry I know about which release. Of course I tried that on 1.5.3 but with the result above :(. Then I returned to 1.5.2 and forgot about the right version.

@dmksnnk
Copy link
Contributor Author

dmksnnk commented Oct 5, 2018

I'm investigating this, but have you considered using {} for empty object instead of None/null?
I'm not sure if it is a good idea to set object to None.
Summoning help @dtkav, @jmcs

@kinokrt
Copy link

kinokrt commented Oct 5, 2018

Of course it is not good I absolutely agree. But this is some legacy stuff, no way to change.

@kinokrt
Copy link

kinokrt commented Oct 5, 2018

BTW the problem is always on additionalProperties when there is a x-nullable ref and None comes.

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

5 participants