fix(controller): ensure upsert works on both Postgres and SQLite #1137
Conversation
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
onematchfox
requested review from
EItanya,
ilackarms and
yuval-k
as code owners
There was a problem hiding this comment.
Pull request overview
This PR updates the database upsert logic to properly work with both PostgreSQL and SQLite by replacing the previous "try-create-then-save-on-error" approach with GORM's native OnConflict clause. This change enables the controller to scale across multiple replicas with PostgreSQL while maintaining backward compatibility with SQLite for single-replica deployments.
Key changes:
- Replaces the dual-operation upsert (Create + Save on error) with a single atomic upsert using
clause.OnConflict{UpdateAll: true} - Removes the TODO comment indicating the previous implementation wasn't truly idempotent
- Updates error message to reflect the actual operation being performed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func save[T Model](db *gorm.DB, model *T) error { | ||
| if err := db.Create(model).Error; err != nil { | ||
| if err == gorm.ErrDuplicatedKey { | ||
| return db.Save(model).Error | ||
| } | ||
| return fmt.Errorf("failed to create model: %w", err) | ||
| if err := db.Clauses(clause.OnConflict{ | ||
| UpdateAll: true, | ||
| }).Create(model).Error; err != nil { | ||
| return fmt.Errorf("failed to upsert model: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The updated save function lacks test coverage to verify the upsert behavior works correctly with both PostgreSQL and SQLite. Given that this change is specifically to ensure compatibility across database backends, consider adding tests that:
- Verify the upsert correctly inserts new records
- Verify the upsert updates existing records (especially with composite primary keys like
Event,Session,Tool, etc.) - Test with both SQLite and PostgreSQL to ensure the behavior is consistent
- Verify that
CreatedAtandUpdatedAttimestamps are handled correctly on conflict
…ent-dev#1137) Split this out of kagent-dev#1133 to try reduce the size of that PR - but also because it's not strictly related to being able to scale the controller - it simply manifested when needing to switch to postgres when running multiple controller replicas. Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com> Signed-off-by: Ivan Porta <porta.ivan@outlook.com>
2 participants
Split this out of #1133 to try reduce the size of that PR - but also because it's not strictly related to being able to scale the controller - it simply manifested when needing to switch to postgres when running multiple controller replicas.