Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Assigning nil value to *uuid.UUID field in Updates #7099

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: Assigning nil value to *uuid.UUID field in Updates #7099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11ecd03
fix: Assigning nil value to *uuid.UUID field in Updates
omkar-foss Jul 5, 2024
753049d
Add handling for embedded structs
omkar-foss Jul 10, 2024
316303a
Use gorm datatypes.UUID instead of uuid.UUID
omkar-foss Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,14 @@ go 1.18
require (
github.com/jinzhu/inflection v1.0.0
github.com/jinzhu/now v1.1.5
golang.org/x/text v0.14.0
golang.org/x/text v0.18.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, we need to ensure that gorm.io/gorm does not introduce any external dependencies.

We could put all related dependencies within the tests package.

)

require gorm.io/datatypes v1.2.2

require (
filippo.io/edwards25519 v1.1.0 // indirect
github.com/go-sql-driver/mysql v1.8.1 // indirect
github.com/google/uuid v1.6.0 // indirect
gorm.io/driver/mysql v1.5.7 // indirect
)
31 changes: 31 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,37 @@
filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA=
filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4=
github.com/go-sql-driver/mysql v1.7.0/go.mod h1:OXbVy3sEdcQ2Doequ6Z5BW6fXNQTmx+9S1MCJN5yJMI=
github.com/go-sql-driver/mysql v1.8.1 h1:LedoTUt/eveggdHS9qUFC1EFSa8bU2+1pZjSRpvNJ1Y=
github.com/go-sql-driver/mysql v1.8.1/go.mod h1:wEBSXgmK//2ZFJyE+qWnIsVGmvmEKlqwuVSjsCm7DZg=
github.com/golang-sql/civil v0.0.0-20220223132316-b832511892a9 h1:au07oEsX2xN0ktxqI+Sida1w446QrXBRJ0nee3SNZlA=
github.com/golang-sql/sqlexp v0.1.0 h1:ZCD6MBpcuOVfGVqsEmY5/4FtYiKz6tSyUv9LPEDei6A=
github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I=
github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM=
github.com/jackc/pgservicefile v0.0.0-20231201235250-de7065d80cb9 h1:L0QtFUgDarD7Fpv9jeVMgy/+Ec0mtnmYuImjTz6dtDA=
github.com/jackc/pgx/v5 v5.5.5 h1:amBjrZVmksIdNjxGW/IiIMzxMKZFelXbUoPNb+8sjQw=
github.com/jackc/puddle/v2 v2.2.1 h1:RhxXJtFG022u4ibrCSMSiu5aOq1i77R3OHKNJj77OAk=
github.com/jinzhu/inflection v1.0.0 h1:K317FqzuhWc8YvSVlFMCCUb36O/S9MCKRDI7QkRKD/E=
github.com/jinzhu/inflection v1.0.0/go.mod h1:h+uFLlag+Qp1Va5pdKtLDYj+kHp5pxUVkryuEj+Srlc=
github.com/jinzhu/now v1.1.5 h1:/o9tlHleP7gOFmsnYNz3RGnqzefHA47wQpKrrdTIwXQ=
github.com/jinzhu/now v1.1.5/go.mod h1:d3SSVoowX0Lcu0IBviAWJpolVfI5UJVZZ7cO71lE/z8=
github.com/mattn/go-sqlite3 v1.14.15 h1:vfoHhTN1af61xCRSWzFIWzx2YskyMTwHLrExkBOjvxI=
github.com/microsoft/go-mssqldb v0.17.0 h1:Fto83dMZPnYv1Zwx5vHHxpNraeEaUlQ/hhHLgZiaenE=
golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30=
golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ=
golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=
golang.org/x/text v0.18.0 h1:XvMDiNzPAl0jr17s6W9lcaIhGUfUORdGCNsuLmPG224=
golang.org/x/text v0.18.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY=
gorm.io/datatypes v1.2.2 h1:sdn7ZmG4l7JWtMDUb3L98f2Ym7CO5F8mZLlrQJMfF9g=
gorm.io/datatypes v1.2.2/go.mod h1:f4BsLcFAX67szSv8svwLRjklArSHAvHLeE3pXAS5DZI=
gorm.io/driver/mysql v1.5.6 h1:Ld4mkIickM+EliaQZQx3uOJDJHtrd70MxAUqWqlx3Y8=
gorm.io/driver/mysql v1.5.6/go.mod h1:sEtPWMiqiN1N1cMXoXmBbd8C6/l+TESwriotuRRpkDM=
gorm.io/driver/mysql v1.5.7 h1:MndhOPYOfEp2rHKgkZIhJ16eVUIRf2HmzgoPmh7FCWo=
gorm.io/driver/mysql v1.5.7/go.mod h1:sEtPWMiqiN1N1cMXoXmBbd8C6/l+TESwriotuRRpkDM=
gorm.io/driver/postgres v1.5.0 h1:u2FXTy14l45qc3UeCJ7QaAXZmZfDDv0YrthvmRq1l0U=
gorm.io/driver/sqlite v1.4.3 h1:HBBcZSDnWi5BW3B3rwvVTc510KGkBkexlOg0QrmLUuU=
gorm.io/driver/sqlserver v1.4.1 h1:t4r4r6Jam5E6ejqP7N82qAJIJAht27EGT41HyPfXRw0=
gorm.io/gorm v1.25.7/go.mod h1:hbnx/Oo0ChWMn1BIhpy1oYozzpM15i4YPuHDmfYtwg8=
4 changes: 3 additions & 1 deletion schema/field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,7 +896,9 @@ func (field *Field) setupValuerAndSetter() {
if !reflectV.IsValid() {
field.ReflectValueOf(ctx, value).Set(reflect.New(field.FieldType).Elem())
} else if reflectV.Kind() == reflect.Ptr && reflectV.IsNil() {
return
if field.FieldType.Elem().Kind() == reflect.Array && field.OwnerSchema == nil {
field.ReflectValueOf(ctx, value).Set(reflectV)
}
} else if reflectV.Type().AssignableTo(field.FieldType) {
field.ReflectValueOf(ctx, value).Set(reflectV)
} else if reflectV.Kind() == reflect.Ptr {
Expand Down
6 changes: 3 additions & 3 deletions tests/connpool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,12 @@ func TestConnPoolWrapper(t *testing.T) {
db: nativeDB,
expect: []string{
"SELECT VERSION()",
"INSERT INTO `users` (`created_at`,`updated_at`,`deleted_at`,`name`,`age`,`birthday`,`company_id`,`manager_id`,`active`) VALUES (?,?,?,?,?,?,?,?,?)",
"INSERT INTO `users` (`created_at`,`updated_at`,`deleted_at`,`name`,`age`,`birthday`,`company_id`,`manager_id`,`active`,`user_uuid`) VALUES (?,?,?,?,?,?,?,?,?,?)",
"SELECT * FROM `users` WHERE name = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT ?",
"INSERT INTO `users` (`created_at`,`updated_at`,`deleted_at`,`name`,`age`,`birthday`,`company_id`,`manager_id`,`active`) VALUES (?,?,?,?,?,?,?,?,?)",
"INSERT INTO `users` (`created_at`,`updated_at`,`deleted_at`,`name`,`age`,`birthday`,`company_id`,`manager_id`,`active`,`user_uuid`) VALUES (?,?,?,?,?,?,?,?,?,?)",
"SELECT * FROM `users` WHERE name = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT ?",
"SELECT * FROM `users` WHERE name = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT ?",
"INSERT INTO `users` (`created_at`,`updated_at`,`deleted_at`,`name`,`age`,`birthday`,`company_id`,`manager_id`,`active`) VALUES (?,?,?,?,?,?,?,?,?)",
"INSERT INTO `users` (`created_at`,`updated_at`,`deleted_at`,`name`,`age`,`birthday`,`company_id`,`manager_id`,`active`,`user_uuid`) VALUES (?,?,?,?,?,?,?,?,?,?)",
"SELECT * FROM `users` WHERE name = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT ?",
"SELECT * FROM `users` WHERE name = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT ?",
},
Expand Down
2 changes: 2 additions & 0 deletions tests/embedded_struct_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"
"time"

"github.com/google/uuid"
"gorm.io/gorm"
. "gorm.io/gorm/utils/tests"
)
Expand Down Expand Up @@ -114,6 +115,7 @@ func TestEmbeddedPointerTypeStruct(t *testing.T) {
ContentPtr *Content
Birthday time.Time
BirthdayPtr *time.Time
AuthorUUID *uuid.UUID
}

type HNPost struct {
Expand Down
2 changes: 1 addition & 1 deletion tests/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ require (
github.com/jinzhu/now v1.1.5
github.com/lib/pq v1.10.9
github.com/stretchr/testify v1.9.0
gorm.io/datatypes v1.2.2
gorm.io/driver/mysql v1.5.7
gorm.io/driver/postgres v1.5.9
gorm.io/driver/sqlite v1.5.6
Expand All @@ -24,7 +25,6 @@ require (
github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect
github.com/jackc/pgx/v5 v5.7.1 // indirect
github.com/jinzhu/inflection v1.0.0 // indirect
github.com/kr/text v0.2.0 // indirect
github.com/mattn/go-sqlite3 v1.14.23 // indirect
github.com/microsoft/go-mssqldb v1.7.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
Expand Down
6 changes: 3 additions & 3 deletions tests/sql_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ func TestDryRun(t *testing.T) {
dryRunDB := DB.Session(&gorm.Session{DryRun: true})

stmt := dryRunDB.Create(&user).Statement
if stmt.SQL.String() == "" || len(stmt.Vars) != 9 {
if stmt.SQL.String() == "" || len(stmt.Vars) != 10 {
t.Errorf("Failed to generate sql, got %v", stmt.SQL.String())
}

Expand Down Expand Up @@ -403,7 +403,7 @@ func TestToSQL(t *testing.T) {
sql = DB.ToSQL(func(tx *gorm.DB) *gorm.DB {
return tx.Model(&User{}).Create(user)
})
assertEqualSQL(t, `INSERT INTO "users" ("created_at","updated_at","deleted_at","name","age","birthday","company_id","manager_id","active") VALUES ('2021-10-18 00:00:00','2021-10-18 00:00:00',NULL,'foo',20,NULL,NULL,NULL,false) RETURNING "id"`, sql)
assertEqualSQL(t, `INSERT INTO "users" ("created_at","updated_at","deleted_at","name","age","birthday","company_id","manager_id","active","user_uuid") VALUES ('2021-10-18 00:00:00','2021-10-18 00:00:00',NULL,'foo',20,NULL,NULL,NULL,false,NULL) RETURNING "id"`, sql)

// save
user = &User{Name: "foo", Age: 20}
Expand All @@ -412,7 +412,7 @@ func TestToSQL(t *testing.T) {
sql = DB.ToSQL(func(tx *gorm.DB) *gorm.DB {
return tx.Model(&User{}).Save(user)
})
assertEqualSQL(t, `INSERT INTO "users" ("created_at","updated_at","deleted_at","name","age","birthday","company_id","manager_id","active") VALUES ('2021-10-18 00:00:00','2021-10-18 00:00:00',NULL,'foo',20,NULL,NULL,NULL,false) RETURNING "id"`, sql)
assertEqualSQL(t, `INSERT INTO "users" ("created_at","updated_at","deleted_at","name","age","birthday","company_id","manager_id","active","user_uuid") VALUES ('2021-10-18 00:00:00','2021-10-18 00:00:00',NULL,'foo',20,NULL,NULL,NULL,false,NULL) RETURNING "id"`, sql)

// updates
user = &User{Name: "bar", Age: 22}
Expand Down
33 changes: 33 additions & 0 deletions tests/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"
"time"

"gorm.io/datatypes"
"gorm.io/gorm"
"gorm.io/gorm/clause"
"gorm.io/gorm/utils"
Expand Down Expand Up @@ -183,6 +184,38 @@ func TestUpdates(t *testing.T) {

user3.Age += 100
AssertObjEqual(t, user4, user3, "UpdatedAt", "Age")

// Updates() with map and datatypes.UUID - Case 1 - Update with UUID value
uuidVal := datatypes.NewUUIDv4()
tx := DB.Model(&user4)
uuidErr := tx.Updates(map[string]interface{}{"user_uuid": uuidVal}).Error
if uuidErr != nil {
t.Errorf("No error should occur while updating with UUID value, but got %v", uuidErr)
}
// Expecting the model object (user4) to reflect the UUID value assignment.
AssertEqual(t, user4.UserUUID, uuidVal)

// Updates() with map and datatypes.UUID - Case 2 - Update with UUID nil pointer
var nilUUIDPtr *datatypes.UUID = nil
uuidErr = tx.Updates(map[string]interface{}{"user_uuid": nilUUIDPtr}).Error
if uuidErr != nil {
t.Errorf("No error should occur while updating with nil UUID pointer, but got %v", uuidErr)
}
// Expecting the model object (user4) to reflect the UUID nil pointer assignment.
AssertEqual(t, user4.UserUUID, nilUUIDPtr)

// Updates() with map and datatypes.UUID - Case 3 - Update with a non-nil UUID pointer
uuidVal2 := datatypes.NewUUIDv1()
if uuidErr != nil {
t.Errorf("No error should occur while generating UUID, but got %v", uuidErr)
}
var nonNilUUIDPtr *datatypes.UUID = &uuidVal2
uuidErr = tx.Updates(map[string]interface{}{"user_uuid": nonNilUUIDPtr}).Error
if uuidErr != nil {
t.Errorf("No error should occur while updating with non-nil UUID pointer, but got %v", uuidErr)
}
// Expecting the model object (user4) to reflect the non-nil UUID pointer assignment.
AssertEqual(t, user4.UserUUID, nonNilUUIDPtr)
}

func TestUpdateColumn(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions utils/tests/models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"database/sql"
"time"

"gorm.io/datatypes"
"gorm.io/gorm"
)

Expand All @@ -30,6 +31,7 @@ type User struct {
Languages []Language `gorm:"many2many:UserSpeak;"`
Friends []*User `gorm:"many2many:user_friends;"`
Active bool
UserUUID *datatypes.UUID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/go-gorm/gorm/pull/7099/files#diff-f67f30d302f2aefbe6f6df6172b789c0bd4d13e36af6cd04d3329d08cbdb7d3fR899

My understanding is that the expectation of this PR is to rewrite uuid to nil, so this expectation should actually take effect in embedded (the embedded structure is nil, or if the embedded has other fields that are not nil, then the field is nil).
But it cannot be done through the judgment of reflect.Array and field.OwnerSchema, and the judgment of reflect.Array is not easy to understand unless someone sees this PR.
From the expectation, I think the PR is only partially completed, although it does solve the problem in #7090.

-UserUUID  *datatypes.UUID
+EmbeddedUUID *EmbeddedUUID `gorm:"embedded"`

+type EmbeddedUUID struct {
+	UserUUID *datatypes.UUID 
+}

Copy link
Contributor Author

@omkar-foss omkar-foss Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree with you that this is not the cleanest way to fix this problem.

Using an embedded structure is much cleaner (and no code change here!), although I'm not sure how many users will be aware of using it when the problem arises - which is why I felt that we should support this way.

But if using embedded structures is the only right way, please feel free to close this PR. Thank you! 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean we should use embedded, but we should keep the same behavior as embedded.
You can wait for jinzhu's review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure no problem, thank you for your review though.

}

type Account struct {
Expand Down
Loading