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

Add optional field argument to __modify_schema__ #3434

Merged
merged 2 commits into from Dec 18, 2021

Conversation

jasujm
Copy link
Contributor

@jasujm jasujm commented Nov 21, 2021

Change Summary

Add optional field parameter to model.__modify_schema__() to opt in to receiving the ModelField object of the field. This makes it possible e.g. for the implementation of __modify_schema__() of generic type to know the runtime type of its subfields.

The concept is inspired by the optional parameters in validators, implemented in the pydantic.class_validators module. There are some differences:

  • Only field is supported. values does not make sense in this context, since validation deals with concrete object, and this with the class itself. Model config might be supported, but is not included in this PR.
  • field is Optional[ModelField] as opposed to ModelField, unlike validators. This is because the schema methods can be used on things that are not model fields, in which case None will be passed to as the field argument.
  • Whereas class_validators does plenty of checks on the validator method signature before wrapping it, my implementation simply checks for the presence of field, and calls __modify_schema__() with or without the field parameter. This is because with validators there is separate prepare and execute steps, whereas __modify_schema__() is called directly when generating schema. If the signature doesn't match the expected, the error message and stack trace will be clear enough.

Related issue number

There are multiple requests for this:

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100% (expected to pass since actually running on CI is gate keeped)
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@@ -87,6 +87,16 @@
TypeModelSet = Set[TypeModelOrEnum]


def _apply_modify_schema(modify_schema: Callable, field: ModelField, schema: Dict[str, Any]):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The field should be Optional[ModelField] and not ModelField, since __modify_schema__() is not only called for fields. If it's called for entire model, there is no ModelField in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in newer revision ☝️

@jasujm jasujm changed the title WIP: Add optional field argument to __modify_schema__ Add optional field argument to __modify_schema__ Nov 27, 2021
@jasujm jasujm marked this pull request as ready for review November 27, 2021 17:50
@jasujm
Copy link
Contributor Author

jasujm commented Nov 27, 2021

please review

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

Comment on lines 134 to 137
You can also add any of the following arguments to the signature to use them in the implementation:

* `field`: the field whose schema is customized. Type of object is `pydantic.fields.ModelField`.
* `**kwargs`: if provided, the above argument will be provided in they `kwargs` dictionary
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can also add any of the following arguments to the signature to use them in the implementation:
* `field`: the field whose schema is customized. Type of object is `pydantic.fields.ModelField`.
* `**kwargs`: if provided, the above argument will be provided in they `kwargs` dictionary
`__modify_schema__` can also take a `field` argument which will have type `Optional[ModelField]`, pydantic will inspect the signature of `__modify_schema__` to determine
whether the `field` argument should be included.

Or something. Explaining about **kwargs is overkill here I think.

Might also be worth adding an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified the documentation as suggested, and added an example



@pytest.mark.skipif(
sys.version_info < (3, 7), reason='schema generation for generic fields is not available in python < 3.7'
Copy link
Member

Choose a reason for hiding this comment

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

The test doesn't need to use a generic, so can remove the skipif

Copy link
Member

Choose a reason for hiding this comment

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

If there's a good reason to add a test for a generic here too, you can do that as a separate test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No there's no reason for it to be generic, so changed the test to work without them


class GenModel(Generic[T]):
@classmethod
def __modify_schema__(cls, field_schema, field):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def __modify_schema__(cls, field_schema, field):
def __modify_schema__(cls, field_schema, field):
assert isinstance(field, ModelField)

@samuelcolvin
Copy link
Member

please update

@jasujm
Copy link
Contributor Author

jasujm commented Dec 11, 2021

Thank you for the suggestions. I updated and rebased the PR accordingly. please review

@samuelcolvin samuelcolvin merged commit 63337fb into pydantic:master Dec 18, 2021
@samuelcolvin
Copy link
Member

thanks so much.

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

3 participants