Skip to content

Commit

Permalink
Add context to mergeMaps function (#66)
Browse files Browse the repository at this point in the history
It's very needed, as it's one of the most fragile parts of aggregation
handling.
When something goes wrong (e.g. for me during testing), it's very often
there, and so far we didn't catch any errors in UI because of lack of
context.
  • Loading branch information
trzysiek authored May 9, 2024
1 parent e354b8f commit 62955a2
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 6 deletions.
2 changes: 1 addition & 1 deletion quesma/queryparser/query_translator.go
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ func (cw *ClickhouseQueryTranslator) MakeAggregationPartOfResponse(queries []mod
}
aggregation := cw.makeResponseAggregationRecursive(query, ResultSets[i+1], 0, 0)
if len(aggregation) != 0 {
aggregations = util.MergeMaps(aggregations, aggregation[0]) // result of root node is always a single map, thus [0]
aggregations = util.MergeMaps(cw.Ctx, aggregations, aggregation[0]) // result of root node is always a single map, thus [0]
}
}
return aggregations
Expand Down
9 changes: 5 additions & 4 deletions quesma/util/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package util

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -255,15 +256,15 @@ func JsonDifference(jsonActual, jsonExpected string) (JsonMap, JsonMap, error) {
// but none of them works for nested maps, so needed to write our own.
// * mActual - uses JsonMap fully: values are []JsonMap, or JsonMap, or base types
// * mExpected - value can also be []any, because it's generated from Golang's json.Unmarshal
func MergeMaps(mActual, mExpected JsonMap) JsonMap {
func MergeMaps(ctx context.Context, mActual, mExpected JsonMap) JsonMap {
var mergeMapsRec func(m1, m2 JsonMap) JsonMap
// merges 'i1' and 'i2' in 3 cases: both are JsonMap, both are []JsonMap, or both are some base type
mergeAny := func(i1, i2 any) any {
switch i1Typed := i1.(type) {
case JsonMap:
i2Typed, ok := i2.(JsonMap)
if !ok {
logger.Error().Msgf("mergeAny: i1 is map, i2 is not. i1: %v, i2: %v", i1, i2)
logger.ErrorWithCtx(ctx).Msgf("mergeAny: i1 is map, i2 is not. i1: %v, i2: %v", i1, i2)
return i1
}
return mergeMapsRec(i1Typed, i2Typed)
Expand All @@ -277,13 +278,13 @@ func MergeMaps(mActual, mExpected JsonMap) JsonMap {
i2Typed = append(i2Typed, val)
}
} else {
logger.Error().Msgf("mergeAny: i1 is []JsonMap, i2 is not an array. i1: %v, i2: %v", i1Typed, i2)
logger.ErrorWithCtx(ctx).Msgf("mergeAny: i1 is []JsonMap, i2 is not an array. i1: %v, i2: %v", i1Typed, i2)
}
}

// lengths should be always equal in our usage of this function, maybe that'll change
if len(i1Typed) != len(i2Typed) {
logger.Error().Msgf("mergeAny: i1 and i2 are slices, but have different lengths. i1: %v, i2: %v", i1, i2)
logger.ErrorWithCtx(ctx).Msgf("mergeAny: i1 and i2 are slices, but have different lengths. len(i1): %v, len(i2): %v, i1: %v, i2: %v", len(i1Typed), len(i2Typed), i1, i2)
return []JsonMap{}
}
mergedArray := make([]JsonMap, len(i1Typed))
Expand Down
3 changes: 2 additions & 1 deletion quesma/util/utils_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package util

import (
"context"
"encoding/json"
"github.com/stretchr/testify/assert"
"reflect"
Expand Down Expand Up @@ -602,7 +603,7 @@ func TestMergeMaps(t *testing.T) {
for i, tt := range cases {
t.Run("TestMergeMaps_"+strconv.Itoa(i), func(t *testing.T) {
// simple == or Equal doesn't work on nested maps => need DeepEqual
assert.True(t, reflect.DeepEqual(tt.wanted, MergeMaps(tt.m1, tt.m2)))
assert.True(t, reflect.DeepEqual(tt.wanted, MergeMaps(context.Background(), tt.m1, tt.m2)))
})
}
}
Expand Down

0 comments on commit 62955a2

Please sign in to comment.