From 2879c7e762114883eba596ae210e201140829892 Mon Sep 17 00:00:00 2001 From: Douglas Danger Manley Date: Thu, 7 May 2020 16:54:33 -0400 Subject: [PATCH] Issue #3007: Fix search clone array corruption Method-chaining in gorm is predicated on `search`'s `clone` method returning a copy of the `search` instance such that no change that is made to the copy affects the original. However, the original implementation copied the slices of the various slice properties (such as `whereConditions`), with the incorrect understanding that `append` would create a new slice each time that it was called. In some cases, this is true. Practically, go doubles the size of the slice once it gets full, so the following slice `append` calls would result in a new slice: * 0 -> 1 * 1 -> 2 * 2 -> 4 * 4 -> 8 * and so on. So, when the number of "where" conditions (or any other slice property) was 0, 1, 2, or 4, method-chaining would work as expected. However, when it was 3, 5, 6, or 7, modifying the copy would modify the original. The solution is simply to create new slices in `clone`. --- search.go | 52 +++++++++++++++++++++++++++++++++++++++++++++++++- search_test.go | 22 +++++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/search.go b/search.go index 7c4cc184..52ae2efc 100644 --- a/search.go +++ b/search.go @@ -32,7 +32,57 @@ type searchPreload struct { } func (s *search) clone() *search { - clone := *s + clone := search{ + db: s.db, + whereConditions: make([]map[string]interface{}, len(s.whereConditions)), + orConditions: make([]map[string]interface{}, len(s.orConditions)), + notConditions: make([]map[string]interface{}, len(s.notConditions)), + havingConditions: make([]map[string]interface{}, len(s.havingConditions)), + joinConditions: make([]map[string]interface{}, len(s.joinConditions)), + initAttrs: make([]interface{}, len(s.initAttrs)), + assignAttrs: make([]interface{}, len(s.assignAttrs)), + selects: s.selects, + omits: make([]string, len(s.omits)), + orders: make([]interface{}, len(s.orders)), + preload: make([]searchPreload, len(s.preload)), + offset: s.offset, + limit: s.limit, + group: s.group, + tableName: s.tableName, + raw: s.raw, + Unscoped: s.Unscoped, + ignoreOrderQuery: s.ignoreOrderQuery, + } + for i, value := range s.whereConditions { + clone.whereConditions[i] = value + } + for i, value := range s.orConditions { + clone.orConditions[i] = value + } + for i, value := range s.notConditions { + clone.notConditions[i] = value + } + for i, value := range s.havingConditions { + clone.havingConditions[i] = value + } + for i, value := range s.joinConditions { + clone.joinConditions[i] = value + } + for i, value := range s.initAttrs { + clone.initAttrs[i] = value + } + for i, value := range s.assignAttrs { + clone.assignAttrs[i] = value + } + for i, value := range s.omits { + clone.omits[i] = value + } + for i, value := range s.orders { + clone.orders[i] = value + } + for i, value := range s.preload { + clone.preload[i] = value + } return &clone } diff --git a/search_test.go b/search_test.go index 4db7ab6a..bf8e84fb 100644 --- a/search_test.go +++ b/search_test.go @@ -1,6 +1,7 @@ package gorm import ( + "fmt" "reflect" "testing" ) @@ -28,3 +29,24 @@ func TestCloneSearch(t *testing.T) { t.Errorf("selectStr should be copied") } } + +func TestWhereCloneCorruption(t *testing.T) { + for whereCount := 1; whereCount <= 8; whereCount++ { + t.Run(fmt.Sprintf("w=%d", whereCount), func(t *testing.T) { + s := new(search) + for w := 0; w < whereCount; w++ { + s = s.clone().Where(fmt.Sprintf("w%d = ?", w), fmt.Sprintf("value%d", w)) + } + if len(s.whereConditions) != whereCount { + t.Errorf("s: where count should be %d", whereCount) + } + + q1 := s.clone().Where("finalThing = ?", "THING1") + q2 := s.clone().Where("finalThing = ?", "THING2") + + if reflect.DeepEqual(q1.whereConditions, q2.whereConditions) { + t.Errorf("Where conditions should be different") + } + }) + } +}