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(validate-assignment): do not validate extra fields when `validate_assignment` is on #724

Merged
merged 4 commits into from Aug 8, 2019

Conversation

@YaraslauZhylko
Copy link
Contributor

commented Aug 7, 2019

Change Summary

Do not validate extra fields when validate_assignment option is set to True.

This prevent KeyError to be raised when trying to add an extra field.

Also, it seems more logical to validate only those fields that are explicitly set for the model.

See related bug description for details.

Related issue number

#723

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • change description file added to changes/,
    see changes/README.md for details
    on the format
@codecov

This comment has been minimized.

Copy link

commented Aug 7, 2019

Codecov Report

Merging #724 into v0.32 will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff          @@
##           v0.32   #724   +/-   ##
====================================
  Coverage    100%   100%           
====================================
  Files         15     15           
  Lines       2727   2729    +2     
  Branches     539    541    +2     
====================================
+ Hits        2727   2729    +2

@YaraslauZhylko YaraslauZhylko changed the base branch from master to v0.32 Aug 7, 2019

@YaraslauZhylko

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

change description file added to changes/

As I'm basing off of v0.32 I decided to update the HISTORY.rst instead.
Is it OK, @samuelcolvin?

@YaraslauZhylko YaraslauZhylko changed the base branch from v0.32 to master Aug 7, 2019

@samuelcolvin
Copy link
Owner

left a comment

Please base of the v0.32 branch.

self.__fields_set__.add(name)
known_field = self.fields.get(name, None)
if known_field:
value_, error_ = known_field.validate(value, self.dict(exclude={name}), loc=name)

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner
Suggested change
value_, error_ = known_field.validate(value, self.dict(exclude={name}), loc=name)
value, error_ = known_field.validate(value, self.dict(exclude={name}), loc=name)

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner

we should have a test that would catch this, please add.

This comment has been minimized.

Copy link
@YaraslauZhylko

YaraslauZhylko Aug 8, 2019

Author Contributor

Good catch with value instead of value_!
Mea culpa!

This comment has been minimized.

Copy link
@YaraslauZhylko

YaraslauZhylko Aug 8, 2019

Author Contributor

Fixed.

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner

please can you add a test for this so similar problem doesn't happen again.

This comment has been minimized.

Copy link
@YaraslauZhylko

YaraslauZhylko Aug 8, 2019

Author Contributor

we should have a test that would catch this, please add.

@samuelcolvin What exactly do you mean by this?

We already have test_validating_assignment_ok() and test_validating_assignment_fail() in test_validators.py, and test_validate_assignment() and test_validate_assignment_error() in test_dataclasses.py to check known_field behaviour.

I only added tests for unknown extra values.

What scenario do we need to test additionally?

This comment has been minimized.

Copy link
@YaraslauZhylko

YaraslauZhylko Aug 8, 2019

Author Contributor

Oh, I see.
value vs. value_.
So we need some validator that changes the value. Like trimming the input, right?

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner

yes, I guess something like

def test_change_assignment():
    class Model(BaseModel):
        a: int

        @validator('a')
        def double_a(cls, v):
            return v * 2

        class Config:
            validate_assignment = True

    m = Model(a=1)
    assert m.a == 2

    m.a = 4
    assert m.a == 8

I know it's not technically part of the original change here, but I think if you catch a problem like this by chance it's imperative that you add a test that would automatically catch it next time.

This comment has been minimized.

Copy link
@YaraslauZhylko

YaraslauZhylko Aug 8, 2019

Author Contributor

Done.

Show resolved Hide resolved HISTORY.rst Outdated

@YaraslauZhylko YaraslauZhylko changed the base branch from master to v0.32 Aug 8, 2019

@YaraslauZhylko

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Please base of the v0.32 branch.

@samuelcolvin It is already based off of v0.32 branch. But I modified the PR to target v0.32 instead of master.

Is that enough?

YaraslauZhylko added some commits Aug 8, 2019

@samuelcolvin samuelcolvin merged commit 0252f13 into samuelcolvin:v0.32 Aug 8, 2019

7 of 10 checks passed

Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to 10c18ee
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190808.13 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Aug 8, 2019

great, thanks.

Since this isn't a new bug I assume it isn't critically urgent. I'll wait a few days in case anyone else finds problems with v0.32, then release.

@YaraslauZhylko

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Since this isn't a new bug I assume it isn't critically urgent.

We a actually a little bit blocked by it. 😅
But I guess it can definitely wait till Monday.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Aug 8, 2019

Okay, I'll release now.

@YaraslauZhylko

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

There's a minor typo there. 😅
In the History and commit/PR name:

do not validate extra fields when 'vaidate_assignment' is on, #724 by @YaraslauZhylko

It should be validate_assignment... l is missing.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Aug 8, 2019

will fix.

@YaraslauZhylko

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Cool!

@YaraslauZhylko YaraslauZhylko changed the title fix(validate-assignment): do not validate extra fields when `vaidate_assignment` is on fix(validate-assignment): do not validate extra fields when `validate_assignment` is on Aug 8, 2019

@YaraslauZhylko

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

#723?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.