Skip to content
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

Support exclude_if callable at field level #1535

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

andresliszt
Copy link
Contributor

@andresliszt andresliszt commented Nov 8, 2024

Change Summary

This PR aims to add support for excluding field from serialization based on condition. As @davidhewitt suggested I added a new exclude_if argument at field level, which is callable that checks if the field value meets a condition. However, in this pydantic issue David suggested to use skip_serializing_if from Rust's serde, I could't find a way to use it due that serialization is being doing a loop for each field using serialize_map.

Related issue number

pydantic/pydantic#10728

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.63%. Comparing base (ab503cb) to head (a836948).
Report is 237 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1535      +/-   ##
==========================================
- Coverage   90.21%   89.63%   -0.58%     
==========================================
  Files         106      112       +6     
  Lines       16339    17908    +1569     
  Branches       36       40       +4     
==========================================
+ Hits        14740    16052    +1312     
- Misses       1592     1836     +244     
- Partials        7       20      +13     
Files with missing lines Coverage Δ
python/pydantic_core/core_schema.py 94.85% <100.00%> (+0.08%) ⬆️
src/serializers/fields.rs 96.03% <100.00%> (+0.39%) ⬆️
src/serializers/type_serializers/dataclass.rs 85.80% <100.00%> (+0.97%) ⬆️
src/serializers/type_serializers/model.rs 95.40% <100.00%> (+1.52%) ⬆️
src/serializers/type_serializers/typed_dict.rs 97.50% <100.00%> (+0.27%) ⬆️

... and 54 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83ff1cf...a836948. Read the comment docs.

---- 🚨 Try these New Features:

@andresliszt
Copy link
Contributor Author

please review

Copy link

codspeed-hq bot commented Nov 8, 2024

CodSpeed Performance Report

Merging #1535 will not alter performance

Comparing andresliszt:support/exclude-if-at-field-level (a836948) with main (83ff1cf)

Summary

✅ 155 untouched benchmarks

serializer: &CombinedSerializer,
) -> PyResult<bool> {
let py = value.py();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this should be called only if any condition is met, right? inside the if

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably handle this outside of the exclude_default function as to decouple them for readability. We don't couple exclude_default with exclude_none or exclude_unset, so I think we should retain separation here as well!

@sydney-runkle
Copy link
Member

Super exciting! Will give this a thorough review soon. Working on getting v2.10 out, and this will be included in v2.11 :)

Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to see an implementation on the pydantic side as well - you can point to this branch.

Looks like a great start. I'm impressed with how simple this is going to be!

serializer: &CombinedSerializer,
) -> PyResult<bool> {
let py = value.py();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably handle this outside of the exclude_default function as to decouple them for readability. We don't couple exclude_default with exclude_none or exclude_unset, so I think we should retain separation here as well!

@sydney-runkle
Copy link
Member

I would anticipate that folks might want to set this at the model_dump level as well, and I think we should probably support that too, given that's where the other exclude_X specs live.

@andresliszt
Copy link
Contributor Author

Would love to see an implementation on the pydantic side as well - you can point to this branch.

Looks like a great start. I'm impressed with how simple this is going to be!

Hello @sydney-runkle ! How do you do the implementation in pydantic when you need a pydantic-core feature? First create a release of pydantic-core and then use it in pydantic? In this case are you requesting to create a PR in pydantic pointing to this branch to see what it looks like, right?

@andresliszt
Copy link
Contributor Author

I would anticipate that folks might want to set this at the model_dump level as well, and I think we should probably support that too, given that's where the other exclude_X specs live.

I'm curious how the callable should work in the model_dump, are you thinking in a callable with one argument Callable[[Any], bool] to be applied in all fields, for example:

class Model(BaseModel):
    foo: str
    bar: list[str]
    foobaz: int

>>> Model(foo = "", bar = [], foobaz = 1).model_dump(exclude_if = lambda field: not field)
{"foobaz": 1}

@sydney-runkle
Copy link
Member

? In this case are you requesting to create a PR in pydantic pointing to this branch to see what it looks like, right?

Yes, exactly this! Once we merge, we'll include in a minor pydantic-core release, then we can bump the pydantic-core version officially on your pydantic PR and merge that one.

It's just helpful to see the implementation on both sides before we merge the pydantic-core one.

@sydney-runkle
Copy link
Member

I'm curious how the callable should work in the model_dump, are you thinking in a callable with one argument Callable[[Any], bool] to be applied in all fields

Yes, exactly this. That being said, I chatted with the team, and we decided that introducing this at the field level first makes sense, and we can follow with other config / runtime specs later.

@andresliszt andresliszt force-pushed the support/exclude-if-at-field-level branch from d7d7d0e to b225197 Compare November 24, 2024 14:52
@andresliszt
Copy link
Contributor Author

Alright, @sydney-runkle I created a PR pointing to this branch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants