-
Notifications
You must be signed in to change notification settings - Fork 413
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
feat(event_handler): add custom response validation in OpenAPI utility #6189
base: develop
Are you sure you want to change the base?
feat(event_handler): add custom response validation in OpenAPI utility #6189
Conversation
… error status code.
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
Hey hey @amin-farjadi! Thanks a lot for this PR! I'm working on fixing something else and will review it by Monday, ok? |
Hi @leandrodamascena 👋 Absolutely fine, thanks |
I see that our CI is failing with some importing or type annotation. Can you take a look please? |
Why are the tests checked against python 3.9 when the codebase is not compatible with it? Union operator for typing is used extensively in the codebase (which fails the python 3.9 test). If I turn |
|
||
class Todo(BaseModel): | ||
userId: int | ||
id_: int | None = Field(alias="id", default=None) |
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.
id_: int | None = Field(alias="id", default=None) | |
id_: Optional[int] = Field(alias="id", default=None) |
There are some inconsistencies when using Pydantic models and Union operations.. Try to make this Optional and remove |
I see! |
@leandrodamascena added ruff rule |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #6189 +/- ##
========================================
Coverage 96.25% 96.26%
========================================
Files 236 236
Lines 11407 11429 +22
Branches 830 834 +4
========================================
+ Hits 10980 11002 +22
Misses 337 337
Partials 90 90 ☔ View full report in Codecov by Sentry. |
…s and add more detailed error messages.
I'm starting my review of this 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.
Hey @amin-farjadi! This is an excellent PR, and I really appreciate your work on this. I've left some comments that I believe we should address before moving to the next round of review.
I'm also curious about potentially enabling this response validation code for individual routes as well. We could have it at both the global level and per route. This isn't a mandatory change, but I'd like to hear your thoughts on this idea.
Let me know what you think, and thanks again for your valuable contribution!
response_validation_error_http_status | ||
Enables response validation and sets returned status code if response is not validated. |
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.
response_validation_error_http_status | |
Enables response validation and sets returned status code if response is not validated. | |
response_validation_error_http_status | |
HTTP status code returned when response validation fails. |
Response validation is already enabled, we are modifying the customer to inform a specific http status code.
@@ -1496,6 +1500,7 @@ def __init__( | |||
serializer: Callable[[dict], str] | None = None, | |||
strip_prefixes: list[str | Pattern] | None = None, | |||
enable_validation: bool = False, | |||
response_validation_error_http_status=None, |
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.
response_validation_error_http_status=None, | |
response_validation_error_http_code=None, |
What do you think about this?
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.
Looks good
if response_validation_error_http_status and not enable_validation: | ||
msg = "'response_validation_error_http_status' cannot be set when enable_validation is False." | ||
raise ValueError(msg) | ||
|
||
if ( | ||
not isinstance(response_validation_error_http_status, HTTPStatus) | ||
and response_validation_error_http_status is not None | ||
): | ||
|
||
try: | ||
response_validation_error_http_status = HTTPStatus(response_validation_error_http_status) | ||
except ValueError: | ||
msg = f"'{response_validation_error_http_status}' must be an integer representing an HTTP status code." | ||
raise ValueError(msg) from None | ||
|
||
self._response_validation_error_http_status = ( | ||
response_validation_error_http_status | ||
if response_validation_error_http_status | ||
else HTTPStatus.UNPROCESSABLE_ENTITY | ||
) |
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.
if response_validation_error_http_status and not enable_validation: | |
msg = "'response_validation_error_http_status' cannot be set when enable_validation is False." | |
raise ValueError(msg) | |
if ( | |
not isinstance(response_validation_error_http_status, HTTPStatus) | |
and response_validation_error_http_status is not None | |
): | |
try: | |
response_validation_error_http_status = HTTPStatus(response_validation_error_http_status) | |
except ValueError: | |
msg = f"'{response_validation_error_http_status}' must be an integer representing an HTTP status code." | |
raise ValueError(msg) from None | |
self._response_validation_error_http_status = ( | |
response_validation_error_http_status | |
if response_validation_error_http_status | |
else HTTPStatus.UNPROCESSABLE_ENTITY | |
) | |
self._response_validation_error_http_status = self._validate_response_validation_error_status( | |
response_validation_error_http_status, | |
enable_validation | |
) |
And then we can simplify the logic and move the code for a specific function:
def _validate_response_validation_error_status(self, response_validation_error_http_status, enable_validation):
if response_validation_error_http_status and not enable_validation:
raise ValueError("'response_validation_error_http_status' cannot be set when enable_validation is False.")
if response_validation_error_http_status is None:
return HTTPStatus.UNPROCESSABLE_ENTITY
if isinstance(response_validation_error_http_status, HTTPStatus):
return response_validation_error_http_status
try:
return HTTPStatus(response_validation_error_http_status)
except ValueError:
raise ValueError(f"'{response_validation_error_http_status}' must be an integer representing an HTTP status code.")
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.
Great idea
http_status = ( | ||
self._response_validation_error_http_status | ||
if self._response_validation_error_http_status | ||
else HTTPStatus.UNPROCESSABLE_ENTITY | ||
) |
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.
http_status = ( | |
self._response_validation_error_http_status | |
if self._response_validation_error_http_status | |
else HTTPStatus.UNPROCESSABLE_ENTITY | |
) | |
http_status = self._response_validation_error_http_status |
Am I wrong or aren't we testing this condition before setting the HTTP code? Do we need it again here?
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.
Absolutely right
custom_serialize_response_error: ValidationException, optional | ||
Optional error type to raise when response to be returned by the endpoint is not | ||
serialisable according to field type. | ||
Raises RequestValidationError by default. |
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'm not sure I fully understand this. Could you please explain it to me?
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 have no idea where this came from :D. Will remove it and add _has_response_validation_error
param to docstring.
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 shouldn't be a unit test; it's more suited as a functional test. I'd like it moved to https://github.com/aws-powertools/powertools-lambda-python/tree/develop/tests/functional/event_handler/_pydantic.
Also, please avoid adding tests inside classes. Please move the tests to plain functions, and utilizes fixtures when needed.
@@ -38,7 +38,8 @@ lint.ignore = [ | |||
"B904", # raise-without-from-inside-except - disabled temporarily | |||
"PLC1901", # Compare-to-empty-string - disabled temporarily | |||
"PYI024", | |||
"A005" | |||
"A005", | |||
"FA102" # project must require python >= 3.9 making this error obsolete |
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'll review this once all other reviews are complete.
Hi @leandrodamascena, thanks for the review. Will implement suggested changes by the end of the week. Regarding custom http status code on a route-level, I will familiarise myself with the |
Thanks @amin-farjadi! It's great that you understand the |
…lidation_error_http_code
…error_http_code param.
…r_http_code param.
…r_http_code param being None.
Hi @leandrodamascena, I believe all your points have been addressed in the recent commits. I have drafted an implemented for route-specific custom response validation http status code which I can add in a subsequent PR |
|
Hi @amin-farjadi! Thanks a lot for addressing all the points! I'll review it again today and I'll probably have some very small changes to make, especially in the documentation part, but nothing critical and I can push a commit before approving it, ok? I'm very happy to have this new feature, it will give customers more flexibility when working with data validation. Regarding to the validation code per route, please open an issue with a pseudo code showing the experience, this will be super nice! |
I'm happy to try and close this PR today. Absolutely fine by me, please do feel free. I drafted this issue for route-specific custom response validation http status code (that's a mouthful!) #6245 |
Hi @amin-farjadi! Just a quick explanation. A while ago you opened this issue and we fixed it in this PR. But this created a regression and I had to revert it while I find the right solution. So because of this, some tests in the
|
Issue number: #5888
Summary
Add the option to distinguish between payload validation and response validation.
Changes
has_response_validation_error
argument has been added toOpenAPIValidationMiddleware
.response_validation_error_http_status
argument has been added to theApiGatewayResolver
and its subclasses.ResponseValidationError
exception class has been added to OpenAPI exceptionsAPIGatewayRestResolver
response validation.User experience
Nothing will change. The user will have the option to set a custom response validation http status for Amazon API Gateway REST and HTTP APIs, Application Load Balancer (ALB), Lambda Function URLs, and VPC Lattice.
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.