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.

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

embedded should be nil if not exists #6288

Closed
hykuan opened this issue May 2, 2023 · 4 comments · Fixed by #6293
Closed

embedded should be nil if not exists #6288

hykuan opened this issue May 2, 2023 · 4 comments · Fixed by #6293
Assignees

Comments

@hykuan
Copy link
Contributor

hykuan commented May 2, 2023

Your Question

refer to the pr #6219

the test from tests/embedded_struct_test.go works fine.

but after i added one field with data type int, the test will failed.

from

type Author struct {
	ID    string
	Name  string
	Email string
}

to

type Author struct {
	ID    string
	Name  string
	Email string
	Age   int
}

the output will show: Expected to get back a nil Author but got: &{ 0}

Expected answer

the author should also be nil.

More information

I test int & uint types, here is the result

data type int64 int32 int16 int8 int uint64 uint32 uint16 uint8 uint
test result pass failed failed failed failed pass failed failed failed failed
@github-actions github-actions bot added type:missing reproduction steps missing reproduction steps type:question general questions and removed type:missing reproduction steps missing reproduction steps labels May 2, 2023
@hykuan
Copy link
Contributor Author

hykuan commented May 2, 2023

maybe we need to add something like this ?

from gorm.io/gorm/schema/field.go

	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
		field.Set = func(ctx context.Context, value reflect.Value, v interface{}) (err error) {
			switch data := v.(type) {
			case **int64:
				if data != nil && *data != nil {
					field.ReflectValueOf(ctx, value).SetInt(**data)
				}
+			case **int:
+				if data != nil && *data != nil {
+					field.ReflectValueOf(ctx, value).SetInt(int64(**data))
+				}

@a631807682 a631807682 added type:with reproduction steps with reproduction steps contribution welcome and removed type:question general questions labels May 4, 2023
@a631807682
Copy link
Member

PR welcome

@aclich
Copy link
Contributor

aclich commented May 10, 2023

Hi
I'm facing similar issue too. After tested, found it was cause by the following datatypes

  • interface{} (custom data types)
  • *interface{} (custom data types)
  • *time.time

I add those types in tests/embedded_struct_test.go, and it will also make the test failed.
Like following example

type Content struct {
	Content interface{} `gorm:"type:String"`
}

func (c Content) Value() (driver.Value, error) {
	return json.Marshal(c)
}

func (c *Content) Scan(src interface{}) error {
	...
}

type Author struct {
	ID          string
	Name        string
	Email       string
	Age         int
	Content     Content
	ContentPtr  *Content
	BirthDay    time.Time
	BirthDayPtr *time.Time
}

I've trying to change the reflect logic on struct scanner & pointer scanner to resolve this problem. Not sure if it's gorm wants?
gorm.io/gorm/schema/field.go

diff --git a/schema/field.go b/schema/field.go
index 7d1a178..dd08e05 100644
--- a/schema/field.go
+++ b/schema/field.go
@@ -846,7 +846,7 @@ func (field *Field) setupValuerAndSetter() {
                        field.Set = func(ctx context.Context, value reflect.Value, v interface{}) error {
                                switch data := v.(type) {
                                case **time.Time:
-                                       if data != nil {
+                                       if data != nil && *data != nil {
                                                field.ReflectValueOf(ctx, value).Set(reflect.ValueOf(*data))
                                        }
                                case time.Time:
@@ -882,14 +882,12 @@ func (field *Field) setupValuerAndSetter() {
                                        reflectV := reflect.ValueOf(v)
                                        if !reflectV.IsValid() {
                                                field.ReflectValueOf(ctx, value).Set(reflect.New(field.FieldType).Elem())
+                                       } else if reflectV.Kind() == reflect.Ptr && reflectV.IsNil() {
+                                               return
                                        } else if reflectV.Type().AssignableTo(field.FieldType) {
                                                field.ReflectValueOf(ctx, value).Set(reflectV)
                                        } else if reflectV.Kind() == reflect.Ptr {
-                                               if reflectV.IsNil() || !reflectV.IsValid() {
-                                                       field.ReflectValueOf(ctx, value).Set(reflect.New(field.FieldType).Elem())
-                                               } else {
-                                                       return field.Set(ctx, value, reflectV.Elem().Interface())
-                                               }
+                                               return field.Set(ctx, value, reflectV.Elem().Interface())
                                        } else {
                                                fieldValue := field.ReflectValueOf(ctx, value)
                                                if fieldValue.IsNil() {
@@ -910,14 +908,12 @@ func (field *Field) setupValuerAndSetter() {
                                        reflectV := reflect.ValueOf(v)
                                        if !reflectV.IsValid() {
                                                field.ReflectValueOf(ctx, value).Set(reflect.New(field.FieldType).Elem())
+                                       } else if reflectV.Kind() == reflect.Ptr && reflectV.IsNil() {
+                                               return
                                        } else if reflectV.Type().AssignableTo(field.FieldType) {
                                                field.ReflectValueOf(ctx, value).Set(reflectV)
                                        } else if reflectV.Kind() == reflect.Ptr {
-                                               if reflectV.IsNil() || !reflectV.IsValid() {
-                                                       field.ReflectValueOf(ctx, value).Set(reflect.New(field.FieldType).Elem())
-                                               } else {
-                                                       return field.Set(ctx, value, reflectV.Elem().Interface())
-                                               }
+                                               return field.Set(ctx, value, reflectV.Elem().Interface())
                                        } else {
                                                if valuer, ok := v.(driver.Valuer); ok {
                                                        v, _ = valuer.Value()

@a631807682
Copy link
Member

@aclich You are welcome to create a PR. If you can confirm that this is a bug and provide a unit test, we will review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants