Fix schema validation with internal fields (fixes #1244) #1259
Conversation
leplatrem
changed the title
| if required_fields: | ||
| schema = {**schema, 'required': required_fields} | ||
| else: | ||
| schema = {f: v for f, v in new.items() if f != 'required'} |
There was a problem hiding this comment.
I don't think we need to go to this lengths to avoid putting an empty required field. The JSON Schema Spec says that omitting it is the same as an empty array. Unless you're trying to do this so that we don't confuse users by secretly changing their schema, but I don't think that's valuable, since we're already changing the schema.
Should we be doing these manipulations to the schema at schema save time?
Should we be removing definitions of internal fields from schema['properties']?
There was a problem hiding this comment.
I agree with @glasserc that we should do all this at save time, rejecting the schema or changing it silently if it has internal fields in it.
There was a problem hiding this comment.
This approach has some advantages:
- it avoids breaking with existing collection metadata
- we strip record and schema at the same place (stripping record fields will still be necessary if the users don't allow additional props on their schemas)
- we are in a resource class, where
self.model.id_fieldetc. are available. It's not the case within the colander validator for collection schema - it is fairly similar to what there were before this PR, so I'm not chocked by those changes
Please expose your whys :)
There was a problem hiding this comment.
I don't think we need to go to this lengths to avoid putting an empty required field
The empty array gives me an error in jsonschema library that we use:
Failed validating 'minItems' in schema['properties']['required']:
{'items': {'type': 'string'},
'minItems': 1,
'type': 'array',
'uniqueItems': True}
On instance['required']:
[]
| schema = {**schema, 'required': required_fields} | ||
| else: | ||
| schema = {f: v for f, v in new.items() if f != 'required'} | ||
| data = {f: v for f, v in new.items() if f not in internal_fields} |
leplatrem
force-pushed
the
1244-fix-id-schema-validation
branch
from
2065990 to
c6bf967
Compare
|
Please open an issue if you think that we should refactor this. |
Fixes #1244