Skip to content

Conversation

liov
Copy link
Contributor

@liov liov commented Jul 22, 2025

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

This PR is for: #7492, doesn't completely solve the problem,, and only optimizes the performance for FieldByName to avoid every for loop

The implementation of #7492 does not completely solve the problem and introduces performance issues

The following is a reprinted comment

type Age struct {
	Age int
}

func TestGORM(t *testing.T) {
	man := Man{Id: 1, Name: "Man1", Age: 1}
	DB.Create(&man)

	// update data
	idx := Man{Id: 1}

	change1 := Man{
		Age: 10,
	}
	change2 := map[string]interface{}{
		"age": 10,
	}
	change3 := struct {
		Age
	}{Age{Age: 10}}

	_, _, _ = change1, change2, change3
	var err error

	// change1 -> yes
	err = idx.update(change1)
	if err != nil {
		return
	}

	// change2 -> yes
	err = idx.update(change2)
	if err != nil {
		return
	}

	// change3 -> yes
	err = idx.update(change3)
	if err != nil {
		return
	}
}

After a lot of testing and consideration, I still don't think this PR should be merged.

Firstly, FieldByName performs poorly with large structs, which makes caching fields ineffective and unnecessary.

Secondly, field.ValueOf only addresses one case — the default implementation remains problematic. Additionally, field.ReflectValueOf hasn't been modified at all.

	switch {
	case len(field.StructField.Index) == 1 && fieldIndex >= 0:
		field.ValueOf = func(ctx context.Context, value reflect.Value) (interface{}, bool) {
			fieldValue := reflect.Indirect(value).FieldByName(field.Name)
			return fieldValue.Interface(), fieldValue.IsZero()
		}
	default:
		field.ValueOf = func(ctx context.Context, v reflect.Value) (interface{}, bool) {
			v = reflect.Indirect(v)
			for _, fieldIdx := range field.StructField.Index {
				if fieldIdx >= 0 {
					v = v.Field(fieldIdx)
...
}

And filed. FindByName does not solve problems similar to the following

type Age struct {
	Age int
}
db.Updates(struct {
		Age
	}{Age{Age: 30}})

@jinzhu
Copy link
Member

jinzhu commented Jul 22, 2025

Maybe we should introduce another field DestSchema, and use DestSchema specifically to set field values.

@jinzhu jinzhu merged commit 52b4744 into go-gorm:master Jul 22, 2025
25 of 26 checks passed
@liov liov mentioned this pull request Jul 23, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants