Skip to content

Fixing Subschema Required Properties Validation #22

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

Merged
merged 3 commits into from
Apr 13, 2018
Merged

Fixing Subschema Required Properties Validation #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56be4b1
Fixing Subschema Required Properties Validation
Apr 4, 2018
c18c4a5
Updating Version
Apr 5, 2018
84d36e3
Revert "Updating Version"
Apr 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,6 @@ ENV/

# mypy
.mypy_cache/

# Jetbrains project files
.idea/
14 changes: 12 additions & 2 deletions openapi_core/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ def get_all_properties(self):

return properties

def get_all_required_properties(self):
required = self.required.copy()

for subschema in self.all_of:
subschema_req = subschema.get_all_required_properties()
required += subschema_req

return required

def get_cast_mapping(self):
mapping = DEFAULT_CAST_CALLABLE_GETTER.copy()
mapping.update({
Expand Down Expand Up @@ -121,6 +130,7 @@ def _unmarshal_object(self, value):
value = loads(value)

all_properties = self.get_all_properties()
all_required_properties = self.get_all_required_properties()
all_properties_keys = all_properties.keys()
value_keys = value.keys()

Expand All @@ -135,7 +145,7 @@ def _unmarshal_object(self, value):
try:
prop_value = value[prop_name]
except KeyError:
if prop_name in self.required:
if prop_name in all_required_properties:
raise MissingProperty(
"Missing schema property {0}".format(prop_name))
if not prop.nullable and not prop.default:
Expand Down Expand Up @@ -167,7 +177,7 @@ def __init__(self, dereferencer):
def create(self, schema_spec):
schema_deref = self.dereferencer.dereference(schema_spec)

schema_type = schema_deref.get('type')
schema_type = schema_deref.get('type', 'object')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you think object is default schema type? I couldn't find it in the doc.

Copy link
Author

@AMcManigal AMcManigal Apr 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Which doc? Do you mean the OAS 3.0 spec? If not could you post a link?

As far as the logic for making objects the default, it seems that if it's not any of the other type the only conclusion, assuming a correct spec structure, is that it's an object. I would argue that the existing logic acts this way.

For example, a schema can only have an allOf if it is an object. Even if the type is unspecified the schema will receive an allOf property, meaning that it is implicitly treated as an object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AMcManigal I was thinking about JSON Schema Specification Wright Draft 00.

Consider the following example:

{
  "anyOf": [
    { "type": "string", "maxLength": 5 },
    { "type": "number", "minimum": 0 }
  ]
}

It comes from https://spacetelescope.github.io/understanding-json-schema/reference/combining.html

Copy link
Author

@AMcManigal AMcManigal Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, thanks for the explanation!

I'm not sure if I feel that a json schema paradigm is the way to look at this.

Consider test_post_pets. If SchemaFactory defaults to an object then the response body is correctly parsed as a PetCreate object. However, if the type is left blank then the body remains a string and the test fails.

I believe what is happening (you would know this better) is that the code checks the schema for PetCreate to determine how to parse the response body. In this case it is absolutely correct to consider it an object because it is.

PetCreate:
      x-model: PetCreate
      allOf:
        - $ref: "#/components/schemas/PetCreatePartOne"
        - $ref: "#/components/schemas/PetCreatePartTwo"

If we translate the PetCreate to json it becomes even more obvious:

{
  "PetCreate": {
    "x-model": "PetCreate",
    "allOf": [
      {
        "$ref": "#/components/schemas/PetCreatePartOne"
      },
      {
        "$ref": "#/components/schemas/PetCreatePartTwo"
      }
    ]
  }
}

In either case we end up with a paradigm where it is more correct to treat untyped members as object since that is the convention of the OAS 3.0 spec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right it will fail but x-model is an extension and not part of spec and shouldn't be considered in schema type resolution process. The problem is i can't find in OAS spec that this is the convention of the OAS 3.0 spec but I can't see any other solution.

schema_format = schema_deref.get('format')
model = schema_deref.get('x-model', None)
required = schema_deref.get('required', False)
Expand Down
11 changes: 10 additions & 1 deletion tests/integration/data/v3.0/petstore.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,13 @@ components:
type: integer
format: int64
PetCreate:
type: object
x-model: PetCreate
allOf:
- $ref: "#/components/schemas/PetCreatePartOne"
- $ref: "#/components/schemas/PetCreatePartTwo"
PetCreatePartOne:
type: object
x-model: PetCreatePartOne
required:
- name
properties:
Expand All @@ -155,6 +160,10 @@ components:
$ref: "#/components/schemas/Tag"
address:
$ref: "#/components/schemas/Address"
PetCreatePartTwo:
type: object
x-model: PetCreatePartTwo
properties:
position:
$ref: "#/components/schemas/Position"
healthy:
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/test_petstore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,4 +654,4 @@ def test_get_pet_not_found(self, spec, response_validator):
response_result = response_validator.validate(request, response)

assert response_result.errors == []
assert response_result.data == data
assert response_result.data == data_json
1 change: 1 addition & 0 deletions tests/integration/test_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ def test_get_pets(self, validator):
'data': [
{
'id': 1,
'name': 'Sparky'
},
],
}
Expand Down