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 #4192 bug with BaseModel.construct and aliased Fields #4191

Merged
merged 10 commits into from Aug 11, 2022

Conversation

kylebamos
Copy link
Contributor

@kylebamos kylebamos commented Jun 28, 2022

Change Summary

Update BaseModel.construct to consider field aliases when constructing.

Related issue number

fix #4192

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)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@kylebamos kylebamos changed the title WIP: Fix for BaseModel.construct not working appropriately with field aliases fix #4192 Jun 28, 2022
[True, True, 'bar', does_not_raise()],
[True, True, 'bar_', does_not_raise()],
[True, False, 'bar', does_not_raise()],
[True, False, 'bar_', does_not_raise()],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If construct were to behave consistently with respect to allow_population_by_field_name_config, this would raise a ValueError, which is how I had the behavior in my initial commit, but that would be a breaking change, so I decided against it.

@kylebamos kylebamos changed the title fix #4192 fix https://github.com/samuelcolvin/pydantic/issues/4192 Jun 28, 2022
@kylebamos kylebamos changed the title fix https://github.com/samuelcolvin/pydantic/issues/4192 Fix #4192 bug with BaseModel.construct and aliased Fields Jun 28, 2022
@kylebamos
Copy link
Contributor Author

please review

@hramezani
Copy link
Member

Thanks @kylebamos for your patch 👍

Here is my understanding.
By having Foo1 as a model with allow_population_by_field_name=True and Foo2 as a model with allow_population_by_field_name=False:

from pydantic import BaseConfig, BaseModel, Field


class Foo1(BaseModel):
    class Config(BaseConfig):
        allow_population_by_field_name = True

    bar_: int = Field(..., alias="bar")


class Foo2(BaseModel):
    class Config(BaseConfig):
        allow_population_by_field_name = False

    bar_: int = Field(..., alias="bar")


expected_value = 2

Here is the current state:

  • Passing alias in standard instantiation:
# 1 - OK
assert Foo1(bar=expected_value).bar_ == expected_value

# 2 - OK
assert Foo2(bar=expected_value).bar_ == expected_value
  • Passing alias in construct:
# 3 - AttributeError: 'Foo' object has no attribute 'bar_' -- Has to be fixed
assert Foo1.construct(bar=expected_value).bar_ == expected_value

# 4 - AttributeError: 'Foo' object has no attribute 'bar_' -- Ok
assert Foo2.construct(bar=expected_value).bar_ == expected_value
  • Passing field name in standard instantiation:
# 5 - OK
assert Foo1(bar_=expected_value).bar_ == expected_value

# 6 - ValidationError: 1 validation error for Foo2 bar -- OK
assert Foo2(bar_=expected_value).bar_ == expected_value
  • Passing field name in construct:
# 7 - OK
assert Foo1.construct(bar_=expected_value).bar_ == expected_value
# 8 - OK
assert Foo2.construct(bar_=expected_value).bar_ == expected_value

And your patch will fix the Case 3 which is OK.

Regarding Case 8, I think it should raise an error like Case 6.
I think it's not related to your patch and it may break our existing implementation.
What do you think @samuelcolvin and @PrettyWood ?

pydantic/main.py Outdated Show resolved Hide resolved
tests/test_aliases.py Outdated Show resolved Hide resolved
tests/test_aliases.py Outdated Show resolved Hide resolved
tests/test_aliases.py Outdated Show resolved Hide resolved
@hramezani
Copy link
Member

Please update

@kylebamos
Copy link
Contributor Author

kylebamos commented Jul 7, 2022

Regarding case 8 you highlighted above, I addressed that in my comment as well because I initially had it fixed to accommodate that scenario, but it does break other tests.

https://github.com/samuelcolvin/pydantic/pull/4191/files#r909024428

It's as simple as changing line 592 in main.py to be elif cls.__config__.allow_population_by_field_name and name in values:

and adding else: errors.append(ErrorWrapper(MissingError(), loc=field.alias)) at the end of the if...elif chain in that same spot

@kylebamos
Copy link
Contributor Author

please review

@samuelcolvin
Copy link
Member

@kylebamos I have to say I find your avatar very confusing, I keep thinking there are lots of negative comments and reviews.

I'm marginally pro this change, my concern is whether it could be a breaking change to anyone. @PrettyWood do you have any thoughts?

Also conflicts needs solving, please update.

@kylebamos
Copy link
Contributor Author

Conflict resolved. Avatar fixed to your liking, I hope.

@kylebamos
Copy link
Contributor Author

Please review

@samuelcolvin
Copy link
Member

Avatar fixed to your liking, I hope.

Much better 👍 I'm sure I'm not the only one who will prefer it 😏.

@samuelcolvin samuelcolvin mentioned this pull request Aug 9, 2022
14 tasks
@PrettyWood
Copy link
Member

LGTM! Good for v1.10!

@samuelcolvin samuelcolvin merged commit a587ece into pydantic:master Aug 11, 2022
@samuelcolvin
Copy link
Member

@kylebamos thanks so much for this.

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.

BaseModel.construct does not set aliased fields consistently with standard instantiation
4 participants