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

Fix issue with unspecified generic type #554

Merged
merged 7 commits into from May 29, 2019

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented May 26, 2019

With this change, models with bare List or Dict as a typehint still validate for type agreement, but don't validate the type of the parameters.

Change Summary

Checks for TypeVar when looking up validators.
#550

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>

@@ -383,14 +383,16 @@ def find_validators(type_: AnyType, arbitrary_types_allowed: bool = False) -> Li

for val_type, validators in _VALIDATORS:
try:
if issubclass(type_, val_type):
if lenient_issubclass(type_, val_type):
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 this isn't imported.

@samuelcolvin
Copy link
Member

thanks for this, could you also fill out the checklist on the pull request template.

@codecov
Copy link

codecov bot commented May 26, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #554   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2457   2453    -4     
  Branches      488    488           
=====================================
- Hits         2457   2453    -4

@dmontagu
Copy link
Contributor Author

@samuelcolvin I updated the checklist, but I'm not sure if this fix merits any updates to the documentation. Let me know if you'd like me to add anything.

pydantic/validators.py Outdated Show resolved Hide resolved

# This test passes -- should it fail, or is this the desired behavior? (It would make sense to me either way)
# It also passes if I replace Dict with Dict[int, int], so no new problems have been introduced.
assert Model(generic_list=[], generic_dict=[])
Copy link
Member

Choose a reason for hiding this comment

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

This should definitely fail with the error as below.

However I've just check and it's already wrong on master so should be fixed separately. #558

@dmontagu
Copy link
Contributor Author

dmontagu commented May 29, 2019

@samuelcolvin The second to last commit implements the changes you asked for, and I think is basically ready for merging (after removing the bad test).

However, I did look into preventing the empty list from passing dict validation. Ultimately, I think the question is whether you want to support converting iterables of pairs to dictionary. If so, we'd probably have to let it convert empty list to empty dict, or it would break whenever the list of pairs just happened to be empty.

I'm happy to revert the current last commit on this branch and delete the mock test so this can be merged, and possibly start another issue separately, but I wanted to at least get your thoughts on the empty-list-to-dict validation issue before wrapping this up. (Would be easy to combine with this PR if desired.)

@dmontagu
Copy link
Contributor Author

@samuelcolvin I think it's ready to merge

dmontagu and others added 7 commits May 29, 2019 11:24
Seems to solve pydantic#550

With this change, models with bare `List` or `Dict` as a typehint still validate for type agreement, but don't validate the type of the parameters.

I'm not sure this is the "right" fix (I don't know the implications of ignoring TypeVars like this), but considering how simple it was I figured I'd at least share.
Found this discussion: pydantic#545; `lenient_issubclass` does seem to fix it.
Hacky solution to prevent no validator exception. Maybe there's a better way?
@dmontagu
Copy link
Contributor Author

Now rebased onto master

@samuelcolvin samuelcolvin merged commit af26f7f into pydantic:master May 29, 2019
@samuelcolvin
Copy link
Member

thank a lot.

gangefors added a commit to gangefors/pydantic that referenced this pull request May 31, 2019
* upstream/master: (138 commits)
  add 'none-any.whl' to pypi upload (pydantic#564)
  uprev
  update benchmarks (pydantic#563)
  cython (pydantic#548)
  Fix issue with unspecified generic type (pydantic#554)
  Run dataclass' original __post_init__ before validation (pydantic#560)
  try to stop annoying warnings in azure pipeline (pydantic#549)
  azure pipeline failOnStderr: false
  Azure Pipelines - tests for windows (pydantic#538)
  Fix JSON Schema for list, tuple, and set, improving interoperability (pydantic#540)
  uprev.
  Colors (pydantic#516)
  Fix to schema generation for IPv{4,6}{Address,Interface,Network} (pydantic#532)
  Fix __fields_set__ not using alias field names (pydantic#517) (pydantic#518)
  Change return type hint for create_model (pydantic#526)
  Tuple ellipsis (pydantic#512)
  Fix to schema generation for IPvAny{Address,Interface,Network} (pydantic#498) (pydantic#510)
  uprev
  Scheduled monthly dependency update for May (pydantic#499)
  Implement const keyword in Schema. (pydantic#469)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants