v1.2 change in optional behaviour

[samuelcolvin]

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]

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]

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]

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

[dmontagu]

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]

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]

Agreed.