From 2c862acce1231fb9dc98f947200d4b81658ebea5 Mon Sep 17 00:00:00 2001 From: Keenan Nemetz Date: Wed, 27 Sep 2023 11:18:32 -0700 Subject: [PATCH 1/2] fix(i): Flaky normalize filter test (#1912) ## Relevant issue(s) Resolves #1879 ## Description This PR attempts to fix a flaky filter test. The normalization logic has been refactored to a recursive / functional approach that (hopefully) makes it a little easier to understand. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? Ran normalize tests in a loop for 200k+ iterations. Specify the platform(s) on which this was tested: - MacOS --- planner/filter/normalize.go | 275 +++++++++++++++++++++++------------- 1 file changed, 173 insertions(+), 102 deletions(-) diff --git a/planner/filter/normalize.go b/planner/filter/normalize.go index 5f7d275418..181b1f8485 100644 --- a/planner/filter/normalize.go +++ b/planner/filter/normalize.go @@ -16,134 +16,205 @@ import ( ) // normalize normalizes the provided filter conditions. +// // The following cases are subject of normalization: // - _and or _or with one element is removed flattened // - double _not is removed // - any number of consecutive _ands with any number of elements is flattened +// // As the result object is a map with unique keys (a.k.a. properties), // while performing flattening of compound operators if the same property // is present in the result map, both conditions will be moved into an _and func normalize(conditions map[connor.FilterKey]any) map[connor.FilterKey]any { - return normalizeConditions(conditions, false).(map[connor.FilterKey]any) + return normalizeCondition(nil, conditions).(map[connor.FilterKey]any) } -func conditionsArrToMap(conditions []any) map[connor.FilterKey]any { +// normalizeCondition returns a normalized version of the given condition. +func normalizeCondition(parentKey connor.FilterKey, condition any) (result any) { + switch t := condition.(type) { + case map[connor.FilterKey]any: + result = normalizeConditions(parentKey, t) + + case []any: + conditions := make([]any, len(t)) + for i, c := range t { + conditions[i] = normalizeCondition(parentKey, c) + } + result = conditions + + default: + result = t + } + + return normalizeProperty(parentKey, result) +} + +// normalizeConditions returns a normalized version of the given conditions. +func normalizeConditions(parentKey connor.FilterKey, conditions map[connor.FilterKey]any) map[connor.FilterKey]any { result := make(map[connor.FilterKey]any) - for _, clause := range conditions { - if clauseMap, ok := clause.(map[connor.FilterKey]any); ok { - for k, v := range clauseMap { - result[k] = v + for key, val := range conditions { + result[key] = normalizeCondition(key, val) + + // check if the condition is an operator that can be normalized + op, ok := key.(*mapper.Operator) + if !ok { + continue + } + // check if we have any conditions that can be merged + merge := normalizeOperator(parentKey, op, result[key]) + if len(merge) == 0 { + continue + } + delete(result, key) + + // merge properties directly into result + for _, c := range merge { + for key, val := range c.(map[connor.FilterKey]any) { + result[key] = val } } + + // if the merged filter was an _or operator + // there may be child filters that can be merged + if op.Operation == request.FilterOpOr { + result = normalizeConditions(parentKey, result) + } } return result } -func addNormalizedCondition(key connor.FilterKey, val any, m map[connor.FilterKey]any) { - if _, isProp := key.(*mapper.PropertyIndex); isProp { - var andOp *mapper.Operator - var andContent []any - for existingKey := range m { - if op, isOp := existingKey.(*mapper.Operator); isOp && op.Operation == request.FilterOpAnd { - andOp = op - andContent = m[existingKey].([]any) - break - } - } - for existingKey := range m { - if existingKey.Equal(key) { - existingVal := m[existingKey] - delete(m, existingKey) - if andOp == nil { - andOp = &mapper.Operator{Operation: request.FilterOpAnd} - } - m[andOp] = append( - andContent, - map[connor.FilterKey]any{existingKey: existingVal}, - map[connor.FilterKey]any{key: val}, - ) - return - } +// normalizeOperator returns a normalized array of conditions. +func normalizeOperator(parentKey connor.FilterKey, op *mapper.Operator, condition any) []any { + switch op.Operation { + case request.FilterOpNot: + return normalizeOperatorNot(condition) + + case request.FilterOpOr: + return normalizeOperatorOr(condition) + + case request.FilterOpAnd: + return normalizeOperatorAnd(parentKey, condition) + + default: + return nil + } +} + +// normalizeOperatorAnd returns an array of conditions with all _and operators merged. +// +// If the parent operator is _not or _or, the subconditions will not be merged. +func normalizeOperatorAnd(parentKey connor.FilterKey, condition any) []any { + result := condition.([]any) + // always merge if only 1 property + if len(result) == 1 { + return result + } + // always merge if parent is not an operator + parentOp, ok := parentKey.(*mapper.Operator) + if !ok { + return result + } + // don't merge if parent is a _not or _or operator + if parentOp.Operation == request.FilterOpNot || parentOp.Operation == request.FilterOpOr { + return nil + } + return result +} + +// normalizeOperatorOr returns an array of conditions with all single _or operators merged. +func normalizeOperatorOr(condition any) []any { + result := condition.([]any) + // don't merge if more than 1 property + if len(result) > 1 { + return nil + } + return result +} + +// normalizeOperatorNot returns an array of conditions with all double _not operators merged. +func normalizeOperatorNot(condition any) (result []any) { + subConditions := condition.(map[connor.FilterKey]any) + // don't merge if more than 1 property + if len(subConditions) > 1 { + return nil + } + // find double _not occurances + for subKey, subCondition := range subConditions { + op, ok := subKey.(*mapper.Operator) + if ok && op.Operation == request.FilterOpNot { + result = append(result, subCondition) } - for _, andElement := range andContent { - elementMap := andElement.(map[connor.FilterKey]any) - for andElementKey := range elementMap { - if andElementKey.Equal(key) { - m[andOp] = append(andContent, map[connor.FilterKey]any{key: val}) - return - } + } + return result +} + +// normalizeProperty flattens and groups property filters where possible. +// +// Filters targeting the same property will be grouped into a single _and. +func normalizeProperty(parentKey connor.FilterKey, condition any) any { + switch t := condition.(type) { + case map[connor.FilterKey]any: + results := make(map[connor.FilterKey]any) + for _, c := range normalizeProperties(parentKey, []any{t}) { + for key, val := range c.(map[connor.FilterKey]any) { + results[key] = val } } + return results + + case []any: + return normalizeProperties(parentKey, t) + + default: + return t } - m[key] = val } -func normalizeConditions(conditions any, skipRoot bool) any { - result := make(map[connor.FilterKey]any) - switch typedConditions := conditions.(type) { - case map[connor.FilterKey]any: - for rootKey, rootVal := range typedConditions { - rootOpKey, isRootOp := rootKey.(*mapper.Operator) - if isRootOp { - if rootOpKey.Operation == request.FilterOpAnd || rootOpKey.Operation == request.FilterOpOr { - rootValArr := rootVal.([]any) - if len(rootValArr) == 1 || rootOpKey.Operation == request.FilterOpAnd && !skipRoot { - flat := normalizeConditions(conditionsArrToMap(rootValArr), false) - flatMap := flat.(map[connor.FilterKey]any) - for k, v := range flatMap { - addNormalizedCondition(k, v, result) - } - } else { - resultArr := []any{} - for i := range rootValArr { - norm := normalizeConditions(rootValArr[i], !skipRoot) - normMap, ok := norm.(map[connor.FilterKey]any) - if ok { - for k, v := range normMap { - resultArr = append(resultArr, map[connor.FilterKey]any{k: v}) - } - } else { - resultArr = append(resultArr, norm) - } - } - addNormalizedCondition(rootKey, resultArr, result) - } - } else if rootOpKey.Operation == request.FilterOpNot { - notMap := rootVal.(map[connor.FilterKey]any) - if len(notMap) == 1 { - var k connor.FilterKey - for k = range notMap { - break - } - norm := normalizeConditions(notMap, true).(map[connor.FilterKey]any) - delete(notMap, k) - var v any - for k, v = range norm { - break - } - if opKey, ok := k.(*mapper.Operator); ok && opKey.Operation == request.FilterOpNot { - notNotMap := normalizeConditions(v, false).(map[connor.FilterKey]any) - for notNotKey, notNotVal := range notNotMap { - addNormalizedCondition(notNotKey, notNotVal, result) - } - } else { - notMap[k] = v - addNormalizedCondition(rootOpKey, notMap, result) - } - } else { - addNormalizedCondition(rootKey, rootVal, result) - } - } else { - addNormalizedCondition(rootKey, rootVal, result) - } +// normalizeProperty flattens and groups property filters where possible. +// +// Filters targeting the same property will be grouped into a single _and. +func normalizeProperties(parentKey connor.FilterKey, conditions []any) []any { + var merge []any + var result []any + + // can only merge _and groups if parent is not an _or operator + parentOp, isParentOp := parentKey.(*mapper.Operator) + canMergeAnd := !isParentOp || parentOp.Operation != request.FilterOpOr + + // accumulate properties that can be merged into a single _and + // if canMergeAnd is true, all _and groups will be merged + props := make(map[int][]any) + for _, c := range conditions { + for key, val := range c.(map[connor.FilterKey]any) { + op, ok := key.(*mapper.Operator) + if canMergeAnd && ok && op.Operation == request.FilterOpAnd { + merge = append(merge, val.([]any)...) + } else if prop, ok := key.(*mapper.PropertyIndex); ok { + props[prop.Index] = append(props[prop.Index], map[connor.FilterKey]any{key: val}) } else { - addNormalizedCondition(rootKey, normalizeConditions(rootVal, false), result) + result = append(result, map[connor.FilterKey]any{key: val}) } } + } + + // merge filters with duplicate keys into a single _and + for _, val := range props { + if len(val) == 1 { + // only 1 property so no merge required + result = append(result, val...) + } else { + // multiple properties require merge with _and + merge = append(merge, val...) + } + } + + // nothing to merge + if len(merge) == 0 { return result - case []any: - return conditionsArrToMap(typedConditions) - default: - return conditions } + + // merge into a single _and operator + key := &mapper.Operator{Operation: request.FilterOpAnd} + result = append(result, map[connor.FilterKey]any{key: merge}) + return result } From e952be54fb3d6c24e5f4bf1325ba8381d0bc888b Mon Sep 17 00:00:00 2001 From: AndrewSisley Date: Wed, 27 Sep 2023 18:26:22 -0400 Subject: [PATCH 2/2] fix: Infinite loop when updating one-one relation (#1915) ## Relevant issue(s) Resolves #1914 ## Description Fixes an a bug where an infinite loop may be created when updating a self-referencing one-one relation from the secondary side. --- client/descriptions.go | 16 +- db/collection.go | 2 +- db/collection_update.go | 7 +- planner/type_join.go | 13 +- .../one_to_one/with_self_ref_test.go | 191 ++++++++++++++++++ 5 files changed, 218 insertions(+), 11 deletions(-) create mode 100644 tests/integration/mutation/update/field_kinds/one_to_one/with_self_ref_test.go diff --git a/client/descriptions.go b/client/descriptions.go index 0b44f36b83..4f388fa7d3 100644 --- a/client/descriptions.go +++ b/client/descriptions.go @@ -52,13 +52,15 @@ func (col CollectionDescription) GetFieldByID(id FieldID) (FieldDescription, boo return FieldDescription{}, false } -// GetRelation returns the field that supports the relation of the given name. -func (col CollectionDescription) GetRelation(name string) (FieldDescription, bool) { - if !col.Schema.IsEmpty() { - for _, field := range col.Schema.Fields { - if field.RelationName == name { - return field, true - } +// GetFieldByRelation returns the field that supports the relation of the given name. +func (col CollectionDescription) GetFieldByRelation( + relationName string, + otherCollectionName string, + otherFieldName string, +) (FieldDescription, bool) { + for _, field := range col.Schema.Fields { + if field.RelationName == relationName && !(col.Name == otherCollectionName && otherFieldName == field.Name) { + return field, true } } return FieldDescription{}, false diff --git a/db/collection.go b/db/collection.go index cda5cbf584..f5dacccfb1 100644 --- a/db/collection.go +++ b/db/collection.go @@ -1062,7 +1062,7 @@ func (c *collection) save( if isSecondaryRelationID { primaryId := val.Value().(string) - err = c.patchPrimaryDoc(ctx, txn, relationFieldDescription, primaryKey.DocKey, primaryId) + err = c.patchPrimaryDoc(ctx, txn, c.Name(), relationFieldDescription, primaryKey.DocKey, primaryId) if err != nil { return cid.Undef, err } diff --git a/db/collection_update.go b/db/collection_update.go index 1a15482935..2e353dd0d3 100644 --- a/db/collection_update.go +++ b/db/collection_update.go @@ -350,6 +350,7 @@ func (c *collection) isSecondaryIDField(fieldDesc client.FieldDescription) (clie func (c *collection) patchPrimaryDoc( ctx context.Context, txn datastore.Txn, + secondaryCollectionName string, relationFieldDescription client.FieldDescription, docKey string, fieldValue string, @@ -365,7 +366,11 @@ func (c *collection) patchPrimaryDoc( } primaryCol = primaryCol.WithTxn(txn) - primaryField, ok := primaryCol.Description().GetRelation(relationFieldDescription.RelationName) + primaryField, ok := primaryCol.Description().GetFieldByRelation( + relationFieldDescription.RelationName, + secondaryCollectionName, + relationFieldDescription.Name, + ) if !ok { return client.NewErrFieldNotExist(relationFieldDescription.RelationName) } diff --git a/planner/type_join.go b/planner/type_join.go index f37437089e..ee771b01fc 100644 --- a/planner/type_join.go +++ b/planner/type_join.go @@ -259,7 +259,11 @@ func (p *Planner) makeTypeJoinOne( return nil, err } - subTypeField, subTypeFieldNameFound := subTypeCollectionDesc.GetRelation(subTypeFieldDesc.RelationName) + subTypeField, subTypeFieldNameFound := subTypeCollectionDesc.GetFieldByRelation( + subTypeFieldDesc.RelationName, + parent.sourceInfo.collectionDescription.Name, + subTypeFieldDesc.Name, + ) if !subTypeFieldNameFound { return nil, client.NewErrFieldNotExist(subTypeFieldDesc.RelationName) } @@ -481,7 +485,12 @@ func (p *Planner) makeTypeJoinMany( return nil, err } - rootField, rootNameFound := subTypeCollectionDesc.GetRelation(subTypeFieldDesc.RelationName) + rootField, rootNameFound := subTypeCollectionDesc.GetFieldByRelation( + subTypeFieldDesc.RelationName, + parent.sourceInfo.collectionDescription.Name, + subTypeFieldDesc.Name, + ) + if !rootNameFound { return nil, client.NewErrFieldNotExist(subTypeFieldDesc.RelationName) } diff --git a/tests/integration/mutation/update/field_kinds/one_to_one/with_self_ref_test.go b/tests/integration/mutation/update/field_kinds/one_to_one/with_self_ref_test.go new file mode 100644 index 0000000000..16225f4ab3 --- /dev/null +++ b/tests/integration/mutation/update/field_kinds/one_to_one/with_self_ref_test.go @@ -0,0 +1,191 @@ +// Copyright 2023 Democratized Data Foundation +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package one_to_one + +import ( + "fmt" + "testing" + + testUtils "github.com/sourcenetwork/defradb/tests/integration" +) + +func TestMutationUpdateOneToOne_SelfReferencingFromPrimary(t *testing.T) { + user1ID := "bae-decf6467-4c7c-50d7-b09d-0a7097ef6bad" + + test := testUtils.TestCase{ + Description: "One to one update mutation, self referencing from primary", + Actions: []any{ + testUtils.SchemaUpdate{ + Schema: ` + type User { + name: String + boss: User @primary + underling: User + } + `, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "John" + }`, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "Fred" + }`, + }, + testUtils.UpdateDoc{ + DocID: 1, + Doc: fmt.Sprintf( + `{ + "boss_id": "%s" + }`, + user1ID, + ), + }, + testUtils.Request{ + Request: ` + query { + User { + name + boss { + name + } + } + }`, + Results: []map[string]any{ + { + "name": "Fred", + "boss": map[string]any{ + "name": "John", + }, + }, + { + "name": "John", + "boss": nil, + }, + }, + }, + testUtils.Request{ + Request: ` + query { + User { + name + underling { + name + } + } + }`, + Results: []map[string]any{ + { + "name": "Fred", + "underling": nil, + }, + { + "name": "John", + "underling": map[string]any{ + "name": "Fred", + }, + }, + }, + }, + }, + } + + testUtils.ExecuteTestCase(t, test) +} + +func TestMutationUpdateOneToOne_SelfReferencingFromSecondary(t *testing.T) { + user1ID := "bae-decf6467-4c7c-50d7-b09d-0a7097ef6bad" + + test := testUtils.TestCase{ + Description: "One to one update mutation, self referencing from secondary", + + Actions: []any{ + testUtils.SchemaUpdate{ + Schema: ` + type User { + name: String + boss: User + underling: User @primary + } + `, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "John" + }`, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "Fred" + }`, + }, + testUtils.UpdateDoc{ + DocID: 1, + Doc: fmt.Sprintf( + `{ + "boss_id": "%s" + }`, + user1ID, + ), + }, + testUtils.Request{ + Request: ` + query { + User { + name + boss { + name + } + } + }`, + Results: []map[string]any{ + { + "name": "Fred", + "boss": map[string]any{ + "name": "John", + }, + }, + { + "name": "John", + "boss": nil, + }, + }, + }, + testUtils.Request{ + Request: ` + query { + User { + name + underling { + name + } + } + }`, + Results: []map[string]any{ + { + "name": "Fred", + "underling": nil, + }, + { + "name": "John", + "underling": map[string]any{ + "name": "Fred", + }, + }, + }, + }, + }, + } + + testUtils.ExecuteTestCase(t, test) +}