-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Unify response validation #339
Unify response validation #339
Conversation
a0d2bb8
to
be2e41f
Compare
be2e41f
to
5ac5119
Compare
spectree/plugins/flask_plugin.py
Outdated
if resp.expect_list_result(status) and isinstance(model, list): | ||
expected_list_item_type = resp.get_expected_list_item_type(status) | ||
if all(isinstance(entry, expected_list_item_type) for entry in model): | ||
skip_validation = True | ||
result = ( | ||
[ | ||
( | ||
serialize_model_instance(entry) | ||
if isinstance(entry, BaseModel) | ||
else entry | ||
) | ||
for entry in model | ||
], | ||
try: | ||
response_validation_result = validate_response( | ||
skip_validation=skip_validation, | ||
validation_model=resp.find_model(status) if resp else None, | ||
response_payload=( | ||
RawResponsePayload(payload=response_payload.get_json()) | ||
if ( | ||
isinstance(response_payload, flask.Response) | ||
and not skip_validation | ||
) | ||
else response_payload | ||
), | ||
) | ||
except ValidationError: | ||
response = make_response( | ||
jsonify({"message": "response validation error"}), 500 | ||
) | ||
else: | ||
response = make_response( | ||
( | ||
response_validation_result.validated_response_payload, | ||
status, | ||
*rest, | ||
) | ||
elif ( |
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.
You'll notice that this branch is no longer present in the new validate_response
. This is because the new root response handling (previous PR) naturally takes care of list responses!
] | ||
if isinstance(content, list) | ||
else serialize_model_instance(content) | ||
serialize_model_instance(_PydanticResponseModel(__root__=content)) |
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.
This small change here makes this a lot simpler IMO: we completely delegate serialisation to Pydantic so we no longer need to deal with special cases (like lists of models) ourselves.
response_payload=RawResponsePayload(payload=response.body), | ||
) | ||
except ValidationError as err: | ||
response = JSONResponse(err.errors(), 500) |
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've noticed that not all plugins return the same error message when the response validation fails:
- Most only return a generic error message ("response validation error")
- This one returns the list of validation errors from Pydantic
Shall we make this consistent (in another PR)?
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.
Nice catch. Yeah, we can do it in another PR.
assert resp.content_type == "application/json" | ||
assert resp.json["name"] == "flask" | ||
assert resp.json["x_score"] == sorted(resp.json["x_score"], reverse=True) | ||
else: |
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've added this here to make sure we have one test that returns non-validated non-json content. This makes sure that when skip_validation=True
, none of the plugins:
- Accidentally try to deserialise the payload as JSON
- Alter the payload in any way
def ping(): | ||
"""summary | ||
|
||
description""" | ||
return jsonify(msg="pong") | ||
return jsonify(msg="pong"), 202 |
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 wanted to have at least one test return the response HTTP status code inline as a tuple. You can check that without the change of logic in flask/quart plugins (line 209 / 220 respectively), this use case is not supported.
@@ -206,63 +201,38 @@ def validate( | |||
|
|||
status = 200 | |||
rest = [] | |||
if resp and isinstance(result, tuple) and isinstance(result[0], BaseModel): |
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.
Tell me if I'm missing something, but I think we always want to get model
/status
/*rest
even if the first tuple element is not a BaseModel. Thinking of cases where:
- A list of BaseModel is returned, along with a specific error code.
- Something like
return jsonify(message="this is not supported"), 400
.
Granted it's a bit of an edge case, but the right thing to do IMO.
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.
Agree.
@@ -217,63 +213,40 @@ async def validate( | |||
|
|||
status = 200 | |||
rest = [] | |||
if resp and isinstance(result, tuple) and isinstance(result[0], BaseModel): |
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.
Same as comment above for flask
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.
Impressive work!
Thanks for your contribution! 🎉
spectree/plugins/base.py
Outdated
else: | ||
skip_validation = True | ||
final_response_payload = serialize_model_instance(response_payload) | ||
elif isinstance(response_payload, validation_model): |
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.
Shall we use:
if isinstance(response_payload, validation_model):
....
elif is_root_model(validation_model)
@@ -206,63 +201,38 @@ def validate( | |||
|
|||
status = 200 | |||
rest = [] | |||
if resp and isinstance(result, tuple) and isinstance(result[0], BaseModel): |
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.
Agree.
response_payload=RawResponsePayload(payload=response.body), | ||
) | ||
except ValidationError as err: | ||
response = JSONResponse(err.errors(), 500) |
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.
Nice catch. Yeah, we can do it in another PR.
spectree/plugins/base.py
Outdated
|
||
@dataclass(frozen=True) | ||
class ResponseValidationResult: | ||
validated_response_payload: Any |
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.
maybe payload
is enough since the outer class name already contains "validation"
Thank you for taking the time to review 🙌 |
As promised, this is a followup this PR which unifies and simplifies response validation across plugins. Response validation has been generalised in a single/simple function used across plugins.