Validate bytes based on ser_json_bytes #1308
Conversation
|
please review |
|
CC @jcharum |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1308 +/- ##
==========================================
- Coverage 90.21% 89.61% -0.60%
==========================================
Files 106 109 +3
Lines 16339 17304 +965
Branches 36 41 +5
==========================================
+ Hits 14740 15507 +767
- Misses 1592 1777 +185
- Partials 7 20 +13
... and 33 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
I'm also interested in accepting both standard and URL-safe base64 encoding. If this PR is acceptable, I plan to send a separate one for that: d8e-ai/pydantic-core@validate-base64...d8e-ai:pydantic-core:validate-base64-any |
josh-newman
force-pushed
the
validate-base64
branch
from
4c44225 to
f8addc1
Compare
CodSpeed Performance ReportMerging #1308 will not alter performanceComparing Summary
|
|
Thanks for the PR! I fully support the use case and think this makes sense. That said, I worry about silently breaking user code by changing the meaning of the existing option in this way. How about adding a new flag, e.g. Or we could add |
|
Either option works for me. I don't currently know of a use case where someone would want base64 encoding only and not decoding, so I'd lean towards the new bidirectional encoding flag. But maybe that use case exists, or maybe there's a pattern in other Pydantic config to follow (that I'm not familiar with)? I'm happy to make changes for whichever you recommend! (And please feel free to make changes yourself, too, if you prefer.) |
|
Having discussed with @sydney-runkle and @samuelcolvin, I think we would prefer to go with a new I agree that the bidirectional encoding flag is probably what most people actually need, so if you strongly want that I'd probably accept it; with the individual flags it should be an easy layer on top which just sets both of them (and we can error if it is set as well as either individual flag). It's a shame to have the complexity of both forms, but hey, it's where we've ended up. It's possible we could consider deprecating the individual flags in V3. |
josh-newman
force-pushed
the
validate-base64
branch
from
6e7fc01 to
f7ca32f
Compare
josh-newman
force-pushed
the
validate-base64
branch
from
f7ca32f to
1e71ab7
Compare
|
Sounds good, I've switched to a new I'm guessing there's some more work to do with the new config key (docs, Pydantic's |
There was a problem hiding this comment.
Thanks, looks good! I'd like to see a couple of changes for consistency with the rest of the library...
As for updating the Python code in pydantic, yes that's a good idea. I'd suggest opening a PR already, you can test it all locally and then add pytest.xfail markers in the PR which we can then remove when this support gets released in pydantic-core.
|
I've created pydantic/pydantic#9770. I'm guessing it needs to wait until after the pydantic-core upgrade, otherwise a user may see this new option and be confused about why it doesn't work? |
|
I've created pydantic/pydantic#9772 to address the test-pydantic-integration failure. |
|
Well, I think I'm stuck 😅. pydantic/pydantic#9772 has test failures saying the new documented error type isn't in pydantic-core, and this PR has a CI failure saying the new error type isn't documented there. |
src/serializers/mod.rs
Outdated
| @@ -16,7 +16,7 @@ pub use shared::CombinedSerializer; | |||
| use shared::{to_json_bytes, BuildSerializer, TypeSerializer}; | |||
|
|
|||
| mod computed_fields; | |||
| mod config; | |||
| pub(crate) mod config; | |||
There was a problem hiding this comment.
ValBytesMode wraps this so it needs to be visible. I've narrowed the pub(crate) exposure to just BytesMode; is that ok?
I'm new to Rust so I don't know if there's a better way than wrapping. I want to make sure ValBytesMode defines the same variants as BytesMode and this seems to be the only way (other than maybe with macros).
There was a problem hiding this comment.
I guess my thought of a common module would solve this #1308 (comment), just move BytesMode in there and have it know both serialization and deserialization.
|
This looks good to me. We should get @davidhewitt's feedback on #1308 (comment) before we merge 👍 |
|
Once you merge, I can create a new release of |
Change Summary
ser_json_bytestransforms values (with base64 encoding) during serialization. But validation doesn't do a complementary base64 decode, so a serialization round-trip into the same model type yields an unequal object.Related issue number
None for this directly. Other users have mentioned base64 decoding, though: pydantic/pydantic#7000 (comment)
Checklist
pydantic-core(except for expected changes)Selected Reviewer: @davidhewitt