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

Support for additionalProperties #63

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mephinet
Copy link

@mephinet mephinet commented Jan 11, 2022

This pull request adds support for validation of OpenAPI specs that make use of the additionalProperties feature.
For an type=object, additionalProperties and properties can both be specified, or only one of them.
When a schema defines additionalProperties, no __slots__ is set in the generated Model class, so that the additional attributes can be stored in the __dict__ of the instance. As with normal properties, no type checking for string/int/... is performed.

@mephinet mephinet marked this pull request as ready for review January 11, 2022 17:41
@Dorthu Dorthu self-requested a review January 12, 2022 12:11
@Dorthu
Copy link
Owner

Dorthu commented Jan 12, 2022

Thanks for the submission!

Looking at the OpenAPI definition and the relevant parts of JSON Schema, it looks like additionalProperties is intended to be either false (to explicitly say no additional properties are allowed) or a schema describing what is allowed (i.e. {"type": "string"}). I couldn't find any examples of an "additionalProperties": true that allowed all additional properties for a schema. However, JSON Schema's reference article notes that by default, any additional properties are allowed.

I'm going to do more research as to how to interpret what the intended behavior is. However, testing this change, I notice some inconsistencies between the behavior of classes with additionalProperties and those without:

In [4]: # r has additionalProperties set to true
   ...: print(r)
{}

In [5]: # lt does not
   ...: print(lt)
{'id': 'g6-nanode-1', 'label': 'Nanode 1GB', 'disk': 25600, 'class': 'nanode', 'price': {'hourly': 0.0075, 'monthly': 5.0}, 'addons': {'backups': {'price': {'hourly': 0.003, 'monthly': 2.0}}}, 'network_out': 1000, 'memory': 1024, 'successor': None, 'transfer': 1000, 'vcpus': 1, 'gpus': 0}

In [6]: r.__slots__
Out[6]: ['_raw_data', '_schema']

In [7]: lt.__slots__
Out[7]: dict_keys(['id', 'label', 'disk', 'class', 'price', 'addons', 'network_out', 'memory', 'successor', 'transfer', 'vcpus', 'gpus'])

In [8]: r._schema

In [9]: lt._schema
Out[9]: <<class 'openapi3.schemas.Schema'> ['components', 'schemas', 'LinodeType']>

In [10]: r._raw_data

In [11]: lt._raw_data
Out[11]:
{'id': 'g6-nanode-1',
 'label': 'Nanode 1GB',
 'price': {'hourly': 0.0075, 'monthly': 5.0},
 'addons': {'backups': {'price': {'hourly': 0.003, 'monthly': 2.0}}},
 'memory': 1024,
 'disk': 25600,
 'transfer': 1000,
 'vcpus': 1,
 'gpus': 0,
 'network_out': 1000,
 'class': 'nanode',
 'successor': None}

I think that at the very least, these should behave the same way when used as above.

@commonism
Copy link

https://github.com/APIs-guru/openapi-directory/search?q=additionalProperties

I'm rolling with

aiopenapi3/v31/schemas.py:    additionalProperties: Optional[Union[bool, "Schema"]] = Field(default=None)
aiopenapi3/v20/schemas.py:    additionalProperties: Optional[Union[bool, "Schema", Reference]] = Field(default=None)
aiopenapi3/v30/schemas.py:    additionalProperties: Optional[Union[bool, "Schema", Reference]] = Field(default=None)

@chriswhite199
Copy link
Contributor

@Dorthu:

Looking at the OpenAPI definition and the relevant parts of JSON Schema, it looks like additionalProperties is intended to be either false (to explicitly say no additional properties are allowed) or a schema describing what is allowed (i.e. {"type": "string"}). I couldn't find any examples of an "additionalProperties": true that allowed all additional properties for a schema. However, JSON Schema's reference article notes that by default, any additional properties are allowed.

The swagger.io spec (https://swagger.io/specification/#properties) does note the following:

additionalProperties - Value can be boolean or object. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema. Consistent with JSON Schema, additionalProperties defaults to true

The Understanding JSON article does note:

By default any additional properties are allowed.

So this would imply that additionalProperties: true is the default (as the swagger spec noted), and you only need to reach for false (when you want to be strict) or a schema when you want to be somewhat lenient.

@Dorthu
Copy link
Owner

Dorthu commented Jan 26, 2022

Thanks for the excellent references @chriswhite199. In light of that, I think the correct behavior is to:

  • Accept a bool or a Schema for additionalProperties
  • Allow any additional properties in models created for a schema by default, or if additionalProperties is true
  • If additionalProperties is a schema, ensure models created for that schema have only additional properties that match the given schema
  • If additionalProperties is false, enforce that only the fields described in properties are accepted for models created for that schema

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.

4 participants