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

[omkar-foss]

Fixes #7090.

• [*] Do only one thing
• [*] Non breaking API changes
• [*] Tested

What did this pull request do?

• This PR adds handling for the assignment of a nil value to a *uuid.UUID field (resolved as a reflect.Ptr to a reflect.Array), to ensure that the model object reflects the correct value after Updates() has completed.

• This PR also adds few supporting test cases for Updates() with a map and uuid.UUID column.

User Case Description

This fix is necessary to keep the gorm model object consistent with the database operation performed via Updates(). Which means that if nil value is set to a UUID column in the database, then the same should reflect in the model object as well.

Other Notes

Please note that I had to inevitably add the github.com/google/uuid package to the main go.mod file here since the UserUUID column in the test User model needed it as an import. The UserUUID field is being used in the new test cases that I've added in this PR.

[a631807682]

Thanks for the PR.
Why is Array special, what about Slice? I think we shouldn't provide unique compatibility for external packages.
Actually the issue is whether we should update nil values ​​to the original object when performing Update. If so, how should the Embedded object handle it?
Actually this PR still doesn't work in Embedded objects.

[omkar-foss]

Hey @a631807682 thank you so much for your review. My responses to your questions are as below.

Why is Array special, what about Slice? I think we shouldn't provide unique compatibility for external packages.

The *uuid.UUID resolves to a reflect.Ptr to a reflect.Array. This is because the uuid.UUID from the Google UUID package is essentially a 128 bit (16 byte) array. Yes, I agree with you that providing unique compatibility for external packages is not a good idea and can bloat up the project, but considering the widespread use of the UUID package in the Go community, can we support it? It will be really helpful for several developers.

Actually the issue is whether we should update nil values ​​to the original object when performing Update.

Yes, we should surely update nil values in the original object when performing Update, since original object needs to be consistent with the underlying database changes performed in the update operation. It's usually a best practice in ORMs to keep the model entity object (i.e. the original object) consistent with the underlying database, as developers may continue using the original object in code for further operations after the update, assuming that it's already in sync with the database.

If so, how should the Embedded object handle it? Actually this PR still doesn't work in Embedded objects.

This PR specifically fixes only the issue with nil assignment for *uuid.UUID. So it doesn't have any code for handling of nil assignment in embedded objects.

[iTanken]

For this issue, my first instinct is to use Customize Data Types:

package datatypes

import (
	"database/sql/driver"

	"github.com/google/uuid"
	"gorm.io/gorm/schema"
)

type UUID uuid.UUID

func (UUID) GormDataType() string {
	return string(schema.String)
}

func (u *UUID) Scan(value interface{}) error {
	var result uuid.UUID
	if err := result.Scan(value); err != nil {
		return err
	}
	*u = UUID(result)
	return nil
}

func (u UUID) Value() (driver.Value, error) {
	return uuid.UUID(u).Value()
}

func (u UUID) String() string {
	return uuid.UUID(u).String()
}

then use datatypes.UUID instead of uuid.UUID:

var uuidPtr *datatypes.UUID = nil
DB.Model(&p).Updates(map[string]interface{}{"unique_id": uuidPtr}).Error
// UPDATE "p" SET "unique_id"=NULL WHERE "id" = 1

[a631807682]

but considering the widespread use of the UUID package in the Go community, can we support it? It will be really helpful for several developers.

Yes, we should try to support it, but I would like it to be in a more general way.

This PR specifically fixes only the issue with nil assignment for *uuid.UUID. So it doesn't have any code for handling of nil assignment in embedded objects.

This test describes the problem with updating the original object in an embedded model, and the behavior that this PR breaks.

The problem here is that to set the value of a field of an embedded object, you need to instantiate the object, even if it is a nil value.

In my opinion, a more general solution may be to use Scanner and Valuer as @iTanken said (if feasible, it can be included in the https://github.com/go-gorm/datatypes project), or to check whether the embedded object of pointer type is zero value after setting all the fields of the object.

gorm/tests/embedded_struct_test.go

Line 101 in 4a50b36

func TestEmbeddedPointerTypeStruct(t *testing.T) {

	type Author struct {
		ID          string
		Name        string
		Email       string
		Age         int
		Content     Content
		ContentPtr  *Content
		Birthday    time.Time
		BirthdayPtr *time.Time
+		UUID        *uuid.UUID
	}

[omkar-foss]

Hey @a631807682 @iTanken, thank you both for your comments.

Yes I totally agree that we should keep it more general, so as per both your suggestions above, I've raised this PR on the gorm/datatypes repo to add UUID as a datatype for gorm.

@a631807682 I've also added handling for embedded structs and have included the test case that you provided above (refer here). That test case is passing now.

Also have replaced uuid.UUID with datatypes.UUID in this PR, but I haven't committed it since it will break until the datatypes PR has been merged. You can review the diff below. I suppose we can keep this PR open until the datatypes PR is reviewed and merged, after which I can add the below diff to this PR.

Note: PR check reviewdog/runner/golangci-lint has been failing due to an issue with filtering, refer this issue for more information.

diff --git a/tests/update_test.go b/tests/update_test.go
index a4f902b..84d41c9 100644
--- a/tests/update_test.go
+++ b/tests/update_test.go
@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"gorm.io/datatypes"
 	"gorm.io/gorm"
 	"gorm.io/gorm/clause"
 	"gorm.io/gorm/utils"
@@ -185,7 +186,7 @@ func TestUpdates(t *testing.T) {
 	user3.Age += 100
 	AssertObjEqual(t, user4, user3, "UpdatedAt", "Age")
 
-	// Updates() with map and uuid.UUID - Case 1 - Update with UUID value
+	// Updates() with map and datatypes.UUID - Case 1 - Update with UUID value
 	uuidVal, uuidErr := uuid.NewUUID()
 	if uuidErr != nil {
 		t.Errorf("No error should occur while generating UUID, but got %v", uuidErr)
@@ -198,8 +199,8 @@ func TestUpdates(t *testing.T) {
 	// Expecting the model object (user4) to reflect the UUID value assignment.
 	AssertEqual(t, user4.UserUUID, uuidVal)
 
-	// Updates() with map and uuid.UUID - Case 2 - Update with UUID nil pointer
-	var nilUUIDPtr *uuid.UUID = nil
+	// 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)
@@ -207,12 +208,13 @@ func TestUpdates(t *testing.T) {
 	// Expecting the model object (user4) to reflect the UUID nil pointer assignment.
 	AssertEqual(t, user4.UserUUID, nilUUIDPtr)
 
-	// Updates() with map and uuid.UUID - Case 3 - Update with a non-nil UUID pointer
+	// Updates() with map and datatypes.UUID - Case 3 - Update with a non-nil UUID pointer
 	uuidVal2, uuidErr := uuid.NewUUID()
 	if uuidErr != nil {
 		t.Errorf("No error should occur while generating UUID, but got %v", uuidErr)
 	}
-	var nonNilUUIDPtr *uuid.UUID = &uuidVal2
+	castedUUIDVal2 := datatypes.UUID(uuidVal2)
+	var nonNilUUIDPtr *datatypes.UUID = &castedUUIDVal2
 	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)
diff --git a/utils/tests/models.go b/utils/tests/models.go
index 7690641..060467c 100644
--- a/utils/tests/models.go
+++ b/utils/tests/models.go
@@ -4,7 +4,7 @@ import (
 	"database/sql"
 	"time"
 
-	"github.com/google/uuid"
+	"gorm.io/datatypes"
 	"gorm.io/gorm"
 )
 
@@ -31,7 +31,7 @@ type User struct {
 	Languages []Language `gorm:"many2many:UserSpeak;"`
 	Friends   []*User    `gorm:"many2many:user_friends;"`
 	Active    bool
-	UserUUID  *uuid.UUID
+	UserUUID  *datatypes.UUID
 }
 
 type Account struct {

[omkar-foss]

Hi! As per our discussion in the above comment, I've added a datatype datatypes.UUID (as well as datatypes.BinUUID), and both have been released in datatypes v1.2.2.

I've updated this PR to use datatypes v1.2.2 and also replaced uuid.UUID with datatypes.UUID as per discussion. Kindly review and let me know if any further changes are required, thank you! 🙂

[omkar-foss]

sqlserver tests have been failing because the GitHub Actions test containers are failing to start. I tried with increased timeouts and retries, and it still fails after trying for around 10 mins.

Will dig deeper on why this is happening, as I see similar failures in #7198 as well. If anyone has any context on this issue, please share. Thanks!

[a631807682]

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 
+}

[omkar-foss]

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! 😄

[a631807682]

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

[omkar-foss]

Sure no problem, thank you for your review though.

[jinzhu]

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.