🐛 Updated base model to store null instead of empty string#10425
Conversation
| } | ||
| // Gets nullable strings | ||
| getNullableStringProperties: _.memoize(function getNullableStringProperties(options) { | ||
| return options.query.columnInfo().then((columns) => { |
There was a problem hiding this comment.
Using options.query here means that if the query is part of a transaction, this will be part of it too.
There was a problem hiding this comment.
Why do we have to query the database to get the "nullable" information? 🤔
Can't we simply ask the schema?
There was a problem hiding this comment.
We could do - but the database will hold the actual source of truth for whether it is nullable or not - losing sync might not be an issue we want to cater for though.
Do you think the model should have direct access to the db schema?
EDIT: just seen it already does - deffo quicker if we already have precedent
There was a problem hiding this comment.
Yeah IMO model layer is allowed to access the schema.
| return this.tableName; | ||
| }), | ||
|
|
||
| // Sets given values to `null` |
There was a problem hiding this comment.
Useless and misleading comment
|
@kirrg001 Would you mind having a quick look at this, specifically the changes to base model |
|
@kirrg001 This has been updated - looks much better |
|
Does this generic logic work with the settings model? If yes, this LGTM 👍 |
|
Seems to work fine with title and description etc.. columns |
2 participants
refs #10388
This updates the base model to retrieve column information, and explicitly set every property whose column is
nullableand content is the empty string ("") tonullThis is the first half of solving #10388 - This will stop all future updates from storing empty strings, but we will still need to clean up the DB.