fix: EXPOSED-415 SchemaUtils incorrectly generates ALTER statements f…

[obabichevjb]

Here is the fix of generation update with SchemaUtils.statementsRequiredToActualizeScheme() for nullable columns.

The problem is that for MariaDB and MySql it's not possible to determine whether the default value of a nullable column was not defined or it was explicitly set to null. Also an issue that (if I'm right for MySql only) for some cases string default value "NULL" was recognized as null.

The main change that I made with this PR is treating any null-like value (non set null, or explicitly set null) as the same for DDL generation purposes. It means that if a user created a table with a nullable column and without a default value and then added explicit .default(null) Exposed will not offer to alter that column. Such a variant is not 100% accurate in terms of synchronization between actual DB and Exposed definitions but looks like follows to the same result.

It was suggested to check if we can take the information about the default values directly from DB (instead of JDBC), but looks like it's the same for MariaDB anyway.

[obabichevjb]

Probably, we could apply such a logic to MariaDB only, and still generate DDL with updating nullable columns for other DBs. But in this case I'd say the method JdbcDatabaseMetadataImpl::sanitizedDefault should return different values (if possible) for the cases when the value was not set at all, when it was set to null explicitly, and any other values.

It's definitely possible but would require to introduce at least one more constant (enum/object/..) like DB_EXPLICIT_NULL. But actually not sure that it's practically needed...

[bog-walk]

There may be a potential issue with the new MariaDB logic and b' defaults. Please see the comments.

[bog-walk]

I'm not sure if we have good enough test coverage to ok removing some of these branches. The h2Modes are probably not going to be an issue, but removing MariaDB from this substring check may cause problems in a few users.

One example I can think of:
In both MySQL and MariaDB, Exposed takes the opinion to store boolean values in BOOLEAN column type instead of BIT. But let's say a user has an existing table already in the DB that uses BIT (or chooses to create one themselves), and they are using their own custom bool() to register and work with this BIT column using Kotlin booleans.
Attempting something like the following would no longer show the same behavior in both DB:

// represents the existing table in db
exec("CREATE TABLE IF NOT EXISTS tester (bool BIT DEFAULT b'0' NOT NULL)")

// represents the identical Exposed definition
object TestTable : Table("tester") {
    val bool = bool("bool").default(false)
}

// when calling SchemaUtils.createMissingTables....
// MySQL - correctly does not generate ALTER statement
// MariaDB - incorrectly does generate ALTER statement

I'd suggest a third branch (if value starts with b' or something) in the new MariaDB logic that does what the original logic did (i.e. what the new MySQL branch does now). Not sure if the mysql H2 modes would also be affected.

[obabichevjb]

Added check of 'b for MariaDB also, but without proper test actually.

But what I've found during testing of this is that binary column is almost untested and quite broken.
I added test to check binary column ddl generation, and insert/reading values (with binary literal also, and the binary literal actually). So now it looks better, but PR become a bit bigger..

[bog-walk]

Looks good to me. I would still recommend moving the introduction of byteLiteral() and the column changes to its own separate PR (why no byteParam() btw?). It would be easier to look back on in case of anything and it could be merged first if the MariaDB tests depend on it.

[bog-walk]

All this and above functionality already exists within the blob (binary) column type. For example, ExposedBlob has the same function toHexString() that can be extracted and reused.

But if you don't mind the duplicate here, at least nonNullValueToString() should use currentDialect.dataTypeProvider.hexToDb(value.hexString()). From what I'm seeing, existing hexToDb() matches your branches above.

[obabichevjb]

TODO fix with EXPOSED-433 Check all the literals for default/insert/read operations

[bog-walk]

Please also move over existing tests from DDLTests.kt: testBinaryWithoutLength() and testBinary()

[obabichevjb]

TODO fix with EXPOSED-433 Check all the literals for default/insert/read operations

[bog-walk]

Literals are more meant to be used in query building, in WHERE clauses for example, or as column defaults, or where expressions are needed in functions. It might be good to at least test it it 1 of those actual use cases.

[obabichevjb]

TODO fix with EXPOSED-433 Check all the literals for default/insert/read operations