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

When reading from the db, If there's a gorm:"default:null" then the returned resource doesn't have it's values set correctly. #6351

Closed
jimlambrt opened this issue May 24, 2023 · 7 comments
Assignees
Labels
type:with reproduction steps with reproduction steps

Comments

@jimlambrt
Copy link
Contributor

GORM Playground Link

go-gorm/playground#602

Description

We recently updated our gorm dependency and we had some regression tests fail. During our investigation we discovered that when the there's a gorm tag of default:null the value from the database is not set in the returned resource.

We would expect that anytime a resource is read that it's value should match the values in the database, regardless of any gorm tags.

@github-actions github-actions bot added the type:with reproduction steps with reproduction steps label May 24, 2023
@black-06
Copy link
Contributor

This is really strange.

Here data is nil so the field is not set.

gorm/schema/field.go

Lines 797 to 800 in 11fdf46

field.Set = func(ctx context.Context, value reflect.Value, v interface{}) (err error) {
switch data := v.(type) {
case **string:
if data != nil && *data != nil {

Does this need to be fixed? @a631807682

@a631807682
Copy link
Member

We do not recommend re-using the model in the finished api, it may have worked in some intermediate version of gorm for some reason, but this was actually an accident, due to a bug caused by an optimization commit.
For specific reasons, see the issues associated refer to #6219

@jimlambrt
Copy link
Contributor Author

I feel like this is a breaking change... we used this successfully in the following versions:

v1.24.5
v1.23.8
1.22.3
v1.21.17-0.20211013130203-9a5ba3760424
v1.21.14

@a631807682
Copy link
Member

Need to investigate

@a631807682
Copy link
Member

This behavior was last changed in v1.23.0. It is not expected that such behavior occurs after this version.
Sorry, this feat is no longer supported as we are unable to support both this feat and embedded.

@jimlambrt
Copy link
Contributor Author

jimlambrt commented May 31, 2023

I respect your decision as the maintainers, but it would seem trivial to support both embedded and this behavior by simply adding a function call before/while reading the resource like:

func clearNullResourceFields(ctx context.Context, db *gorm.DB, i interface{}) error {
	stmt := db.Model(i).Statement
	if err := stmt.Parse(i); err != nil {
		return err
	}
	v := reflect.ValueOf(i)
	for _, f := range stmt.Schema.Fields {
		switch {
		case f.PrimaryKey:
			continue
		case !strings.EqualFold(f.DefaultValue, "null"):
			continue
		default:
			_, isZero := f.ValueOf(ctx, v)
			if isZero {
				continue
			}
			if err := f.Set(stmt.Context, v, f.DefaultValueInterface); err != nil {
				return fmt.Errorf("unable to set value of non-zero field: %w", err)
			}
		}
	}
	return nil
}

jimlambrt added a commit to hashicorp/go-dbw that referenced this issue May 31, 2023
Added RW.clearDefaultNullResourceFields which will clear fields in the
resource which are defaulted to a null value.  This addresses the
unfixed issue in gorm: go-gorm/gorm#6351
jimlambrt added a commit to hashicorp/go-dbw that referenced this issue May 31, 2023
)

Added RW.clearDefaultNullResourceFields which will clear fields in the
resource which are defaulted to a null value.  This addresses the
unfixed issue in gorm: go-gorm/gorm#6351
@a631807682
Copy link
Member

I respect your decision as the maintainers, but it would seem trivial to support both embedded and this behavior by simply adding a function call before/while reading the resource like:

func clearNullResourceFields(ctx context.Context, db *gorm.DB, i interface{}) error {
	stmt := db.Model(i).Statement
	if err := stmt.Parse(i); err != nil {
		return err
	}
	v := reflect.ValueOf(i)
	for _, f := range stmt.Schema.Fields {
		switch {
		case f.PrimaryKey:
			continue
		case !strings.EqualFold(f.DefaultValue, "null"):
			continue
		default:
			_, isZero := f.ValueOf(ctx, v)
			if isZero {
				continue
			}
			if err := f.Set(stmt.Context, v, f.DefaultValueInterface); err != nil {
				return fmt.Errorf("unable to set value of non-zero field: %w", err)
			}
		}
	}
	return nil
}

This solves the problem to a certain extent, but doesn't fit well in gorm.

  1. Since automatic migration is not required, default:null is not required, and may not match in this case
  2. Due to version changes, this function is invalid, re-adding it may be a breaking change for the current version, because I don't know if the current user needs to reset its value (this may bring new permission definitions, because if you disable read permission, gorm should not change its value, and its changes may also be related to hooks).
  3. Judging from the current feedback, not many users need this function, which will bring a certain amount of overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:with reproduction steps with reproduction steps
Projects
None yet
Development

No branches or pull requests

4 participants