-
Notifications
You must be signed in to change notification settings - Fork 455
feat(event_handler): add support for Pydantic models in Query and Header types #7253
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 Pydantic models in Query and Header types #7253
Conversation
d584a8d
to
f9141de
Compare
Hey @tonnico ! Thanks for opening this PR, we'll review it and get back to you |
Hey @tonnico thanks a lot for working on this. I see we have some errors in the CI, can you fix them please and then we can start reviewing the code? |
I apologize to overlook I still see some improvements and will fix the remaining issues. |
131310e
to
d3b46be
Compare
@leandrodamascena I fixed code smell and the remaining typing issues. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7253 +/- ##
===========================================
+ Coverage 96.42% 96.45% +0.03%
===========================================
Files 275 275
Lines 13055 13115 +60
Branches 974 987 +13
===========================================
+ Hits 12588 12650 +62
+ Misses 362 360 -2
Partials 105 105 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks @leandrodamascena for the good starting point from your initial PR. I just fixed the merge conflict. |
Reviewing this now. |
|
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 @tonnico! Thank you so much for your patience while waiting for my final review. I went through 4 or 5 rounds of review. I tried to create a scenario where I could break the code/functionality, and fortunately, I couldn't. I ran e2e tests with this in different scenarios.
This code is complex. You've made some important refactorings, and I can think of two or three different ways to do this implementation with the current codebase. But, to be honest, I don't know if I could do it better than you did here. The abstraction you create for some functions, the refactoring to reduce complexity, and the clarity of the code are very good. I hope I'm not missing any major potential problem I haven't seen, but our current tests are passing and that brings more peace.
That's really a super PR, thanks a lot. I sent a commit with some changes in the doc and we are ready to merge tomorrow or Friday.
THANKS A LOT! APPROVED!!!!!
Wow, thanks so much for your review and the nice feedback! Appreciate all the time you spent testing and checking different scenarios. Glad it held up. Looking forward to the merge! |
|
|
|
Merge conflicts resolved after upstream PRs aws-powertools#7227 and aws-powertools#7253: Core Changes: - Updated File class rename from File -> _File with public File alias - Fixed Union import for _resolve_field_type function - Removed unused _File import in dependant.py - Maintained all UploadFile and validation functionality Testing: - All 25 comprehensive tests passing - 96.36%+ codecov coverage maintained - Code quality checks passing (make format && make pr) Compatibility: - Preserves public File API for backward compatibility - Maintains UploadFile OpenAPI schema generation - All validation middleware features intact
Issue number: closes #7039
closes #5325
Summary
Changes
You can use pydantic BaseModel for Query and Header.
User experience
Grouping Query or Header params in one
BaseModel
.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.