-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix: on confilct with default value #6434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hi @black-06 Which might cause a column's value overwritten by an empty value, here is an example: type User struct {
Name string
UUID string `gorm:"default:gen_random_uuid()"`
}
db.Save(&[]User{{Name: "name"}} |
|
|
Sorry, I may not understand what you mean. func TestSaveWithDefaultValue(t *testing.T) {
if DB.Dialector.Name() != "postgres" {
t.Skip()
}
type WithDefaultValue struct {
Name string `gorm:"primaryKey"`
UUID string `gorm:"default:gen_random_uuid()"`
}
err := DB.Migrator().DropTable(&WithDefaultValue{})
AssertEqual(t, err, nil)
err = DB.AutoMigrate(&WithDefaultValue{})
AssertEqual(t, err, nil)
// INSERT INTO "with_default_values" ("name") VALUES ('jinzhu') ON CONFLICT ("name") DO NOTHING RETURNING "uuid"
err = DB.Debug().Save(&[]WithDefaultValue{{Name: "jinzhu"}}).Error
AssertEqual(t, err, nil)
err = DB.Debug().Save(&[]WithDefaultValue{{Name: "jinzhu"}}).Error
AssertEqual(t, err, nil)
}@a631807682 Would you mind helping me? 😄 |
I'll check it tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values.Columns is already filtered data, it must not contain default value, or contain default value and set a value for it at the same time.
The problem here is that if the default value is a database built-in function ( just like gen_random_uuid() or now(), we don't parse DefaultValueInterface), we don't override it. But for custom types, we do not provide a way to override DefaultValueInterface, so it is always excluded.
Here is a smaller example
type WithDefaultValue struct {
Name string `gorm:"primaryKey"`
Content datatypes.JSONMap `gorm:"default:'{}'"`
}
err := DB.Migrator().DropTable(&WithDefaultValue{})
AssertEqual(t, err, nil)
err = DB.AutoMigrate(&WithDefaultValue{})
AssertEqual(t, err, nil)
dfv := WithDefaultValue{Name: "jinzhu"}
err = DB.Save(&dfv).Error
AssertEqual(t, err, nil)
dfv.Content["name"] = "jinzhu"
err = DB.Clauses(clause.OnConflict{UpdateAll: true}).Create(&dfv).Error
AssertEqual(t, err, nil)
var result WithDefaultValue
err = DB.First(&result).Error
AssertEqual(t, err, nil)
AssertEqual(t, result.Content["name"], "jinzhu")
I'm still thinking about your comment, but your test didn't fail...
|
Sorry, this test is to illustrate the problem with custom types before. |
It didn't... Still this test, It was saved twice, but the uuid did not change. func TestSaveWithDefaultValue(t *testing.T) {
if DB.Dialector.Name() != "postgres" {
t.Skip()
}
type WithDefaultValue struct {
Name string `gorm:"primaryKey"`
UUID string `gorm:"default:gen_random_uuid()"`
}
err := DB.Migrator().DropTable(&WithDefaultValue{})
AssertEqual(t, err, nil)
err = DB.AutoMigrate(&WithDefaultValue{})
AssertEqual(t, err, nil)
// INSERT INTO "with_default_values" ("name") VALUES ('jinzhu') ON CONFLICT ("name") DO NOTHING RETURNING "uuid"
err = DB.Debug().Save(&[]WithDefaultValue{{Name: "jinzhu"}}).Error
AssertEqual(t, err, nil)
err = DB.Debug().Save(&[]WithDefaultValue{{Name: "jinzhu"}}).Error
AssertEqual(t, err, nil)
} |
|
As #6434 (review) says, if the value is not set, it will not be overridden, whereas if the value is set it should be.
Does not rewrite to empty value, I tried other cases, and there is no difference from the current behavior (including the default value for built-in functions, such as cc @jinzhu |
# Conflicts: # tests/create_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where fields with JSON default values were not being properly updated during upsert operations with OnConflict{UpdateAll: true}. The fix simplifies the logic in the OnConflict handler by removing an overly restrictive condition that was incorrectly filtering out columns that should be updated.
Key changes:
- Simplified the OnConflict UpdateAll logic to include all non-primary-key columns in updates
- Removed unnecessary condition checking for default values that was causing JSON fields with defaults to be skipped
- Added test coverage for JSON default fields in both create and association scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| callbacks/create.go | Simplified OnConflict UpdateAll logic by removing the complex HasDefaultValue check and removed the now-unused strings import |
| tests/create_test.go | Fixed typo "OnConfilct" → "OnConflict" and added new test for OnConflict with JSON default values |
| tests/associations_test.go | Added test for FullSaveAssociations with JSON default values to verify the fix works with associations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| value.Dep.Params["foo"] = "new bar" | ||
| if err := DB.Session(&gorm.Session{FullSaveAssociations: true}).Save(&value).Error; err != nil { | ||
| t.Errorf("failed to svae value, got err: %v", err) |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'svae' to 'save'.
| t.Errorf("failed to svae value, got err: %v", err) | |
| t.Errorf("failed to save value, got err: %v", err) |
What did this pull request do?
close #6418 ,
and it has a little relationship with #6129
User Case Description