Add support for generics with __get_validators__ #1159

tiangolo · 2020-01-09T19:12:30Z

Change Summary

This adds support for generics that implement __get_validators__ or when arbitrary_types_allowed.

The generic type parameters are saved in field.sub_fields and custom validations using those sub-types are documented.

This would fix/related to #1158

Related issue number

#1158

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
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)

dmontagu · 2020-01-09T19:13:45Z

This is awesome! Substantially easier than I even suspected haha. Let me know if you want help with docs.

samuelcolvin · 2020-01-09T19:16:18Z

LGTM

tiangolo · 2020-01-09T19:18:11Z

Thanks @dmontagu ! I was actually going to ask if we want this to be properly documented or is it enough of a corner case that we prefer to just support it without many docs.

I'm happy to add docs if you think it's reasonable.

samuelcolvin · 2020-01-09T19:18:27Z

LGTM

Except to be consistent we should also allow the case where a generic doesn't implement __get_validator__ but we have "arbitrary types allowed". Is that a separate PR?

dmontagu · 2020-01-09T19:18:53Z

tests/test_edge_cases.py

+        @classmethod
+        def validate(cls, v):
+            if not isinstance(v, cls):
+                raise ValueError('Invalid value')


Do isinstance(v.t1, cls.__args__[0]) and isinstance(v.t2, cls.__args__[1]) work here? I think it might be good to have an example that actually checks the generic types in the validators, for reference by others.

That could just go in the docs, but putting it in the tests too will help make sure it actually works 😅.

(In general you'd maybe want to handle some edge cases better than an isinstance check could, like the unparameterized version or with bounded TypeVars, but for a test/demonstration I think this would be fine.)

Good point, I'm not sure it actually works, because the Field.type_ that is originally the generic is a type alias and the origin is the one that has the __get_validators__, but I'll try to see if it works.

Hmm, I think it would be unfortunate if the implementation prevented you from actually checking the specific parameter types, but maybe that can be fixed later if necessary.

Yeah, just checked.

Sadly, for validate and __get_validators__, cls is a <class '__main__.MyGen'>, not a __main__.MyGen[str, bool].

The way we could implement it would be to send the sub types to the validators provided by __get_validators__ as extra arguments, during validation.

But that would break the current API. I guess it would require another method.

Or, another option is to inspect each validator function returned to see if it requests the types. But that adds extra complexity and I'm not sure how much should we add for this version at least. That option would probably have some performance penalty too, so it might be better just as another method.

It should be possible using the field argument which can optionally be passed to a validator?

Yeah, for performance reasons I think probably it should be the __get_validators__ function that receives the field type, and then that function yields "configured" validators.

field is already available as an argument to a valdator, it will only be provided if the validator takes that argument.

Are you sure it's not possible now?

I hadn't seen these comments before writing my last comment down below. 😅

I'm realizing that __get_validators__ is called on creation, so having some small extra logic for it shouldn't be too bad, as it's called only once on creation, during populate_validators().

Given that, I think I can make it accept an optional FieldModel, very much like in FastAPI (almost everywhere).

The API would look like this:

if __get_validators__ has a signature like:

@classmethod def __get_validators__(cls, field: FieldModel): ...

Then it would receive the FieldModel in the field argument.

Otherwise, it would work as normal (no arguments).

Does that sound okay?

Edit: I think I was talking nonsense and there's nothing extra to implement. Validators seem to always be checked for a field argument, I think it should work.

I'll try and add another shape for generics to distinguish them from tuples and refactor that part. But I think most of what's necessary is already there.

Yeah, given that this is already implemented for validators that seems like the way to go! Awesome!

Suggested change

raise ValueError('Invalid value')

raise TypeError('Invalid value')

samuelcolvin · 2020-01-09T19:19:20Z

Thanks @dmontagu ! I was actually going to ask if we want this to be properly documented or is it enough of a corner case that we prefer to just support it without many docs.

I'm happy to add docs if you think it's reasonable.

I'm neutral, I agree it's a corner case you would expect to work from the current docs, but might be worth adding a note to the custom types section just mentioning this?

codecov · 2020-01-09T19:20:45Z

Codecov Report

Merging #1159 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1159   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          20      20           
  Lines        3479    3490   +11     
  Branches      674     679    +5     
======================================
+ Hits         3479    3490   +11

Impacted Files	Coverage Δ
pydantic/fields.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

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

tiangolo · 2020-01-09T19:22:48Z

Except to be consistent we should also allow the case where a generic doesn't implement get_validator but we have "arbitrary types allowed". Is that a separate PR?

So, if arbitrary_types_allowed is set but __get_validators__ is not in the class, should we still accept the generic?

I'm happy with supporting arbitrary_types_allowed as well if you think that's acceptable, and it probably solves better the current issue.

dmontagu · 2020-01-09T19:31:38Z

So, if arbitrary_types_allowed is set but get_validators is not in the class, should we still accept the generic?

I think so, but I'm wondering which cases we are ruling out with this line: https://github.com/samuelcolvin/pydantic/blob/1c49a3f689a959bec76f0b879a20510883938579/pydantic/fields.py#L422

I can't easily find the test that covers this -- any ideas how to find it? 😅

samuelcolvin · 2020-01-09T19:33:23Z

So, if arbitrary_types_allowed is set but __get_validators__ is not in the class, should we still accept the generic?

I was going to say "yes definitely", but I realise it's more complicated than that, see below.

I can't easily find the test that covers this -- any ideas how to find it? sweat_smile

put a fat raise RuntimeError('snap') on that line and run tests. 🔫

dmontagu · 2020-01-09T19:34:45Z

put a fat raise RuntimeError('snap') on that line and run tests. 🔫

duh 🤦‍♂️

Found it: https://github.com/samuelcolvin/pydantic/blob/1c49a3f689a959bec76f0b879a20510883938579/tests/test_types.py#L1910-L1914

I would personally vote that we allow such types (again, only assuming allow_arbitrary_types is on) and just don't validate them, but I could be convinced either way.

samuelcolvin · 2020-01-09T19:38:19Z

I would personally vote that we allow such types

Humm, the problem is people assuming pydantic is doing validation when it's actually not doing much.

We could:

add another flag 😴
check whether the type is part of the standard libary or custom, seems weird
give a big far warning in the docs

No option there seems great to me, not sure what to do.

tiangolo · 2020-01-09T19:47:58Z

Yeah, I kind of agree with everything... not sure what would be the best option here.

On the other side, these type vars aren't only for class creation, for example:

InT = TypeVar('InT')
OutT = TypeVar('OutT')

class MyGen2(Generic[InT, OutT]):
    def __init__(self, input: InT):
        self.input = input
    
    def output() -> OutT:
        # Something that returns OutT
        return

g = MyGen2[str, bool](input="asdf")

g.output()  # mypy infers this is a bool

Here the OutT in the generic wouldn't have anything to do with the creation of the class itself. So it wouldn't/shouldn't affect how Pydantic takes it.

tiangolo · 2020-01-09T19:53:57Z

And also, another option could be something like an alternative __get_field_validators__ that yields validator functions that take the value plus the original Field.

That way the validator function could take care of checking the Field and extracting any declared subtypes it needs to perform validation.

Although this option would mean another custom dunder and a bigger refactor, because the generic type args are in the original type, but the __get_validators__ is in the .__origin__ without args (this one is what I'm using here).

dmontagu · 2020-01-09T20:02:10Z

Hmm, good investigation @tiangolo. I'll see if I can think of any clean way of resolving all of this, but given it probably won't be super straightforward, I'm personally happy with whatever will get the functionality working for you now.

Presumably if we can find a way to smuggle in the field information into __get_validators__ or a related new method, it won't need to be a breaking change, and validation based on the type parameters in a custom generic should probably be opt in anyway. So I don't see a good reason to block merging on figuring this out.

tiangolo · 2020-01-09T20:13:10Z

Thanks @dmontagu ! 🚀

Also, if this is all restricted to the user setting allow_arbitrary_types, by that point she/he would have to have read the warning in the docs about "not checking generics subtypes" (that I'll add to the docs later if we chose to support this). Given that, I would agree that would be fine, but I'm still fine if __get_validators__ is still required.

Another mitigation I can do is make an explicit example in the docs that doesn't use the type arguments from the generic on instantiation, that should help to avoid giving the idea that those types are checked.

I'll wait before adding allow_arbitrary_types (or not) and documenting how it works.

@samuelcolvin what do you think? It's your call 👑

samuelcolvin · 2020-01-09T20:32:58Z

I think this is fine, just the question above about whether it's already possible to validate the generic class's type.

tiangolo · 2020-01-09T22:57:01Z

I think I got it. Including tests for generic subtypes. I'll add docs and update the PR.

Meanwhile, check this: 🎉 🥇

tiangolo · 2020-01-10T00:51:11Z

I think it's working quite well now. Including optional/custom validation for generic type parameters 🎉

I just had to add the field arg to the validator for it to get the Field instance. All the logic for that was already there.

So, given that, I refactored it all to have a new shape for generics, include sub-types, perform validation for the whole generic (not per-sub-type), etc.

I'm using SHAPE_GENERIC = 10 instead of SHAPE_GENERIC = 9 to keep this PR compatible with #1152. But let me know if you prefer to change their numbers (or feel free to change it).

tiangolo · 2020-01-10T00:54:02Z

Check the new docs here:

I added a small explicit section about arbitrary_types_allowed: https://deploy-preview-1159--pydantic-docs.netlify.com/usage/types/#allow-arbitrary-types . This was mainly to disambiguate and not give the impression that to use custom types they had to be generic, so this clarifies that custom (non-generic) types are supported too.

And one for Generic types with validation: https://deploy-preview-1159--pydantic-docs.netlify.com/usage/types/#generic-classes-as-types

docs/examples/types_generics.py

docs/usage/types.md

pydantic/fields.py

samuelcolvin · 2020-01-10T20:35:45Z

tests/test_edge_cases.py

+        @classmethod
+        def validate(cls, v):
+            if not isinstance(v, cls):
+                raise ValueError('Invalid value')


Suggested change

raise ValueError('Invalid value')

raise TypeError('Invalid value')

tests/test_edge_cases.py

tiangolo · 2020-01-11T14:49:06Z

Thanks for the thorough code review @samuelcolvin , I just finished all the changes.

* ✨ Add support for generics with __get_validators__ * ✅ Add tests for Generics with __get_validators__ * 📝 Add change note * ✨ Add support for Generic fields with validation of sub-types * 📝 Add docs for arbitrary generic types * ✅ Add tests for generic sub-type validation * 📝 Update change note. Generic support is not so "basic" now * 📝 Update docs with code review * ♻️ Update fields module with code review changes * ✅ Update tests from code review * 📝 Update example for generics, try to simplify and explain better * tweak docs example Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

tiangolo added 2 commits January 9, 2020 20:04

✨ Add support for generics with __get_validators__

f59d4ba

✅ Add tests for Generics with __get_validators__

ff48c3f

📝 Add change note

556646a

dmontagu reviewed Jan 9, 2020

View reviewed changes

tiangolo mentioned this pull request Jan 9, 2020

WIP: Fix small misconfigs in example, initial pass explosion/thinc#195

Merged

ines mentioned this pull request Jan 9, 2020

Support for arbitrary generic types in fields #1158

Closed

tiangolo added 4 commits January 10, 2020 01:41

✨ Add support for Generic fields with validation of sub-types

6b663a3

📝 Add docs for arbitrary generic types

23e5a44

✅ Add tests for generic sub-type validation

322392a

📝 Update change note. Generic support is not so "basic" now

afe2156

samuelcolvin reviewed Jan 10, 2020

View reviewed changes

tiangolo added 4 commits January 11, 2020 15:45

📝 Update docs with code review

ca90e71

♻️ Update fields module with code review changes

4ed2e4e

✅ Update tests from code review

ed3be2f

📝 Update example for generics, try to simplify and explain better

1aef357

samuelcolvin added 2 commits January 13, 2020 11:52

Merge branch 'master' into generic-with-get-validators

0e21fa3

tweak docs example

4fb887c

samuelcolvin merged commit aeba494 into pydantic:master Jan 17, 2020

tiangolo deleted the generic-with-get-validators branch January 17, 2020 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for generics with __get_validators__ #1159

Add support for generics with __get_validators__ #1159

tiangolo commented Jan 9, 2020 •

edited

dmontagu commented Jan 9, 2020 •

edited

samuelcolvin commented Jan 9, 2020

tiangolo commented Jan 9, 2020

samuelcolvin commented Jan 9, 2020

dmontagu Jan 9, 2020 •

edited

tiangolo Jan 9, 2020

dmontagu Jan 9, 2020

tiangolo Jan 9, 2020

samuelcolvin Jan 9, 2020

dmontagu Jan 9, 2020

samuelcolvin Jan 9, 2020

tiangolo Jan 9, 2020 •

edited

dmontagu Jan 9, 2020

samuelcolvin Jan 10, 2020

samuelcolvin commented Jan 9, 2020

codecov bot commented Jan 9, 2020 •

edited

tiangolo commented Jan 9, 2020 •

edited

dmontagu commented Jan 9, 2020 •

edited

samuelcolvin commented Jan 9, 2020 •

edited

dmontagu commented Jan 9, 2020 •

edited

samuelcolvin commented Jan 9, 2020

tiangolo commented Jan 9, 2020

tiangolo commented Jan 9, 2020

dmontagu commented Jan 9, 2020 •

edited

tiangolo commented Jan 9, 2020

samuelcolvin commented Jan 9, 2020

tiangolo commented Jan 9, 2020

tiangolo commented Jan 10, 2020

tiangolo commented Jan 10, 2020

samuelcolvin Jan 10, 2020

tiangolo commented Jan 11, 2020

	raise ValueError('Invalid value')
	raise TypeError('Invalid value')

Add support for generics with __get_validators__ #1159

Add support for generics with __get_validators__ #1159

Conversation

tiangolo commented Jan 9, 2020 • edited

Change Summary

Related issue number

Checklist

dmontagu commented Jan 9, 2020 • edited

samuelcolvin commented Jan 9, 2020

tiangolo commented Jan 9, 2020

samuelcolvin commented Jan 9, 2020

dmontagu Jan 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiangolo Jan 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelcolvin commented Jan 9, 2020

codecov bot commented Jan 9, 2020 • edited

Codecov Report

tiangolo commented Jan 9, 2020 • edited

dmontagu commented Jan 9, 2020 • edited

samuelcolvin commented Jan 9, 2020 • edited

dmontagu commented Jan 9, 2020 • edited

samuelcolvin commented Jan 9, 2020

tiangolo commented Jan 9, 2020

tiangolo commented Jan 9, 2020

dmontagu commented Jan 9, 2020 • edited

tiangolo commented Jan 9, 2020

samuelcolvin commented Jan 9, 2020

tiangolo commented Jan 9, 2020

tiangolo commented Jan 10, 2020

tiangolo commented Jan 10, 2020

Choose a reason for hiding this comment

tiangolo commented Jan 11, 2020

tiangolo commented Jan 9, 2020 •

edited

dmontagu commented Jan 9, 2020 •

edited

dmontagu Jan 9, 2020 •

edited

tiangolo Jan 9, 2020 •

edited

codecov bot commented Jan 9, 2020 •

edited

tiangolo commented Jan 9, 2020 •

edited

dmontagu commented Jan 9, 2020 •

edited

samuelcolvin commented Jan 9, 2020 •

edited

dmontagu commented Jan 9, 2020 •

edited

dmontagu commented Jan 9, 2020 •

edited