fix: make preimage a nullable string #139
Conversation
| @@ -85,6 +85,13 @@ func main() { | |||
| log.Fatalf("Failed migrate DB %v", err) | |||
| } | |||
|
|
|||
| // Update payments with preimage as an empty string to use NULL instead | |||
| // TODO: remove this migration in a future release and make preimage column unique | |||
| if err := db.Table("payments").Where("preimage = ?", "").Update("preimage", nil).Error; err != nil { | |||
There was a problem hiding this comment.
don't do migrations like this.
either we have a proper migration setup or you do DB maintenance on the DB.
There was a problem hiding this comment.
And if we don't have proper migrations. Then we must release this as a breaking change with details in the release notes, otherwise we break the deployment of others.
There was a problem hiding this comment.
I vote for a proper migration setup - I don't think we can release this as a breaking change. NWC on all umbrel apps will break and it's unreasonable to ask users to manually do something to fix it.
There was a problem hiding this comment.
There was a problem hiding this comment.
gorm on its own does not seem to support doing a manual migration here (note: the string column from non-nullable to nullable IS migrated automatically, but not the query to update the preimage from '' to NULL needs to be done manually)
It looks like https://github.com/go-gormigrate/gormigrate could be the best option here, it runs inline (as opposed to atlas and golang-migrate)
There was a problem hiding this comment.
I think auto-migrate is mostly a dangerous option as you always have to be careful with the data that you have already in the DB.
and I think it's better to write explicit migrations then it is also possible to support different special SQL dialects (PG, SQLite)
|
Replaced by #148 |
Related to #138
I think the preimage should not be able to be stored as an empty string (then we cannot make add a unique constraint)
The query in GetBudgetUsage is working incorrectly because it's checking
preimage IS NOT NULLwhich always passes.A follow up PR needs to be added to add the unique constraint, once this has been run.