-
Notifications
You must be signed in to change notification settings - Fork 42
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
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also LGTM, couple of comments below
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also change all these other validators to ValueError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_validator
s 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.