null vs empty string

[trondhindenes]

given a model:

class Customer(Table):
    email: str = Varchar()

this migration is auto-generated:

    manager.add_column(
        table_class_name="Customer",
        tablename="customer",
        column_name="email",
        db_column_name="email",
        column_class_name="Varchar",
        column_class=Varchar,
        params={
            "length": 255,
            "default": "",
            "null": False,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

One would think that this would disallow creating a record with an empty email field, but it is allowed:

    customer = Customer()
    customer.save().run_sync()

the sql statement generated from the migration is:

execute __asyncpg_stmt_6__: CREATE TABLE customer ("id" SERIAL PRIMARY KEY NOT NULL, "email" VARCHAR(255) NOT NULL DEFAULT '')

the returned object when queriyng is:

{'id': 1, 'email': ''}

As far as I can see, the bug here is that piccolo sets an empty string as the default for the column, which is the default for posgres. IMHO it would be "more correct" if piccolo disallowed creating objects where non-nullable columns are not specified (it would also be closer to how pydantic works)

Another (and maybe bigger) issue is that setting a column to nullable has some consequences:
the generated sql:

2021-11-15 21:09:24.996 UTC [89] LOG:  execute __asyncpg_stmt_6__: CREATE TABLE customer ("id" SERIAL PRIMARY KEY NOT NULL, "email" VARCHAR(255) DEFAULT '')

...and the code:

    customer = Customer()
    customer.save().run_sync()

    this_cust = Customer.objects().first().run_sync()
    assert this_cust.email is None

this fails, since the email field is empty string instead of None.

even tho an empty string is "non-truthy" in python, treating them as true None would imho be much cleaner.

suggestion

In order not to break backwards compatability maybe there could be a flag in the engine config for configuring "empty string is None", and if true, replace empty strings for nullable fields with a proper None. Another flag could be "fail fast non-nullable-fields" where the engine intercepts/raises a validation error if attempting to write an object with an unset non-nullable field to the database.

So, summing up:

• piccolo allows "non-nullable" collumns to be empty strings (which is the same as is used for "None" values, see item 3)
• piccolo allows "non-nullable" colums to be left unset on object creation (I would expect it to throw prior to writing to the db)
• piccolo returns empty strings instead of None for nullable fields

All of these have confused me somewhat during development with piccolo the last week. I may have overlooked something in the docs describing piccolo's "null" handling but I couldn't find anything.

[trondhindenes]

adding to this: regarding my third "problem", I guess explicitly setting the default value will solve it:

Varchar(null=True, default=None)

to me this feels slightly counter-intuitive but it's a completely valid workaround.

[dantownsend]

@trondhindenes I think what is causing confusion is the default for a Varchar is an empty string.

piccolo/piccolo/columns/column_types.py

Lines 164 to 169 in c569d14

	def __init__(
	self,
	length: int = 255,
	default: t.Union[str, Enum, t.Callable[[], str], None] = "",
	**kwargs,
	) -> None:

Even if it's set as null=True it will still save OK, because it defaults to an empty string.

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

band = Band()
await band.save()

>>> band.name
''

I agree that this can cause confusion, as dealing with truthy/falsy values can lead to bugs. i.e. band.name is None has to be used instead of not band.name.

I know that in Django it's generally recommended to use null=False for Varchar and Text.

https://simpleisbetterthancomplex.com/tips/2016/07/25/django-tip-8-blank-or-null.html

I think the main issue is allowing the programmer to specify that a column must be provided with a value.

We have an attribute called required which can be passed to a column. The reason this was originally created was for Piccolo Admin, so we force the user to provide a value, and not just use the default.

piccolo/piccolo/columns/base.py

Lines 309 to 312 in c569d14

	:param required:
	This isn't used by the database - it's to indicate to other tools that
	the user must provide this value. Example uses are in serialisers for
	API endpoints, and form fields.

We could potentially add a validation check when someone calls the save method. It checks that all columns with required specified are checked to see if the user provided a custom value, otherwise it raises a ValueError.

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

band = Band()
await band.save()
>>> ValueError("`name` is a required field")

[dantownsend]

adding to this: regarding my third "problem", I guess explicitly setting the default value will solve it:

Varchar(null=True, default=None)

to me this feels slightly counter-intuitive but it's a completely valid workaround.

Yeah, you're right.

[trondhindenes]

i.e. band.name is None has to be used instead of not band.name.

This is exactly what bit me the first time :-)
It could be as simple as just documenting really well "how to handle nulls"

As for the "force non-null" I think required would help a lot - I'd love to see that enabled by default when null=False and default isn't specified, but I guess that risks breaking backwards compatability.