ColumnDef builder functions in sea-orm-migration may push additional statements instead of changing them

[Rudi3]

Description

ColumnDef builder functions in sea-orm-migration may push SQL additional statements instead of changing them.

It looks like string(), for example, will add NOT NULL by default. If you add not_null(), you'll get the statement twice.
More problematic is null(), as you will get NOT NULL NULL.
This behaviour may cause other "multi-statements" that can contain contradictions, too.

Steps to Reproduce

manager
    .create_table(
        Table::create()
            .table(Post::Table)
            .if_not_exists()
            .col(pk_auto(Post::Id))
            // will create SQL containing NOT NULL NOT NULL
            .col(string(Post::Title).not_null())
            // will create SQL containing NOT NULL NULL
            // which will cause an error
            .col(string(Post::Text).null())
    )
    .await

Expected Behavior

manager
    .create_table(
        Table::create()
            .table(Post::Table)
            .if_not_exists()
            .col(pk_auto(Post::Id))
            // will create SQL containing just NOT NULL
            .col(string(Post::Title).not_null())
            // will create SQL containing just NULL
            // which will not cause an error
            .col(string(Post::Text).null())
    )
    .await

Reproduces How Often

Is it always reproducible? yes

Workarounds

Creating the ColumnDef from scratch seems to work fine:

manager
    .create_table(
        Table::create()
            .table(Post::Table)
            .if_not_exists()
            .col(pk_auto(Post::Id))
            // will create SQL containing NOT NULL NOT NULL
            .col(string(Post::Title).not_null())
            // will create SQL containing just NULL
            .col(
                ColumnDef::new_with_type(
                    Post::Text,
                    ColumnType::String(StringLen::default())
                )
                .null()
            )
    )
    .await

Versions

• sea-orm-migration v1.1.0-rc1
• PostgreSQL 16.3 (linux)

[Borderliner]

@Rudi3 @billy1624 Possibly related to Discussion #2324

[Rudi3]

@Borderliner yes, looks like it's exactly that. But it may not be that straightforward to fix.

As I understand it, if you wanted to validate for conflicting statements, you'd have to check the previous ones upon pushing. Not sure if that's a feasible approach.

[devklick]

Am I right in thinking sea-orm defaults all columns to be non-nullable?

Might it make more sense to default to whatever the target DB defaults to, and not add a NULL / NOT NULL constraint unless the user explicitely adds them with the null() / not_null() functions? This would be a breaking change though, unless there was an option to opt-in to this behavior.

I suppose this wouldn't fully get around the issue that @Rudi3 mentions though. You'd still need to handle the case where the user calls null().not_null().

Edit: Also, thanks for the workaround!

[Rudi3]

@devklick I'm assuming that sea-orm defaults all columns to be non-nullable atm - I haven't looked at them all, however: https://docs.rs/sea-orm-migration/1.0.1/src/sea_orm_migration/schema.rs.html#112-114

To a certain extent, it may make sense to make e.g. a string non-nullable by default - as it'll give you a string in your model. Otherwise, you'd get Option<String>.

I might have found an easier workaround: sea_orm_migration::schema::string_null
https://docs.rs/sea-orm-migration/1.0.1/src/sea_orm_migration/schema.rs.html#116-118
I haven't tested it myself, but at first glance, it seems like ColumnDef::new(col).string() (used in string_null) won't add another not_null().

[devklick]

@Rudi3 Oh, I see, there's a string() and string_null() function. I like this.

With that being the case, it deffo makes sense for string() to be non-nullable.

[Rudi3]

@devklick yes, looks like it works just fine as long as you use it right/don't touch it afterwards.

The only problem is that it'll silently continue if a user does it wrong.

[Borderliner]

I think this should be highlighted in docs