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

v1.2 change in optional behaviour #1047

Closed
samuelcolvin opened this issue Nov 29, 2019 · 6 comments · Fixed by #1048
Labels
bug

Comments

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Nov 29, 2019

from @bartv on #990

It seems that this change caused a regression in the latest release. The behavior between static and dynamic classes is different now.

from typing import Optional

from pydantic import create_model, BaseModel


def test_optional_value():
    class Test(BaseModel):
        field1: str
        field2: Optional[str]

    Test(field1="a", field2="b")
    Test(field1="a")


def test_dynamic_optional_value():
    fields = {'field1': (str, ...), 'field2': (Optional[str], ...)}
    Test = create_model("Test", __base__=BaseModel, **fields)

    Test(field1="a", field2="b")
    Test(field1="a")

With pydantic 1.1.1 both test cases succeed, with pydantic 1.2 the dynamic test case fails.

Is this expected? Based on the changelog and the documentation I am inclined to think it is not. Should I open a separate bug?

Originally posted by @bartv in #990 (comment)

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Nov 29, 2019

Thanks for reporting and sorry about creating the regression.

We should have noticed this.

I suspect that releasing #1031 as part of a patch release was a mistake. It should have been part of v2.

There are two problems here:

change in optional behaviour

The meaning of foobar: Optional[int] = ... has changed in v1.2 which is not backwards compatible. (See here for an explanation. We knew this but thought it was a price worth paying. I think that was a mistake I will document this better.

create_model

This has affected create_model() where you can't create an annotation only field, and instead have to use an ellipsis.

I don't see a sensible work around here without more breaking changes. I think the best solution is to use

fields = {'field1': (str, ...), 'field2': (Optional[str], None)}

Which is presumably the behaviour you want?

@samuelcolvin samuelcolvin changed the title v1.2 problem with create_model v1.2 change in optional behaviour Nov 29, 2019
@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Nov 29, 2019

In summary I think the best we can do here is better documentation.

If anyone disagrees and has any clever suggestions of workarounds, please let us know.

samuelcolvin added a commit that referenced this issue Nov 29, 2019
@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Nov 29, 2019

Better documentation of this in #1048, comment there if it's not okay, otherwise I'll release that update to the docs soon.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Nov 29, 2019

We should have noticed this.

Agreed 😕.


I think it would be pretty easy to add support for single-element tuples provided to create_model as a field definition, and to use the same logic for those as though you hadn't specified a default value for the field (that way, for example fields = {'field1': (str,), 'field2': (Optional[str],)} would work the way one would expect). I don't think this would be a breaking change since right now you can't specify non-pairs there anyway (but let me know if I'm missing something). Also, given create_model typically shouldn't be getting called in hot loops or with any substantial frequency, I don't think the performance implications related to adding a tuple-length check pose any issue.

It wouldn't mitigate the need to change code to make create_model work as it did before for optional fields (since ... always means required now), but I think ultimately it would provide more consistency.

Thoughts?

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

@samuelcolvin samuelcolvin commented Nov 29, 2019

I agree it would be more consistent and a good idea. It won't cause a regression, but it also won't solve this one.

Ultimately we will change the behavior of Optional in v2 so no default makes a value required even if it's nullable. Thus annotation only and ellipsis fields will mean the same thing again.

@bartv

This comment has been minimized.

Copy link

@bartv bartv commented Nov 30, 2019

Agreed.

samuelcolvin added a commit that referenced this issue Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.