Skip to content

Commit

Permalink
PR: Fix - Update the previous implementation to avoid the use of cloning
Browse files Browse the repository at this point in the history
a dereferenced version of the filter conditions and also get rid of
reflect.DeepEqual() comparison.

- Adhere to code review.
  • Loading branch information
shahzadlone committed Sep 15, 2022
1 parent c27f783 commit 2371b9a
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 45 deletions.
2 changes: 2 additions & 0 deletions connor/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ type FilterKey interface {
// GetOperatorOrDefault returns either the operator that corresponds
// to this key, or the given default.
GetOperatorOrDefault(defaultOp string) string
// Equal returns true if other is equal, otherwise returns false.
Equal(other FilterKey) bool
}
76 changes: 35 additions & 41 deletions query/graphql/mapper/mapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -965,58 +965,52 @@ func (f *Filter) equal(other *Filter) bool {
return f == nil
}

if len(f.Conditions) != len(other.Conditions) {
return deepEqualConditions(f.Conditions, other.Conditions)
}

// deepEqualConditions performs a deep equality of two conditions.
// Handles: map[0xc00069cfd0:map[0xc005eda8c0:<nil>]] -> map[{5}:map[{_ne}:<nil>]]
func deepEqualConditions(
this map[connor.FilterKey]any,
other map[connor.FilterKey]any,
) bool {
if len(this) != len(other) {
return false
}

// Before calling DeepEqual we need to make sure the keys aren't pointers,
// if they are pointers then dereference them for DeepEqual to work properly.
// Interface pointers don't get deferenced automatically and we should
// probably rework the filter keys to not be stored as pointers.
//
// For example if we have this, (DeepEqual will return false):
// f.Conditions : map[0xc00069cfd0:map[0xc005eda8c0:<nil>]]
// other.Conditions : map[0xc00069cfe8:map[0xc005edaa80:<nil>]]
//
// DeepEqual works if we convert the above to it's dereferenced type:
// new-f.Conditions : map[{5}:map[{_ne}:<nil>]]
// new-other.Conditions : map[{5}:map[{_ne}:<nil>]]
thisConditions := derefConditionsKeys(f.Conditions)
otherConditions := derefConditionsKeys(other.Conditions)

return reflect.DeepEqual(thisConditions, otherConditions)
}

// derefConditionsKeys dereferences underlying pointer types of interface which
// are keys in a map. If the value of the map is also a map with pointer keys,
// it will recursively handle them.
//
// This is usefull when we have : map[0xc00069cfd0:map[0xc005eda8c0:<nil>]]
// And want to dereference it rescursively to : map[{5}:map[{_ne}:<nil>]]
func derefConditionsKeys(conditions map[connor.FilterKey]any) map[connor.FilterKey]any {
resultConditions := make(map[connor.FilterKey]any)
// using the name Address because these are possibly pointers.
for keyAddress, valueAddress := range this {
var isFoundInOther bool

for keyAddress, valueAddress := range conditions {
var defrefKey connor.FilterKey
// Ensure a matching key exists in the other map.
for otherKeyAddress, otherValueAddress := range other {
if !keyAddress.Equal(otherKeyAddress) {
continue
}

switch key := keyAddress.(type) {
case *Operator:
defrefKey = *key
valueConditions, thisOk := valueAddress.(map[connor.FilterKey]any)
otherValueConditions, otherOk := otherValueAddress.(map[connor.FilterKey]any)
if thisOk && otherOk {
if isFoundInOther = deepEqualConditions(valueConditions, otherValueConditions); isFoundInOther {
break
}
} else if !thisOk && !otherOk {
// Both are some basic values.
if isFoundInOther = reflect.DeepEqual(valueAddress, otherValueAddress); isFoundInOther {
break
}
}

case *PropertyIndex:
defrefKey = *key
// Only reaches here when only one of them was 'casted' (isn't a matching key) so go check next one.
}

switch value := valueAddress.(type) {
case map[connor.FilterKey]any:
resultConditions[defrefKey] = derefConditionsKeys(value)

default:
resultConditions[defrefKey] = value
// No matching key (including matching data, of the pointer keys) found, so exit early.
if !isFoundInOther {
return false
}
}

return resultConditions
return true
}

// aggregateRequest is an intermediary struct defining a consumer-requested
Expand Down
22 changes: 18 additions & 4 deletions query/graphql/mapper/targetable.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,25 @@ type PropertyIndex struct {
Index int
}

func (k PropertyIndex) GetProp(data interface{}) interface{} {
func (k *PropertyIndex) GetProp(data interface{}) interface{} {
if data == nil {
return nil
}

return data.(core.Doc).Fields[k.Index]
}

func (k PropertyIndex) GetOperatorOrDefault(defaultOp string) string {
func (k *PropertyIndex) GetOperatorOrDefault(defaultOp string) string {
return defaultOp
}

func (k *PropertyIndex) Equal(other connor.FilterKey) bool {
if otherKey, isOk := other.(*PropertyIndex); isOk && *k == *otherKey {
return true
}
return false
}

// Operator is a FilterKey that represents a filter operator.
type Operator struct {
// The filter operation string that this Operator represents.
Expand All @@ -46,14 +53,21 @@ type Operator struct {
Operation string
}

func (k Operator) GetProp(data interface{}) interface{} {
func (k *Operator) GetProp(data interface{}) interface{} {
return data
}

func (k Operator) GetOperatorOrDefault(defaultOp string) string {
func (k *Operator) GetOperatorOrDefault(defaultOp string) string {
return k.Operation
}

func (k *Operator) Equal(other connor.FilterKey) bool {
if otherKey, isOk := other.(*Operator); isOk && *k == *otherKey {
return true
}
return false
}

// Filter represents a series of conditions that may reduce the number of
// records that a query returns.
type Filter struct {
Expand Down

0 comments on commit 2371b9a

Please sign in to comment.