Skip to content

fixes #7486 #7492

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

Merged
merged 3 commits into from
Jun 25, 2025
Merged

fixes #7486 #7492

merged 3 commits into from
Jun 25, 2025

Conversation

Eshan-Jogwar
Copy link
Contributor

@Eshan-Jogwar Eshan-Jogwar commented Jun 19, 2025

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

What did this pull request do?

it fixes #7486. a subset of struct of the main model when passed through to update would create an error in the tx.Statement.Changed() function. this PR fixed this problem

below is a example code which demonstrates that condition
image

struct used for the reference
type Man struct {
Id string
Age string
Name string
Detail string
}

The problem was that if a complete man struct gorm can predict which the column by column index but if a shorter subset of the struct is passed for quick changes then the index of the column lets say Age will not equate to the index in the struct.

User Case Description

fixed issue of #7486

@jinzhu
Copy link
Member

jinzhu commented Jun 23, 2025

Can you add some tests?

@Eshan-Jogwar
Copy link
Contributor Author

yes i can add tests. working on it

@Eshan-Jogwar
Copy link
Contributor Author

sir i added the test case file for the bug fix. Thank You for such a great project and a good community.

@Eshan-Jogwar
Copy link
Contributor Author

I want to contribute more to the repository. Is there any list as top priority issues or anything which needs to be done first. Asking because I did not found such information anywhere.

@jinzhu jinzhu merged commit 1e8baf5 into go-gorm:master Jun 25, 2025
23 checks passed
@jinzhu
Copy link
Member

jinzhu commented Jun 25, 2025

I want to contribute more to the repository. Is there any list as top priority issues or anything which needs to be done first. Asking because I did not found such information anywhere.

Thank you for your contribution! If you'd like to proceed, I believe the best approach would be to start from one of the open issues labeled with "type:with reproduction steps":
https://github.com/go-gorm/gorm/issues?q=is%3Aissue%20state%3Aopen%20label%3A%22type%3Awith%20reproduction%20steps%22

@liov
Copy link
Contributor

liov commented Jun 27, 2025

I don't think there should be support for usages that aren't in the documentation, this change could slow down performance to some extent.

@liov
Copy link
Contributor

liov commented Jun 27, 2025

This is a question that arises from this

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 -> panic: reflect: Field index out of range
	err = idx.update(change3)
	if err != nil {
		return
	}
}

2025/06/27 18:10:08 D:/code/playground/main_test.go:17
[2.614ms] [rows:1] INSERT INTO `men` (`name`,`age`,`id`) VALUES ("Man1",1,1) RETURNING `id`
yes

2025/06/27 18:10:09 D:/code/playground/main.go:13 SLOW SQL >= 200ms
[1662.197ms] [rows:1] UPDATE `men` SET `age`=10 WHERE id = 1 AND `id` = 1
no

2025/06/27 18:10:09 D:/code/playground/main.go:13
[0.000ms] [rows:1] UPDATE `men` SET `age`=10 WHERE id = 1 AND `id` = 1
yes

2025/06/27 18:10:11 D:/code/playground/main.go:13 SLOW SQL >= 200ms
[776.255ms] [rows:1] UPDATE `men` SET `age`=10 WHERE id = 1 AND `id` = 1

@jinzhu

@liov
Copy link
Contributor

liov commented Jul 2, 2025

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}})

If we need to support other types of structs, I believe similar issues will arise in many other places.

	changed := func(field *schema.Field) bool {
		fieldValue, _ := field.ValueOf(stmt.Context, modelValue)
		if v, ok := selectColumns[field.DBName]; (ok && v) || (!ok && !restricted) {
			if mv, mok := stmt.Dest.(map[string]interface{}); mok {
				...
			} else {
				destValue := reflect.ValueOf(stmt.Dest)
				for destValue.Kind() == reflect.Ptr {
					destValue = destValue.Elem()
				}
				var changedValue interface{}
				var zero bool
				if destValue.Type() == modelValue.Type() {
					changedValue, zero = field.ValueOf(stmt.Context, destValue)
				} else {
					destField := reflect.ValueOf(destValue).FieldByName(field.Name)
					changedValue, zero = destField.Interface(), destField.IsZero()
				}
				if v {
					return !utils.AssertEqual(changedValue, fieldValue)
				}
				return !zero && !utils.AssertEqual(changedValue, fieldValue)
			}
		}
		return false
	}

@jinzhu

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.

Statement.Changed can't work on a new custom struct
3 participants