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.

Sign up for GitHub

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

Jump to bottom

[0.16.2 regression] {"key": null} fails to validate as {"type": "object"} #442

Closed
andersk opened this issue Dec 4, 2022 · 4 comments · May be fixed by #446
Closed

[0.16.2 regression] {"key": null} fails to validate as {"type": "object"} #442

andersk opened this issue Dec 4, 2022 · 4 comments · May be fixed by #446
Labels
area/unmarshalling Indicates an issue on unmarshalling area. kind/bug/confirmed

Comments

@andersk
Copy link
Contributor

andersk commented Dec 4, 2022 •
edited
Loading

In 0.16.1, {"key": null} validated successfully against {"type": "object"}—an object that includes a null value is still an object. But in 0.16.2 this incorrectly raises openapi_core.unmarshalling.schemas.exceptions.InvalidSchemaValue: Value None not valid for schema of type any: (<ValidationError: 'None for not nullable'>,).

This regression was introduced by commit 692a915 (#434).

Full reproducible example:

from openapi_core import Spec
from openapi_core.testing import MockRequest
from openapi_core.validation.request import openapi_request_validator

spec = Spec.create(
    {
        "openapi": "3.0.3",
        "info": {"title": "test", "version": "0"},
        "paths": {
            "/test": {
                "post": {
                    "parameters": [
                        {
                            "name": "obj",
                            "in": "query",
                            "content": {
                                "application/json": {
                                    "schema": {"type": "object"},
                                }
                            },
                        },
                    ],
                    "responses": {"200": {"description": ""}},
                },
            },
        },
    },
)
request = MockRequest(
    "http://localhost/", "post", "/test", args={"obj": '{"key": null}'}
)
result = openapi_request_validator.validate(spec, request)
print(result)
result.raise_for_errors()
@Wim-De-Clercq
Copy link
Contributor

Wim-De-Clercq commented Dec 12, 2022 •
edited
Loading

The openapi-schema-validator update introduced this https://github.com/p1c2u/openapi-schema-validator/blob/553d606ad0289e81564b7c60aed05fd0e745f6bd/openapi_schema_validator/validators.py#L62
Which makes every schema have a default "nullable: false" requirement. Which I believe is correct. You should specify "nullable: true" if you want to have nullable properties.

The problem is that openapi-core creates a little dummy schema for the "additionalProperties" -- for any key not present in the schema -- like so:
https://github.com/p1c2u/openapi-core/blob/4055a365591d9ef5ac8c93a5ebaee61c19f04fe0/openapi_core/unmarshalling/schemas/unmarshallers.py#L295

But now that little schema will have a "nullable: false" check, and it incorrectly raises exceptions for this.

The above line should be updated to

additional_prop_schema = Spec.from_dict({"nullable": True})

Or perhaps a little refactor is in place to not create any schema and just take the value as is. It feels a bit wasteful to constantly create these little schemas and do null-validation checks for something which isn't even defined in the schema in the first place.

Wim-De-Clercq added a commit to Wim-De-Clercq/openapi-core that referenced this issue Dec 12, 2022
@Wim-De-Clercq
additionalProperties without schema should allow null.
1e3a296 
Issue python-openapi#442
@Wim-De-Clercq Wim-De-Clercq mentioned this issue Dec 12, 2022
Merged
Wim-De-Clercq added a commit to Wim-De-Clercq/openapi-core that referenced this issue Dec 12, 2022
@Wim-De-Clercq
additionalProperties without schema should allow null.
ae27109 
Issue python-openapi#442
Wim-De-Clercq added a commit to Wim-De-Clercq/openapi-core that referenced this issue Dec 12, 2022
@Wim-De-Clercq
additionalProperties without schema should allow null.
e017634 
Issue python-openapi#442
@p1c2u
Copy link
Collaborator

p1c2u commented Dec 17, 2022 •
edited
Loading

Or perhaps a little refactor is in place to not create any schema and just take the value as is. It feels a bit wasteful to constantly create these little schemas and do null-validation checks for something which isn't even defined in the schema in the first place.

I must admit it was silly. I will change that .

EDIT: Now I remember why I made this change. It was to unmarshall additional properties to objects instead of just returning pure python dict.

@p1c2u p1c2u mentioned this issue Dec 17, 2022
Open
@p1c2u p1c2u added kind/bug/confirmed area/unmarshalling Indicates an issue on unmarshalling area. labels Dec 20, 2022
@Wim-De-Clercq
Copy link
Contributor

Wim-De-Clercq commented Apr 17, 2023 •
edited
Loading

@p1c2u
Do you reckon this bug is actually fixed in the meantime? If I git-checkout 0.17.1 and run the above test (with updated imports to reflect changes) it seems to report no errors.

Update: ah yes, I see my commit got merged: e017634
Fixed since >= 0.16.3

I guess we can close this issue (and related still open PRs). I'll leave it to you.

@p1c2u
Copy link
Collaborator

p1c2u commented Apr 18, 2023

Yes it was merged hence closing

@p1c2u p1c2u closed this as completed Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/unmarshalling Indicates an issue on unmarshalling area. kind/bug/confirmed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

AdditionalProperties free form nullable fix python-openapi/openapi-core
3 participants
@andersk @p1c2u @Wim-De-Clercq