8000 fix: serializer use default valueOf in assignInterfacesToValue by ag9920 · Pull Request #5168 · go-gorm/gorm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: serializer use default valueOf in assignInterfacesToValue #5168

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
Closed
wants to merge 1 commit into from
Closed

fix: serializer use default valueOf in assignInterfacesToValue #5168

wants to merge 1 commit into from

Conversation

ag9920
Copy link
Contributor
@ag9920 ag9920 commented Mar 17, 2022 •
edited
Loading
  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

Fix the serializer feature in the scenario of Assign().FirstOrCreate() and provide the corresponding test case.

User Case Description

My team uses Assign().FirstCreate() to implement CreateOrUpdateXXXX function. But when I try to run the following code (the intention was to simply insert the new record into the user_config table). I got an error message " failed to set value {"CommonItemList":[1,2,3,4]} to field UserConfig ".

Then I dig into the FirstOrCreate() function, and compare the implementation with Create function. GORM would do a field.ValueOf to get the field and then call field.Set to actually set the data (in assignInterfacesToValue function). After assigning, GORM would call Create() to insert the data.

Since se 8000 rializer's ValueOf returned a schema.serializer object, not the real FieldType (in this case, *main.UserConfig), it's not possible to assign the value to the field.

schema.serializer serves like a wrapper for the field that requires serialization, it implements driver.Valuer interface.

But the corresponding ValueOf method will be called in ConvertToCreateValues function, which transforms *main.UserConfig to schema.serializer. So the early transformation in assignInterfacesToValue in not needed, we just need the default ValueOf implementation.

package main

import (
	"context"
	"fmt"

	"gorm.io/driver/sqlite"
	"gorm.io/gorm"
)

type BizUserConfig struct {
	ID         int64       `gorm:"column:id;primary_key" json:"id"`
	UserConfig *UserConfig `gorm:"column:user_config;type:string;serializer:json" json:"user_config"`
}

type UserConfig struct {
	CommonItemList []int64
}

func (w *BizUserConfig) TableName() string {
	return "biz_user_config"
}

func CreateOrUpdateUserConfig(ctx context.Context, cfg *BizUserConfig) error {
	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
	if err != nil {
		return err
	}
	err = db.AutoMigrate(&BizUserConfig{})
	if err != nil {
		return err
	}
	out := BizUserConfig{}
	tx := db.
		Model(&BizUserConfig{}).
		Assign(cfg).
		FirstOrCreate(&out)

	if tx.Error != nil {
		return fmt.Errorf("gorm.FirstOrCreate err=%s", tx.Error)
	}
	return nil
}

func main() {
	ctx := context.Background()
	cfg := &BizUserConfig{
		UserConfig: &UserConfig{
			CommonItemList: []int64{1, 2, 3, 4},
		},
	}
	err := CreateOrUpdateUserConfig(ctx, cfg)
	if err != nil {
		fmt.Println("err", err)
	}
}

@ag9920 ag9920 closed this Mar 17, 2022
@ag9920 ag9920 reopened this Mar 17, 2022
@ag9920 ag9920 force-pushed the fix_serializer_assign branch from 18af4ba to b5796c9 Compare March 17, 2022 14:54
@jinzhu
Copy link
Member
jinzhu commented Mar 17, 2022

The Valuer, ReflectValuer func write like before mostly due to performance reason, want to cache as much as possible when parsing the struct.

@ag9920 ag9920 force-pushed the fix_serializer_assign branch from b5796c9 to fb92330 Compare March 18, 2022 02:54
@ag9920
Copy link
Contributor Author
ag9920 commented Mar 18, 2022

hi @jinzhu , thanks for point out the performance issue. I just revert back the replacement of DefaultValueOf and DefaultReflectValueOf in setupValuerAndSetter. But for the sake of fix the problem, I keep the DefaultValueOf method call with the serializer if statement in assignInterfacesToValue.

Do you have any better idea on how to fix it? Currently I think for the serializer scenario, there must exist two different ValueOf, one that gives a schema.serializer that implements driver.Valuer, one that simply gives the original reflect value. And we should use the second one for assign values.

@ag9920
Copy link
Contributor Author
ag9920 commented Mar 18, 2022

Also another thing to notice, in setupValuerAndSetter GORM replaced the default ValueOf for serializer, the returned value is actually a plain schema.serializer.

if field.Serializer != nil {
	oldValuerOf := field.ValueOf
	field.ValueOf = func(ctx context.Context, v reflect.Value) (interface{}, bool) {
		value, zero := oldValuerOf(ctx, v)
		if zero {
			return value, zero
		}

		s, ok := value.(SerializerValuerInterface)
		if !ok {
			s = field.Serializer
		}

		return serializer{
			Field:           field,
			SerializeValuer: s,
			Destination:     v,
			Context:         ctx,
			fieldValue:      value,
		}, false
	}
}

But in the replacement of serializer's field.Set method, the type assert for v is a pointer to schema.serializer

field.Set = func(ctx context.Context, value reflect.Value, v interface{}) (err error) {
	if s, ok := v.(*serializer); ok {
		if err = s.Serializer.Scan(ctx, field, value, s.value); err == nil {
			if sameElemType {
				field.ReflectValueOf(ctx, value).Set(reflect.ValueOf(s.Serializer).Elem())
				s.Serializer = reflect.New(reflect.Indirect(reflect.ValueOf(field.Serializer)).Type()).Interface().(SerializerInterface)
			} else if sameType {
				field.ReflectValueOf(ctx, value).Set(reflect.ValueOf(s.Serializer))
				s.Serializer = reflect.New(reflect.Indirect(reflect.ValueOf(field.Serializer)).Type()).Interface().(SerializerInterface)
			}
		}
	} else {
		err = oldFieldSetter(ctx, value, v)
	}
	return
}

Is that a typo ?

@jinzhu jinzhu closed this in 3c00980 Mar 18, 2022
@jinzhu
Copy link
Member
jinzhu commented Mar 18, 2022

Also another thing to notice, in setupValuerAndSetter GORM replaced the default ValueOf for serializer, the returned value is actually a plain schema.serializer.

if field.Serializer != nil {
	oldValuerOf := field.ValueOf
	field.ValueOf = func(ctx context.Context, v reflect.Value) (interface{}, bool) {
		value, zero := oldValuerOf(ctx, v)
		if zero {
			return value, zero
		}

		s, ok := value.(SerializerValuerInterface)
		if !ok {
			s = field.Serializer
		}

		return serializer{
			Field:           field,
			SerializeValuer: s,
			Destination:     v,
			Context:         ctx,
			fieldValue:      value,
		}, false
	}
}

But in the replacement of serializer's field.Set method, the type assert for v is a pointer to schema.serializer

field.Set = func(ctx context.Context, value reflect.Value, v interface{}) (err error) {
	if s, ok := v.(*serializer); ok {
		if err = s.Serializer.Scan(ctx, field, value, s.value); err == nil {
			if sameElemType {
				field.ReflectValueOf(ctx, value).Set(reflect.ValueOf(s.Serializer).Elem())
				s.Serializer = reflect.New(reflect.Indirect(reflect.ValueOf(field.Serializer)).Type()).Interface().(SerializerInterface)
			} else if sameType {
				field.ReflectValueOf(ctx, value).Set(reflect.ValueOf(s.Serializer))
				s.Serializer = reflect.New(reflect.Indirect(reflect.ValueOf(field.Serializer)).Type()).Interface().(SerializerInterface)
			}
		}
	} else {
		err = oldFieldSetter(ctx, value, v)
	}
	return
}

Is that a typo ?

fixed the issue, and merged the PR in 3c00980

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ag9920 @jinzhu
0