-
Notifications
You must be signed in to change notification settings - Fork 440
feat(event_handler): add support for form data in OpenAPI utility #7028
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 support for form data in OpenAPI utility #7028
Conversation
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
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 @oyiz-michael thanks a lot for working on this PR! Can you please run make format
and push a new commit? Ruff is complaining about some unformatted files.
…/oyiz-michael/powertools-lambda-python into feat/openapi-header-file-support
@leandrodamascena Done! Just pushed the formatting fixes. 👍 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7028 +/- ##
===========================================
+ Coverage 96.24% 96.27% +0.02%
===========================================
Files 275 275
Lines 12896 12912 +16
Branches 952 954 +2
===========================================
+ Hits 12412 12431 +19
+ Misses 378 376 -2
+ Partials 106 105 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Hi @oyiz-michael, thank you so much for resolving the issue with Ruff.
I'll start reviewing this code, but so far I've suggested two changes. Could you add tests for the File and Form types? I didn't see any tests, and it's good to have tests to ensure it's working as expected and that we can catch any errors.
Thank you.
I added 22 new comprehensive tests that cover: Content Type Inference Tests: Routes with File/Form parameters → multipart inference Invalid JSON handling and error responses URL-encoded form data with single/multiple values Missing boundary error handling Field name extraction (quoted vs unquoted) Unsupported content types |
Successfully Enhanced Test Coverage: The test file now includes comprehensive coverage for all the refactored helper methods in the OpenAPI validation middleware. All Tests Pass: No test failures, confirming that our new tests are well-written and the underlying code is working correctly. Coverage Target Exceeded: The overall project coverage of 96.16% significantly exceeds the minimum requirement of 90%. Robust Testing: The new tests cover various edge cases including: Content type inference for different scenarios |
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.
Hi @oyiz-michael! I really appreciate your patience and effort in making this happen! I've been reviewing the code, tests, and implementation and have some feedback to help make our work easier.
By the way, I love that you're using GenAI to iterate on comments and resolve them; that's fantastic. I use Kiro and Amazon Q extensively in my daily work. However, some tests created with these tools aren't working as expected, and the Form
implementation has some critical issues.
So, what I think make sense to merge this PR is:
1/ The implementation of Form
is working as expected and tests are good. We can keep it.
2/ The implementation of File
has critical bugs and the tests are not covering some critical scenarios. For example, browsers like Safari and some old versions of Chrome can use WebKitFormBoundary
instead of boundary
to define file elements and the code is not covering that. So, lets revert this to _File and remove all the code for this object, keeping only Form
.
Again, thanks a lot for your work and please let me know if you want me to make this refactor and merge this PR.
@leandrodamascena Thank you so much for the detailed review and appreciate your feedback. Thanks again for the thorough review and for helping make this feature production-ready |
|
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.
Hi @oyiz-michael, I made some changes to remove the File
object and everything is ready to be merged!
Thank you so much for working on this, and we'll mention your alias in our release notes when we release a new version.
APPROVED!
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
Issue number: #7024
Summary
Problem Statement: OpenAPI spec generation didn't support File/Form parameters
Solution: Adds minimal, uniform support for file uploads and form fields in OpenAPI generation.
File
params →multipart/form-data
requestBody withformat: binary
Form
params →application/x-www-form-urlencoded
requestBodyChanges
dependant.py
to infer media types from File/Form params-Updated get_body_field_info to set correct media types
_File
inparams.py
to auto-addformat: binary
File = _File
,Form = _Form
User experience
Before
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
No — fully backward-compatible.RFC issue number: N/A
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.