fix(contrib/drivers/sqlite): infer save conflict from primary keys#4766
Open
gqcn wants to merge 1 commit into
Open
fix(contrib/drivers/sqlite): infer save conflict from primary keys#4766gqcn wants to merge 1 commit into
gqcn wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the SQLite and SQLiteCGO drivers’ Save (upsert) behavior to infer conflict targets from table primary keys when OnConflict(...) is omitted, aligning behavior with other upsert-capable drivers.
Changes:
- Add driver-specific
DoInsertoverrides for SQLite/SQLiteCGO to inferOnConflictfrom primary keys forInsertOptionSave. - Update unit tests for SQLite/SQLiteCGO
Saveto assert successful insert+update behavior and error on missing primary key data. - Add OpenSpec proposal/design/spec/task/changelog artifacts documenting the new behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| openspec/changes/sqlite-save-primary-conflict/tasks.md | Marks the feedback task as completed for the new behavior. |
| openspec/changes/sqlite-save-primary-conflict/specs/sqlite-upsert/spec.md | Specifies required upsert-conflict inference behavior and error cases. |
| openspec/changes/sqlite-save-primary-conflict/proposal.md | Summarizes motivation and desired alignment with other drivers. |
| openspec/changes/sqlite-save-primary-conflict/design.md | Describes the implementation approach at a high level. |
| openspec/changes/sqlite-save-primary-conflict/.openspec.yaml | Adds OpenSpec metadata for the change. |
| contrib/drivers/sqlitecgo/sqlitecgo_z_unit_core_test.go | Updates SQLiteCGO Save unit test expectations for inferred conflicts. |
| contrib/drivers/sqlitecgo/sqlitecgo_do_insert.go | Adds DoInsert override to infer conflict columns from PKs for Save. |
| contrib/drivers/sqlite/sqlite_z_unit_core_test.go | Updates SQLite Save unit test expectations for inferred conflicts. |
| contrib/drivers/sqlite/sqlite_do_insert.go | Adds DoInsert override to infer conflict columns from PKs for Save. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+48
to
+58
| func saveDataHasPrimaryKeys(list gdb.List, primaryKeys []string) bool { | ||
| if len(list) == 0 || len(primaryKeys) == 0 { | ||
| return false | ||
| } | ||
| for _, primaryKey := range primaryKeys { | ||
| if !saveDataHasKey(list[0], primaryKey) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
Comment on lines
+48
to
+58
| func saveDataHasPrimaryKeys(list gdb.List, primaryKeys []string) bool { | ||
| if len(list) == 0 || len(primaryKeys) == 0 { | ||
| return false | ||
| } | ||
| for _, primaryKey := range primaryKeys { | ||
| if !saveDataHasKey(list[0], primaryKey) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
Comment on lines
+47
to
+68
| // saveDataHasPrimaryKeys reports whether the first save record contains all primary keys. | ||
| func saveDataHasPrimaryKeys(list gdb.List, primaryKeys []string) bool { | ||
| if len(list) == 0 || len(primaryKeys) == 0 { | ||
| return false | ||
| } | ||
| for _, primaryKey := range primaryKeys { | ||
| if !saveDataHasKey(list[0], primaryKey) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // saveDataHasKey reports whether the save data contains the given key case-insensitively. | ||
| func saveDataHasKey(data gdb.Map, key string) bool { | ||
| for dataKey := range data { | ||
| if strings.EqualFold(dataKey, key) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
| return nil, gerror.NewCodef( | ||
| gcode.CodeMissingParameter, | ||
| `Save operation requires conflict detection: `+ | ||
| `either specify OnConflict() columns or ensure table '%s' has primary keys in the data`, |
Comment on lines
+446
to
+453
| t.Assert(rowsAffected, 1) | ||
|
|
||
| data["nickname"] = "T10-updated" | ||
| result, err = db.Save(ctx, "t_user", data, 10) | ||
| t.AssertNil(err) | ||
| rowsAffected, err = result.RowsAffected() | ||
| t.AssertNil(err) | ||
| t.Assert(rowsAffected, 1) |
Comment on lines
+446
to
+453
| t.Assert(rowsAffected, 1) | ||
|
|
||
| data["nickname"] = "T10-updated" | ||
| result, err = db.Save(ctx, "t_user", data, 10) | ||
| t.AssertNil(err) | ||
| rowsAffected, err = result.RowsAffected() | ||
| t.AssertNil(err) | ||
| t.Assert(rowsAffected, 1) |
Comment on lines
+442
to
+443
| result, err := db.Save(ctx, "t_user", data, 10) | ||
| t.AssertNil(err) |
houseme
approved these changes
May 8, 2026
Member
|
我看改动以后sqlite如果遇到复合主键必须所有字段都存在，但是在pgsql或者dm中的逻辑是复合主键只要遇到其中有一个就行了，几个TODO consider composite primary keys的todo注释也证明了这一点，是否需要让其他pgsql/dm/oracle遇到复合主键也走严格检查？ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request improves the upsert (
Save) behavior for both SQLite and SQLiteCGO drivers by allowing them to automatically infer primary key columns as conflict targets whenOnConflictis not explicitly specified. This brings their behavior in line with other upsert-capable drivers, enhancing usability and reducing the need for manual configuration. The implementation includes new logic in the drivers, updated tests to verify the new behavior, and documentation/spec updates.SQLite/SQLiteCGO Driver Improvements:
sqliteandsqlitecgodrivers now automatically detect and use table primary keys as conflict columns forSaveoperations ifOnConflictis not provided, and return a clear error if no usable primary key is present in the data. [1] [2]Testing Enhancements:
Saveworks as expected when primary keys are present, updates existing rows, and returns an error if required primary keys are missing. [1] [2]Specification and Documentation: