fix(openapi): correct response validation for falsy objects

[amin-farjadi]

Issue number: closes #5887

Summary

Fix openapi response validation behaviour on Falsy return objects.

Changes

in openapi_validation.py, OpenAPIResponseValidationMiddleware class:

• behaviour of _handle_response method modified
• behaviour of _serialize_response method modified
• _serialize_response method renamed to _serialize_response_with_validation

in test_openapi_validation_middleware.py:

• remove the skip from test_validation_error_none_returned_non_optional_type to cover issue Bug: Open API Validation not validating response serialization when body is Falsy. #5887
• added test_validation_error_different_list_returned_non_optional_type and test_validate_list_response to cover additional scenarios

in test_http_resolver_pydantic.py:

• test_async_handler_with_validation marked to skip due to a separate issue

User experience

Now Falsy response will be validated againt the OpenAPI return field (is there is any).

---

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.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.63%. Comparing base (aa969f7) to head (4ad00a9).

Files with missing lines	Patch %	Lines
...ls/event_handler/middlewares/openapi_validation.py	88.88%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #7990      +/-   ##
===========================================
- Coverage    96.73%   96.63%   -0.10%     
===========================================
  Files          278      278              
  Lines        13657    13659       +2     
  Branches      1086     1086              
===========================================
- Hits         13211    13200      -11     
- Misses         327      338      +11     
- Partials       119      121       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[amin-farjadi]

Hi @leandrodamascena is there anything I can do for this?

[dreamorosi]

Thanks for the PR and for proposing a fix.

If I understand the issue correctly, this fix might be more complex than necessary.

The root cause is that falsy values are being skipped due to if response.body and response.is_json().

A minimal fix would be:

- if response.body and response.is_json():
+ if response.is_json():

This single line change should fix both issues without more significant changes.

Do you think we could simplify the approach?

[amin-farjadi]

Hi @dreamorosi,

Your suggestion will not work for test_validation_error_none_returned_non_optional_type

[dreamorosi]

I've tested the one-liner change I suggested above and it works correctly for the test_validation_error_none_returned_non_optional_type test.

None is JSON-serializable and becomes "null", response.is_json() returns True for None and the validation middleware runs and correctly fails with 422.

[leandrodamascena]

Hey @amin-farjadi and @dreamorosi thanks a lot for working on this! I looked into both approaches here.

Andrea's suggestion works well and passes all the tests, but we had a similar solution for the PR #6119 and it ended up causing a breaking change for some customers.

I prefer your approach here @amin-farjadi. Instead of relying on the response content-type to decide if we should validate, you check if there's a return type annotation - that feels like the right thing to do and avoids the kind of edge cases where it validate the response and not check the type annotation.

@amin-farjadi before merging, please re-enable the two skipped test blocks in test_openapi_validation_middleware.py - they should work now:

• test_none_returned_for_falsy_return
• test_custom_response_validation_error_http_code_invalid_response_none

Just remove the @pytest.mark.skipif(reason="Test temporarily disabled until falsy return is fixed") decorators from both.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[amin-farjadi]

thank you for your replies @dreamorosi @leandrodamascena

Removed the skips, Leonardo 🚀

[leandrodamascena]

Hey @amin-farjadi thanks a lot for fixing this! I'll approve but won't merge now because I'll release a new version next week with this fix #7901 and I don't want to change a lot of stuff in the same release.

I'll merge this PR next week and release in 2, 3 weeks max.

Dismiss