fix: serializer use default valueOf in assignInterfacesToValue

[ag9920]

• 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 serializer'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)
	}
}

[jinzhu]

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

[ag9920]

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]

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]

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!