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 nil pointer structs should be retrieved as nil #5431

Closed
nickpalmer opened this issue Jun 15, 2022 · 4 comments · Fixed by #6219
Closed

Embedded nil pointer structs should be retrieved as nil #5431

nickpalmer opened this issue Jun 15, 2022 · 4 comments · Fixed by #6219
Assignees
Labels
type:feature_request feature request

Comments

@nickpalmer
Copy link

Describe the feature

Gorm should round trip an embedded pointer to a struct as nil when the struct is given as nil.

Motivation

My domain model has optional embedded objects, which then may have required fields on them.

Our validator will validate the embedded object's required fields if the pointer value is non-nil but will ignore the required validations on the embedded struct if it is nil.

Gorm should detect that that the embedded struct has the zero value and not set it to the zero value if only zero values have been loaded into that value.

Without this a save of nil is not preserved on reload for the embedded struct.

Related Issues

  • None that I could find.

Example Failing Test

diff --git a/tests/embedded_struct_test.go b/tests/embedded_struct_test.go
index 312a5c3..e5db47a 100644
--- a/tests/embedded_struct_test.go
+++ b/tests/embedded_struct_test.go
@@ -90,9 +90,15 @@ func TestEmbeddedPointerTypeStruct(t *testing.T) {
                URL   string
        }

+       type Author struct {
+               ID    string
+               Name  string
+               Email string
+       }
+
        type HNPost struct {
                *BasePost
                Upvotes int32
+               *Author `gorm:"EmbeddedPrefix:user_"` // Embedded struct
        }

        DB.Migrator().DropTable(&HNPost{})
@@ -110,6 +116,10 @@ func TestEmbeddedPointerTypeStruct(t *testing.T) {
        if hnPost.Title != "embedded_pointer_type" {
                t.Errorf("Should find correct value for embedded pointer type")
        }
+
+       if hnPost.Author != nil {
+               t.Errorf("Expected to get back a nil Author but got: %v", hnPost.Author)
+       }
 }

 type Content struct {

This change to the existing test outputs:

--- FAIL: TestEmbeddedPointerTypeStruct (0.01s)
    embedded_struct_test.go:121: Expected to get back a nil Author but got: &{  }

This test would pass if this feature were implemented.

@a631807682
Copy link
Member

Cause by #5417 #5388

When we reuse Slice elem, we need to reset its value, which results in constructing a embedded model. This model may also have been instantiated in other subsets of slices.

We can only reset before Field.Set , or set embedded model with zero value after, but both make #5388 no performance gain.

I recommend adding test files and revert them.
@jinzhu

@nickpalmer
Copy link
Author

nickpalmer commented Jun 16, 2022

I recommend adding test files and revert them. @jinzhu

Not sure what you mean here.

As a workaround I wrote a utility that walks the object and nils out any zero values recursively depth first, but of course it would be faster for Gorm to recycle the zero values it allocated in scan.go instead of Field.Set them.

It is probably necessary to add a Config defaulting to off to allow users of Gorm turn on this behavior, since consumers of Gorm may have code that already assumes they get a zero value and not nil.

@rgcostea
Copy link

I had to implement a similar utility to work around this. Note that I am using gorm 1.23.4 and I'm seeing different results than those from @nickpalmer . If the Author remains as defined above in his test, then the Author is returned as nil. But if the Author struct is changed to use a pointer within it like so:

type Author struct {
	ID    string
	Name  string
	Email *string
}

Then, the result is &{ }

When upgrading to 1.23.8, I see &{ } either way.

ori-amizur added a commit to ori-amizur/assisted-service that referenced this issue Dec 6, 2022
…ining structure

There are locations that our code assumes that embedded structure is
always not nil.  This change adds tests to verify it.
For more information: go-gorm/gorm#5431
openshift-merge-robot pushed a commit to openshift/assisted-service that referenced this issue Dec 6, 2022
…ining structure (#4730)

There are locations that our code assumes that embedded structure is
always not nil.  This change adds tests to verify it.
For more information: go-gorm/gorm#5431
@akshay58538
Copy link

@jinzhu Is this issue being worked upon? Has this been acknowledged?

This is causing regression failure for us. One of our embedded struct which used to be read as nil, is now coming as zero value of the struct.

This was working for us in v1.23.4 and breaking in 1.24.6.

danielerez pushed a commit to danielerez/assisted-service that referenced this issue Oct 15, 2023
…ining structure (openshift#4730)

There are locations that our code assumes that embedded structure is
always not nil.  This change adds tests to verify it.
For more information: go-gorm/gorm#5431
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature_request feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants