Skip to content

DSL: preserve the skip_empty setting in to_dict() recursive serializations #3041

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

Merged
merged 3 commits into from
Aug 20, 2025

Conversation

pquentin
Copy link
Member

I wanted to fix elastic/elasticsearch-dsl-py#1577, but realized I wasn't able to reproduce it.

@miguelgrinberg
Copy link
Contributor

@pquentin added a 2nd commit with a small adjustment to your attempt that does reproduce the issue. The 3rd commit contains the fix, which involves the plumbing to pass skip_empty through recursive serializations.

@miguelgrinberg miguelgrinberg force-pushed the reproduce-issue-1577 branch 3 times, most recently from 477b784 to 6c4f43d Compare August 14, 2025 10:24
@miguelgrinberg miguelgrinberg changed the title Try reproducing DSL issue 1577 respect to_dict's skip_empty setting in recursive serializations Aug 14, 2025
@miguelgrinberg miguelgrinberg changed the title respect to_dict's skip_empty setting in recursive serializations DSL: preserve the skip_empty setting in to_dict() recursive serializations Aug 14, 2025
Copy link
Member Author

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. It does fix the issue.

(My approval does not count for GitHub as the original author of the PR.)

Comment on lines +125 to +131
def _safe_serialize(self, data: Any, skip_empty: bool) -> Any:
try:
return self._serialize(data, skip_empty)
except TypeError:
# older method signature, without skip_empty
return self._serialize(data) # type: ignore[call-arg]

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we need this? Is this for third-party libraries that want to subclass Field?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed for people who created their own custom fields and use the old signature in their _serialize() methods.

@pquentin pquentin merged commit 4761d56 into elastic:main Aug 20, 2025
19 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 20, 2025
…lizations (#3041)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------

Co-authored-by: Miguel Grinberg <[email protected]>
(cherry picked from commit 4761d56)
github-actions bot pushed a commit that referenced this pull request Aug 20, 2025
…lizations (#3041)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------

Co-authored-by: Miguel Grinberg <[email protected]>
(cherry picked from commit 4761d56)
github-actions bot pushed a commit that referenced this pull request Aug 20, 2025
…lizations (#3041)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------

Co-authored-by: Miguel Grinberg <[email protected]>
(cherry picked from commit 4761d56)
github-actions bot pushed a commit that referenced this pull request Aug 20, 2025
…lizations (#3041)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------

Co-authored-by: Miguel Grinberg <[email protected]>
(cherry picked from commit 4761d56)
miguelgrinberg added a commit that referenced this pull request Aug 20, 2025
…lizations (#3041) (#3047)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------


(cherry picked from commit 4761d56)

Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Miguel Grinberg <[email protected]>
miguelgrinberg added a commit that referenced this pull request Aug 20, 2025
…lizations (#3041) (#3046)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------


(cherry picked from commit 4761d56)

Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Miguel Grinberg <[email protected]>
miguelgrinberg added a commit that referenced this pull request Aug 20, 2025
…lizations (#3041) (#3044)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------


(cherry picked from commit 4761d56)

Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Miguel Grinberg <[email protected]>
miguelgrinberg added a commit that referenced this pull request Aug 20, 2025
…lizations (#3041) (#3045)

* Try reproducing DSL issue 1577

* better attempt to reproduce

* preserve skip_empty setting in recursive serializations

---------


(cherry picked from commit 4761d56)

Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Miguel Grinberg <[email protected]>
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.

to_dict(skip_empty=False) doesn't work recursively
2 participants