Skip to content
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

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

amin-farjadi
Copy link

Issue number: #5888

Summary

Add the option to distinguish between payload validation and response validation.

Changes

  • Optional has_response_validation_error argument has been added to OpenAPIValidationMiddleware.
  • Optional response_validation_error_http_status argument has been added to the ApiGatewayResolver and its subclasses.
  • ResponseValidationError exception class has been added to OpenAPI exceptions
  • Tests have been added for APIGatewayRestResolver response validation.
  • Add Validating responses section in the docs

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:

  • Migration process documented
  • Implement warnings (if it can live side by side)

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.

@amin-farjadi amin-farjadi requested a review from a team as a code owner February 28, 2025 14:37
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation event_handlers tests labels Feb 28, 2025
Copy link

boring-cyborg bot commented Feb 28, 2025

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 28, 2025
@leandrodamascena leandrodamascena changed the title feat: add custom response validation feat(openapi): add custom response validation Feb 28, 2025
@github-actions github-actions bot added feature New feature or functionality and removed documentation Improvements or additions to documentation labels Feb 28, 2025
@leandrodamascena
Copy link
Contributor

Hey hey @amin-farjadi! Thanks a lot for this PR! I'm working on fixing something else and will review it by Monday, ok?

@amin-farjadi
Copy link
Author

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

@leandrodamascena
Copy link
Contributor

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?

@amin-farjadi
Copy link
Author

amin-farjadi commented Feb 28, 2025

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 int | None to Optional[int], formatter will be unhappy. But, happy to do so if needs be


class Todo(BaseModel):
userId: int
id_: int | None = Field(alias="id", default=None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id_: int | None = Field(alias="id", default=None)
id_: Optional[int] = Field(alias="id", default=None)

@leandrodamascena
Copy link
Contributor

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 int | None to Optional[int], formatter will be unhappy. But, happy to do so if needs be

There are some inconsistencies when using Pydantic models and Union operations.. Try to make this Optional and remove future annotations. I think this can solve the problem for now. If not, I can take a look during the review.

@amin-farjadi
Copy link
Author

I see!

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Feb 28, 2025
@amin-farjadi
Copy link
Author

amin-farjadi commented Feb 28, 2025

@leandrodamascena added ruff rule FA102 to be ignored for linting as this project uses python >=3.9 . This resolved the issue I was having in precommit, forcing me to import annotations in the test.

@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Feb 28, 2025
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.26%. Comparing base (bf64061) to head (218c666).

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.
📢 Have feedback on the report? Share it here.

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Feb 28, 2025
@leandrodamascena leandrodamascena changed the title feat(openapi): add custom response validation feat(event_handler): add custom response validation in OpenAPI utility Mar 1, 2025
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Mar 1, 2025
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Mar 3, 2025
@leandrodamascena
Copy link
Contributor

I'm starting my review of this PR.

Copy link
Contributor

@leandrodamascena leandrodamascena left a 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!

Comment on lines 1523 to 1524
response_validation_error_http_status
Enables response validation and sets returned status code if response is not validated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
response_validation_error_http_status=None,
response_validation_error_http_code=None,

What do you think about this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Comment on lines 1542 to 1561
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
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea

Comment on lines 2412 to 2416
http_status = (
self._response_validation_error_http_status
if self._response_validation_error_http_status
else HTTPStatus.UNPROCESSABLE_ENTITY
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely right

Comment on lines 75 to 78
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.
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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
Copy link
Contributor

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.

@amin-farjadi
Copy link
Author

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 Route implementation and has a crack at it in this PR.

@leandrodamascena
Copy link
Contributor

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 Route implementation and has a crack at it in this PR.

Thanks @amin-farjadi! It's great that you understand the Route implementation. However, after further consideration, let's keep this PR as is and create a new issue to discuss the implementation of the route-specific functionality. We can then work on that in a separate PR.

@amin-farjadi
Copy link
Author

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

Copy link

@leandrodamascena
Copy link
Contributor

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!

@amin-farjadi
Copy link
Author

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

@leandrodamascena
Copy link
Contributor

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 test_openapi_validation_middleware.py file are failing, but this is not caused by you. Can you please disable the those tests? Don't remove, just add the decorator @pytest.mark.skipif(reason="Test temporarily disabled until falsy return is fixed")?

test_validation_error_none_returned_non_optional_type
test_none_returned_for_falsy_return
test_custom_response_validation_error_http_code_invalid_response_none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation event_handlers feature New feature or functionality size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: HTTP response of an unsuccessful response validation must be a server-side error
2 participants