Validate illegal fields are not present under attributes and relationships #83
Conversation
This is validated for Attributes and Relationships. When/If we get to pydantic v1.0, root_validators can be used for this instead.
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
==========================================
- Coverage 84.32% 83.96% -0.37%
==========================================
Files 35 35
Lines 2099 2058 -41
==========================================
- Hits 1770 1728 -42
- Misses 329 330 +1
Continue to review full report at Codecov.
|
optimade/models/jsonapi.py
Outdated
| class Config: | ||
| extra = "allow" | ||
|
|
||
| @validator("relationships", "links", "id", "type") | ||
| def check_illegal_attributes_fields(cls, v): | ||
| raise AssertionError( |
There was a problem hiding this comment.
When I try to add a test for this (see my commit), this gets raised as an AssertionError rather than a ValidationError (as I'd expect). Do we want to make this return a validation error instead?
There was a problem hiding this comment.
Sure. But isn't that a bit weird? Not the change, but that you're not catching it. We should have an AssertionError in the structure model as well. Do you catch that one?
Should I change it to ValueError or a ValidationError?
There was a problem hiding this comment.
I think the point is that pydantic is meant to internally catch AssertionErrors (it worked with just assert statements right?) so I'm a bit confused why this doesn't work (see https://pydantic-docs.helpmanual.io/usage/validators/). ValueError would probably be the next most appropriate?
(The point being this would break any reporting of further errors in validation)
This should hopefully catch the error and not break the reporting of further errors in validation.
|
|
||
| @validator("id", "type") | ||
| def check_illegal_relationships_fields(cls, v): | ||
| raise AssertionError('"id", "type" MUST NOT be fields under relationships') |
There was a problem hiding this comment.
Should we also change all these other validators to ValueError?
There was a problem hiding this comment.
To speed things up, I'll accept as is and you can merge if you want. If you change it just re-request my review and I'll accept.
There was a problem hiding this comment.
I guess we should? Since we didn't encounter this before, it must mean that we are not actually testing this validator?
Fixes #22
When/If we get to pydantic v1.0,
root_validators can be used for this instead, see here.I have tried simply having the validator without specifying the fields in the models, but then the validator will never be called - for some reason. So by specifying the illegal fields as fields it is possible to have a working validator for them that simply raises immediately.