-
Notifications
You must be signed in to change notification settings - Fork 408
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: Advanced parser utility (pydantic) #118
Conversation
This pull request introduces 5 alerts when merging cf1e795 into 81539a0 - view on LGTM.com new alerts:
|
open issues:
|
Codecov Report
@@ Coverage Diff @@
## develop #118 +/- ##
===========================================
- Coverage 99.86% 98.69% -1.17%
===========================================
Files 52 64 +12
Lines 2154 2380 +226
Branches 97 109 +12
===========================================
+ Hits 2151 2349 +198
- Misses 3 24 +21
- Partials 0 7 +7
Continue to review full report at Codecov.
|
fcc20f6
to
3a68baa
Compare
3d8ba80
to
7da981f
Compare
* develop: (26 commits) docs: move tenets; remove extra space docs: use table for clarity docs: add blog post, and quick example docs: subtle rewording for better clarity docs: fix typos, log_event & sampling wording docs: make sensitive info more explicit with an example docs: create Patching modules section; cleanup response wording docs: move concurrent asynchronous under escape hatch chore: update internal docstrings for consistency fix: remove actual response from debug logs docs: grammar docs: bring new feature upfront when returning sensitive info chore: update changelog to reflect new feature chore: clarify changelog bugfix vs breaking change chore: remove/correct unnecessary debug logs chore: fix debug log adding unused obj fix: naming and staticmethod consistency improv: naming consistency fix: correct in_subsegment assertion feat: capture_response as metadata option #127 ...
* develop: docs: Fix doc for log sampling (#135) fix(logging): Don't include `json_default` in logs (#132) chore: bump to 1.4.0 docs: add Lambda Layer SAR App url and ARN fix: upgrade dot-prop, serialize-javascript fix heading error due to merge formatting for bash script add layer to docs and how to use it from SAR moved publish step to publish workflow after pypi push fix(ssm): Make decrypt an explicit option and refactoring (#123) change to eu-west-1 default region remove tmp release flag and set trigger to release published add overwrite flag for ssm add relase tag simulation more typos fix typo in branch trigger fix indent, yaml ... line endings
Notes to self to review on Friday
|
Is there a possibility of implementing event schema validation using the existing fastjsonschema dependency instead of introducing pydantic? |
@jplock yes you can, but pydantic is much more than just a schema validator. It gives the users advanced validation capabilities, more type checking (types that JSON doesnt provide), custom logic checks on values (other than empty or null), validate custom relationships between parameters and more. In this PR , we dont only validate but give back the parsed object (a pydantic dataclass basically). We also give the users an option to just validate and dont get back a parsed object. |
This pull request introduces 1 alert when merging bce7aab into 8da0cce - view on LGTM.com new alerts:
|
@jplock suppose we were to use Pydantic (parser+validation), how would you see yourself using this feature? If not, what would be helpful? I'm still awe with Pydantic flexibility, the ability to provide runtime safety for customers, and providing auto-complete for customers on common event sources they typically hop onto multiple doc pages to find the right format. However, the UX is not easy to use yet. I'm looking at the PR more carefully tomorrow, and will block Monday afternoon to think about ways to simplify this, as Ran did a great job in pulling this together already.
Pros
Cons
I fear we might add a powerful feature that not many customers will be able to leverage it to its full extent. If we can't think of a way to find a middle ground by early next week, this will be a good opportunity to break this into two simpler utilities (parser, validator), and add simplicity to this project Tenets. |
@heitorlessa i like the idea of event schema validation but pydantic feels too heavy for me. All I’d want to do is validate a schema and get a dict. I wouldn’t personally use the full models. That’s why I thought we could build this feature using fastjsonschema as a first release. |
@heitorlessa - i don't like any having any kind of extra dependencies. Currently with no extra dependencies we are already at 8.3M I see a few options:
I prefer a combination of 1 and 3. Current dependencies:
|
Thanks @jplock and @michaelbrewer for your inputs, much appreciated. We'll review this today with the core team and circle back with a solution - As of now, this doesn't meet our Keep It Lean tenet however useful this is. |
I'll add my two cents here if you dont mind :) Regarding the usage -I added per @heitorlessa 's request the validate function. It's not the decorator. it's a simpler version that take a schema and returns a parsed object, just like you wanted. Regarding the extra size, as an end user, if i'm getting good performance and more usability options, i really dont mind. I think this repo can set a high standard for validation. It's a great opportunity and I believe that pydantic can help that. |
The format is now solved upstream in develop - rebasing should work. For the Pytest on examples, it's best to completely remove the example folder from the repo, since we now have a cookiecutter for that purpose. I'll push that change to develop later and ping here for rebasing |
00e8844
to
eb3153f
Compare
eb3153f
to
38e1582
Compare
@heitorlessa all ready for ya ;) |
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.
New user experience looks GREAT! Added some comments to help us maintain as not all maintainers are well versed with Pydantic, so any additional context helps.
aws_lambda_powertools/utilities/advanced_parser/envelopes/event_bridge.py
Show resolved
Hide resolved
Merge branch 'develop' of https://github.com/awslabs/aws-lambda-powertools-python into pydantic
THANK YOU @risenberg-cyberark for all those changes, UX improvements, and additional comments you've made. I'm gonna merge this as-is now. As agreed, I'll make a PR to write the docs for this feature and get your review before merging it. As you also spent a ton of effort already, I'll create another PR the following minor changes:
|
@heitorlessa can you maybe put them in comments? i'd hate for that logic to be forgotten. These are connections that json schemas cant really define (only documentation). |
Sure thing - Commented them out, and added a link to this discussion for history purposes. |
chore: ease maintenance of upcoming parser #118
@heitorlessa are the extras required for this functionality included in the SAR app / lambda layer? Or do those need to be handled separately? Might be worth adding a note in the docs, not sure if that would be under the parser docs or the lambda layer installation docs? |
Hey @mwarkentin - the latter, handled separately as described in the parser docs. Unsure whether it worth creating a separate Layer for that tbh given the additional operational overhead. In V2, we'd like to create each utility to be pip installable if a theory works, then we could consider multiple layers I suppose. Happy to discuss more in a separate issue tho ;) |
Issue, if available: #147, #95
Description of changes:
Added a new validation module. It has the validator decorator code with 3 envelopes (eventbridge, dynamoDB and custom user) and tests. Also has the eventbridge & dynamoDB schemas.
Checklist
Breaking change checklist
**RFC issue #95 :
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.