#245 fix validation for additionalProperties #246
Conversation
|
Nice! Do we need an update to the readme as well? @sondrelg can you add sonar scanner so coverage is picked? |
|
Looks like the tests are failing on Python 3.10. From what I can tell it's not related to the PR, we just need to add this line to our dev dependencies in master. |
I took a look and didn't find anything in the README file that would be affected by these changes. What did you have in mind? I thought I could add a line to the |
|
Go for it 👏 |
|
LGTM. Thanks! |
|
I can look over this later tonight and release a patch, unless you're already doing so @Goldziher 👍 |
Nope, go ahead 😃 |
| @@ -295,8 +295,7 @@ def test_openapi_object( | |||
| required_keys = [key for key in schema_section.get("required", []) if key not in write_only_properties] | |||
| response_keys = data.keys() | |||
| additional_properties: Optional[Union[bool, dict]] = schema_section.get("additionalProperties") | |||
| if not properties and isinstance(additional_properties, dict): | |||
| properties = additional_properties | |||
| additional_properties_allowed = additional_properties is not None | |||
There was a problem hiding this comment.
Since we specify
elif isinstance(additional_properties, dict):we currently won't have any handling for non-bool/non-dict types. What do you think about amending this assignment to this, so all data types are handled?
| additional_properties_allowed = additional_properties is not None | |
| additional_properties_allowed = additional_properties is True or isinstance(additional_properties, dict) |
There was a problem hiding this comment.
additionalProperties can only be a boolean or an object, so I think those two would be roughly equivalent. The only difference, if the schema is not considered invalid by the schema validation, would be in how we want to handle schemas where additionalProperties is defined as a different type.
So say there's a schema defined like this:
type: object
additionalProperties: 1
Do we want to allow additionalProperties in this case or not? With the is not None check we would allow them but not validate anything about them. With your proposed change we would not allow additionalProperties. I don't mind either option.
There was a problem hiding this comment.
From what I can gather, it would be possible for users to specify whatever they want, but anything other than a dict/bool/null would be a violation of the spec - does that seem right? If that's correct, we're deciding whether we think it's within the scope of this library to raise an error for the invalidity of the schema or not.
I can also see both sides, but am probably leaning towards raising an error here. Perhaps instead of the suggestion I made above, we should do something like this?
if not isinstance(additional_properties, (bool, dict, None)):
raise OpenAPISchemaError('Invalid additional properties type')
additional_properties_allowed = additional_properties is not NoneWhat do you think @Goldziher? 🙂
There was a problem hiding this comment.
yes, i agree @sondrelg - i think your last suggestion is the cleanest of what has been suggested so far.
There was a problem hiding this comment.
I've added the check and a test. Changed it a little bit because None cannot be used as a type in isinstance.
|
Kudos, SonarCloud Quality Gate passed!
|
|
@idesoto-rover - thanks for your work. Linting is failing though. I will let @sondrelg handle it from here. |
|
Nice 👏 Thanks @idesoto-rover! I'll fix the linting when bumping the version 👍 |
Fixes issues described in #245:
additionalPropertiesand validates any extra keys in the response that are not defined in thepropertiessection against this schemaadditionalPropertiescan be an empty schema{}and this means any values are allowed in extra keysadditionalPropertiescan beTrueand this behaves the same as an empty schema (this was already working)