fix! EXPOSED-458 Stop sending default and null values in insert state… #2295
Conversation
| @@ -109,24 +108,16 @@ class UpsertTests : DatabaseTestsBase() { | |||
|
|
|||
| withTables(excludeSettings = TestDB.ALL_H2_V1, tester) { testDb -> | |||
| val primaryKeyValues = Pair("User A", "Key A") | |||
| if (testDb == TestDB.ORACLE) { | |||
There was a problem hiding this comment.
Oracle does not throw an error here, because for the case of autogenerated onUpdate part the key columns are filtered (filtered for Oracle only)
| // `amount` must be passed explicitly now due to usage of that column inside the custom onUpdate statement | ||
| // There is an option to call `tester.amount.defaultValueFun?.let { it() }!!`, | ||
| // it looks ugly but prevents regression on changes in default value | ||
| it[amount] = 25 |
There was a problem hiding this comment.
It was one of my biggest concern on upsert statement changes. The main problem is that that column amount is used inside onUpdate section (and here this section is not autogenerated). Before the changes in the PR it was working, because amount with default value was propagated automatically.
There was a problem hiding this comment.
I don't think this is a large problem, as long as this change is well and clearly documented.
The amount being intentionally excluded here is only contingent on the fact that it is actually being set in the insert statement. I don't believe that a user will intentionally use insertValue() in the onUpdate arg unless they either explicitly want to reference an insert value that they are manually setting anyway or they are aware that Exposed actually includes the column in the statement and are taking advantage of this.
Either way, choosing to use insertValue() means accepting the syntax contract, which states that the referenced column must be included in the insert clause.
So they may be surprised, but I doubt it would be considered unfair or illogical. And if they still want the original behavior, I believe it would still be achievable by defining:
integer("amount").default(25).clientDefault { 25 }.
There was a problem hiding this comment.
Thank you, I will add a hint about that in the documentation
About the variant integer("amount").default(25).clientDefault { 25 }, I'd prefer to avoid such a difinition of course, and (not sure, but it can be that) clientDefault() may overwrite default() because they modify the same fields of column
| @@ -366,7 +366,7 @@ class CreateMissingTablesAndColumnsTests : DatabaseTestsBase() { | |||
| actual.forEach { exec(it) } | |||
| } else { | |||
| SchemaUtils.drop(whiteSpaceTable) | |||
| SchemaUtils.create(whiteSpaceTable) | |||
| SchemaUtils.create(emptyTable) | |||
There was a problem hiding this comment.
It was the error in the test, but it didn't occured before because database side defaults were not used.
| val acceptableDefaults = mutableListOf("CURRENT_TIMESTAMP", "CURRENT_TIMESTAMP()", "NOW()", "CURRENT_TIMESTAMP(6)", "NOW(6)") | ||
| return e.toString().trim() in acceptableDefaults && isFractionDateTimeSupported() | ||
|
|
||
| // This check is quite optimistic, it will not allow to create a varchar columns with "CURRENT_DATE" default value for example | ||
| // Comparing to the previous variant with white list of functions the new variant does not reject valid values, | ||
| // it could be checked on the test UpsertTests::testUpsertWithColumnExpressions() | ||
| return e.toString().trim() !in notAcceptableDefaults |
There was a problem hiding this comment.
Just to make sure I'm understanding correctly:
- Whenever any invalid column default value is used (for example, CURRENT_DATE with MySQL v5 & no client default), then the behavior is that Exposed just logs an error and defines the column with SQL NULL in the CREATE ddl instead. So tests (like the datetime ones below) used to pass, before all these changes, because the insert statements included the current date. And now, the same ddl with NULL marker will be defined, but the insert statement will not include these columns, so a date would never be inserted unless done manually.
This will be a fairly specific breaking change and I'd recommend mend highlighting it whenever the breaking changes docs is updated.
I would also suggest updating the log message that is output in Column.descriptionDdl() to reflect the new behavior more explicitly.
There was a problem hiding this comment.
Actually, I'm looking at the math functions test below.
I see that the new behavior (for MySQL v5) will be that any invalid default values (other than current_date) will be included in the ddl, leading to a syntax exception, instead of logging an error and marking as NULL.
It might be an edge case, given the older version, but is the plan to include any new api that could avoid this?
Something like a clientDefaultExpression(), so that the DDL is not affected, but Exposed could automatically set the specified database function on every insert?
There was a problem hiding this comment.
The main idea of the change that before that change any value that is not in the list (event if it's valid default value) was recognized as invalid default value, and skipped in DDL.
Actually, I can say I see straightforward solution here. At the current moment we have some features to help user if he tries to put non valid default value. That includes:
- prevent creating DDL with invalid value
- convert column into nullable one if there is no client default value
- sending default value in every insert query
My PR removes the third case.
One option is to try to continue what we did before, and for example add not only conversion to nullable column, but also conversion default column into clientDefaultExpression column, or we can throw errors and ask users not to put invalid values into default columns (kind of removing the magic we do for users now), or probably there are other variants I don't see now.
@joc-a do you remember why we added such a logic? Probably you also have some thoughts what better to do if we loose sending all the defaults in inserts.
There was a problem hiding this comment.
According to clientDefaultExpression() as public API, we had already some discussions on it, but those time it was added more for feature completeness (if we have default, clientDefault, and defaultExpression, why we should not add clientDefaultExpresson), but decided that's probably not the thing that is needed by many people.
But with new reality it could be a good idea to add it. I could add it with refactoring of current 3 fields for default values.
| // `amount` must be passed explicitly now due to usage of that column inside the custom onUpdate statement | ||
| // There is an option to call `tester.amount.defaultValueFun?.let { it() }!!`, | ||
| // it looks ugly but prevents regression on changes in default value | ||
| it[amount] = 25 |
There was a problem hiding this comment.
I don't think this is a large problem, as long as this change is well and clearly documented.
The amount being intentionally excluded here is only contingent on the fact that it is actually being set in the insert statement. I don't believe that a user will intentionally use insertValue() in the onUpdate arg unless they either explicitly want to reference an insert value that they are manually setting anyway or they are aware that Exposed actually includes the column in the statement and are taking advantage of this.
Either way, choosing to use insertValue() means accepting the syntax contract, which states that the referenced column must be included in the insert clause.
So they may be surprised, but I doubt it would be considered unfair or illogical. And if they still want the original behavior, I believe it would still be achievable by defining:
integer("amount").default(25).clientDefault { 25 }.
documentation-website/Writerside/topics/DSL-CRUD-operations.topic
Outdated
Show resolved
Hide resolved
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/UpsertTests.kt
Outdated
Show resolved
Hide resolved
exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/InsertTests.kt
Outdated
Show resolved
Hide resolved
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/InsertStatement.kt
Show resolved
Hide resolved
…lumnValuePreferredFromResultSet with deprecation
obabichevjb
force-pushed
the
obabichev/exposed-458-no-default-values-in-insert-statemnt
branch
from
f3bdac8 to
efe92ad
Compare
obabichevjb
deleted the
obabichev/exposed-458-no-default-values-in-insert-statemnt
branch
With this PR insert, upsert and replace SQL commands will not propogate default values that could be created on the database by default.
The main change is in the logic of how
argumentsof statements are calculated. Instead of taking all the defaults only client defaults are taken now. The methodvaluesAndDefaults()that was used for creating arguments was deprecated, because it's open and could be used by other people. Also were added some bug fixes for default values.Type of Change
Updates/remove existing public API methods:
Affected databases:
Related Issues
EXPOSED-458 Stop sending default and null values in insert statements