chore: make pair (timestamp, appName) unique #1513
Conversation
size-limit report 📦
|
Codecov ReportBase: 66.62% // Head: 66.62% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1513 +/- ##
=======================================
Coverage 66.62% 66.62%
=======================================
Files 146 146
Lines 4966 4966
Branches 1350 1350
=======================================
Hits 3308 3308
Misses 1653 1653
Partials 5 5 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| if err := tx.Clauses(clause.OnConflict{ | ||
| Columns: []clause.Column{ | ||
| {Name: "app_name"}, | ||
| {Name: "timestamp"}, | ||
| }, | ||
| // Update fields we care about | ||
| DoUpdates: clause.AssignmentColumns([]string{"app_name", "timestamp", "content"}), | ||
| // Update updateAt fields | ||
| UpdateAll: true, | ||
| }).Create(&u).Error; err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Not sure this is an improvement.
There are a few things happening here:
- We need to tell which fields we are interested in conflicting (
app_name,timestamp), otherwise it defaults to the primary key. I decided to not use a natural primary key here, since we may want to for eg modify an existing annotation (changeappNamefor example) DoUpdatessince we need to tell which columns we want to update. Otherwise it doesn't do anything.UpdateAllto tell it to updateupdateAtfield. Here's where it gets confusing: I thoughtUpdateAllwould update everything (duh), but it seems to ignore the other fields.
There was a problem hiding this comment.
It's interesting – I was sure that UpdateAll does update all columns but primary keys.
Maybe we should update content and updated_at explicitly – just for clarity:
{
Columns: ...,
DoUpdates: clause.AssignmentColumns([]string{"content", "updated_at"}),
}There was a problem hiding this comment.
Did not work, I think it's due to how updated_at is generated dynamically by gorm.
| func addIndexesUniqueTableMigration() *gormigrate.Migration { | ||
| type annotation struct { | ||
| AppName string `gorm:"index:idx_appname_timestamp,unique;not null;default:null"` | ||
| Timestamp time.Time `gorm:"index:idx_appname_timestamp,unique;not null;default:null"` |
There was a problem hiding this comment.
Previous declaration has a typo: form -> gorm
Timestamp time.Time `form:"not null;default:null"`There was a problem hiding this comment.
I noticed (and fixed it), but not in migration thought right?
|
Regarding SQL logging – I'd say that we should not enable it by default |
|
Honestly, I don't quite understand the restriction: why can't we have multiple annotations for the same application and the same moment of time? |
Yeah, I didn't even commit it. I was just documenting for the next person.
From a technical perspective? None. From a practical perspective? Couple reasons (at least):
|
3 participants
Closes #1509
Plus refactors the
upsertas per @kolesnikovae's here #1508 (comment)Here's the sql schema (generated with
.schema annotations):offtopic/self documenting: had to turn on sql log to make sense out of what the orm is doing