1132 If a column is nullable, make it optional by default in create_pydantic_model

[dantownsend]

Resolves #1132

[Skelmis]

Assuming required becoming None doesn't break the admin panels behavior I like the look of this

[sinisaos]

I agree with these changes and everything works, but we have one case where someone makes a mistake and marks a null field as optional. We have four options.

class Band(Table):
    name = Varchar() # null=False by default

>>> create_pydantic_model(Band).model_fields['name'].annotation
str # correct

class Band(Table):
    name = Varchar(null=True)

>>> create_pydantic_model(Band).model_fields['name'].annotation
Optional[str] # correct

class Band(Table):
    name = Varchar(required=True, null=True)

>>> create_pydantic_model(Band).model_fields['name'].annotation
str # correct

class Band(Table):
    name = Varchar(required=False) # null=False by default

>>> create_pydantic_model(Band).model_fields['name'].annotation
Optional[str] # not correct, should be str (required because is not null constraint in db)

Should we prevent these possible user errors or not? If we change this line of code, we can force required to be True if null is False, to prevent user errors. After that change result is

class Band(Table):
    name = Varchar(required=False) # null=False by default

>>> create_pydantic_model(Band).model_fields['name'].annotation
str # correct

which is correct.

[dantownsend]

@Skelmis and @sinisaos - thanks for your feedback on this.

I've been thinking about it today, and I'm a bit undecided still.

Current behaviour

The current behaviour in Piccolo is required=False by default, which means that if we have this:

class Band(Table):
    name = Varchar()

The it generates the following Pydantic model:

class BandModel(BaseModel):
    name: Optional[str]

In terms of generating the Pydantic model, null is completely ignored, it's completely driven by required.

This is similar to Django where they have nullable and blank arguments. Only blank determines if the field is required in Django Admin.

Maybe we should have made required=True the default - it's hard to say.

Multiple Pydantic models

The downside with putting required on the Piccolo Table is that you will often have multiple Pydantic models per table. For example:

1. A model for filtering or PATCH (where every field is optional)
2. A model for returning data in a GET request, or receiving a POST (where some fields will be required)

We get around 1 by having the all_optional argument in create_pydantic_model.

create_pydantic_model(
    Band,
    all_optional=True
)

If someone required even more Pydantic models from the same Piccolo table it gets tricky. Perhaps there could be a required_columns arg in create_pydantic_model:

create_pydantic_model(
    Band,
    required_columns=Band.all_columns(exclude=[Band.some_column])
)

That gives the ultimate flexibility.

Conclusions (?)

I'm still a bit on the fence ... I'm interested to hear your thoughts.

I think I'll refactor piccolo-orm/piccolo_admin#432 slightly for now, so it's decoupled from this PR, as I don't want to rush this in and make a mistake.

[dantownsend]

>>> create_pydantic_model(Band).model_fields['name'].annotation
Optional[str] # not correct, should be str (required because is not null constraint in db)

@sinisaos I think for that situation we just assume that the user will add the missing value before saving it to the database.