Unify response validation #339
Conversation
jean-edouard-boulanger
force-pushed
the
unify-response-validation
branch
3 times, most recently
from
a0d2bb8
to
be2e41f
Compare
jean-edouard-boulanger
force-pushed
the
unify-response-validation
branch
from
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.
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.
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.
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.
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.
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.
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.
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.
| @@ -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.
Same as comment above for flask
jean-edouard-boulanger
changed the title
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.
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): | |||
| response_payload=RawResponsePayload(payload=response.body), | ||
| ) | ||
| except ValidationError as err: | ||
| response = JSONResponse(err.errors(), 500) |
There was a problem hiding this comment.
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.
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.