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

Merged
merged 4 commits into from Aug 8, 2019
Merged

fix(validate-assignment): do not validate extra fields when validate_assignment is on #724

merged 4 commits into from Aug 8, 2019

Conversation

YaraslauZhylko
Copy link
Contributor

@YaraslauZhylko YaraslauZhylko 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
Copy link

codecov bot 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
Copy link
Contributor Author

YaraslauZhylko 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
Copy link
Collaborator

@samuelcolvin samuelcolvin left a comment

Please base of the v0.32 branch.

pydantic/main.py Outdated
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)
Copy link
Collaborator

@samuelcolvin samuelcolvin Aug 8, 2019

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@samuelcolvin samuelcolvin Aug 8, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@YaraslauZhylko YaraslauZhylko Aug 8, 2019

Choose a reason for hiding this comment

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

Good catch with value instead of value_!
Mea culpa!

Copy link
Contributor Author

@YaraslauZhylko YaraslauZhylko Aug 8, 2019

Choose a reason for hiding this comment

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

Fixed.

Copy link
Collaborator

@samuelcolvin samuelcolvin Aug 8, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@YaraslauZhylko YaraslauZhylko Aug 8, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@YaraslauZhylko YaraslauZhylko Aug 8, 2019

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@samuelcolvin samuelcolvin Aug 8, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@YaraslauZhylko YaraslauZhylko Aug 8, 2019

Choose a reason for hiding this comment

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

Done.

HISTORY.rst Outdated Show resolved Hide resolved
@YaraslauZhylko YaraslauZhylko changed the base branch from master to v0.32 Aug 8, 2019
@YaraslauZhylko
Copy link
Contributor Author

YaraslauZhylko 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?

@samuelcolvin samuelcolvin merged commit 0252f13 into pydantic:v0.32 Aug 8, 2019
10 checks passed
@samuelcolvin
Copy link
Collaborator

samuelcolvin 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
Copy link
Contributor Author

YaraslauZhylko 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
Copy link
Collaborator

samuelcolvin commented Aug 8, 2019

Okay, I'll release now.

@YaraslauZhylko
Copy link
Contributor Author

YaraslauZhylko 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
Copy link
Collaborator

samuelcolvin commented Aug 8, 2019

will fix.

@YaraslauZhylko
Copy link
Contributor Author

YaraslauZhylko 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
Copy link
Contributor Author

YaraslauZhylko commented Aug 8, 2019

#723?

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