Fixing Subschema Required Properties Validation #22
Conversation
Currently if valid swagger syntax is used for model composition an
error will be thrown due to the lack of a type property. This was
corrected by making object the default type.
schema_type = schema_deref.get('type', 'object')
I changed the swagger definition to test for this. Now PetCreate is a
composite of PetCreatePartOne and PetCreatePartTwo. However, this
caused `test_post_pets_empty_body` to fail, which turned out to be a
bug in the required properties.
In `_unmarshal_object` the `get_all_properties` method is called to get
all properties from the subschemas. However, this is not done for
required properties, meaning that only top level required properties
will be correctly validated. I have added a
`get_all_required_properties’ to fix this.
This caused `test_get_pets` to fail. In this case the bug allowed an
incorrect test case to be introduced. Pet requires `id`, but it also
requires name because it inherits from PetCreate. I have fixed this
test case by adding the missing required property.
After these changes `test_get_pet_not_found` failed due to a string
formatting error (double quotes vs single quotes). I fixed this by
switching to dictionary comparisons.
|
It looks like the build timed out for python 3.6. I don't think this is a legitimate failure. |
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
=========================================
- Coverage 98.55% 98.45% -0.1%
=========================================
Files 18 18
Lines 897 904 +7
=========================================
+ Hits 884 890 +6
- Misses 13 14 +1
Continue to review full report at Codecov.
|
| @@ -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') | |||
There was a problem hiding this comment.
Why you think object is default schema type? I couldn't find it in the doc.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
openapi_core/__init__.py
Outdated
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| __author__ = 'Artur Maciąg' | |||
| __email__ = 'maciag.artur@gmail.com' | |||
| __version__ = '0.4.2' | |||
| __version__ = '0.4.3' | |||
There was a problem hiding this comment.
Don't bump version inside PR please
There was a problem hiding this comment.
Sorry. I'll remove this change.
This reverts commit c18c4a5.
Fixing Subschema Required Properties Validation
Currently if valid swagger syntax is used for model composition an
error will be thrown due to the lack of a type property. This was
corrected by making object the default type.
schema_type = schema_deref.get('type', 'object')
I changed the swagger definition to test for this. Now PetCreate is a
composite of PetCreatePartOne and PetCreatePartTwo. However, this
caused
test_post_pets_empty_bodyto fail, which turned out to be abug in the required properties.
In
_unmarshal_objecttheget_all_propertiesmethod is called to getall properties from the subschemas. However, this is not done for
required properties, meaning that only top level required properties
will be correctly validated. I have added a
`get_all_required_properties’ to fix this.
This caused
test_get_petsto fail. In this case the bug allowed anincorrect test case to be introduced. Pet requires
id, but it alsorequires name because it inherits from PetCreate. I have fixed this
test case by adding the missing required property.
After these changes
test_get_pet_not_foundfailed due to a stringformatting error (double quotes vs single quotes). I fixed this by
switching to dictionary comparisons.