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

Replace custom 'hidden=True' field attribute with builtin 'exclude=True' #741

Merged
merged 2 commits into from Oct 22, 2023

Conversation

gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Oct 14, 2023

Replace the custom hidden Pydantic Field attribute with exclude and remove a bunch of code needed to support it.

@roman-right
Copy link
Owner

Hi,
Thank you for the PR!
This is a breaking change. Please add a check and a warning log in case the hidden parameter was used.

@gsakkis
Copy link
Contributor Author

gsakkis commented Oct 15, 2023

Restored backwards compatibility by transparently converting hidden field to exclude and logging a DeprecationWarning.

props[k] = v
schema["properties"] = props
# remove excluded fields from the json schema
properties = schema.get("properties")
Copy link
Owner

Choose a reason for hiding this comment

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

I don't remember how this call is implemented in Pydantic. Could you please check if the schema was not given as a mutable object here? If so, modifying it can lead to unexpected errors - it is better to clone it then.
I'll double-check this point from my side too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly sure what you mean, schema is a mutable object (dict) as shown in the annotation. The json_schema_extra callable returns None so it's only called for its side-effects, mutating schema. It would have no effect if the schema was cloned.

The calling code in Pydantic v2 is here: https://github.com/pydantic/pydantic/blob/main/pydantic/json_schema.py#L1364-L1368

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, you are right. This is expected behavior.

@roman-right roman-right merged commit a6f1b8d into roman-right:main Oct 22, 2023
21 checks passed
@roman-right
Copy link
Owner

Merged. It will be published tomorrow

@roman-right
Copy link
Owner

Thank you for the PR!

@gsakkis gsakkis deleted the replace-hidden-with-exclude branch December 10, 2023 08:52
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.

None yet

2 participants