From 129819c0d2a582ade9eee0dc484332d01048074e Mon Sep 17 00:00:00 2001 From: moshaad7 Date: Wed, 20 Dec 2023 21:06:28 +0530 Subject: [PATCH] MB-60207 fix (*FacetResult)Merge + Customer Marshal logic of FacetResult can lead to FacetResult.Terms being nil. + (*FacetResult)Merge now handle the above^ case. + added similar nil case handler for NumericRanges and DateRanges --- search/facets_builder.go | 18 ++- search/facets_builder_test.go | 276 ++++++++++++++++++++++------------ 2 files changed, 194 insertions(+), 100 deletions(-) diff --git a/search/facets_builder.go b/search/facets_builder.go index ebe785c02..4b1f2db78 100644 --- a/search/facets_builder.go +++ b/search/facets_builder.go @@ -321,17 +321,29 @@ func (fr *FacetResult) Merge(other *FacetResult) { fr.Total += other.Total fr.Missing += other.Missing fr.Other += other.Other - if fr.Terms != nil && other.Terms != nil { + if other.Terms != nil { + if fr.Terms == nil { + fr.Terms = other.Terms + return + } for _, term := range other.Terms.termFacets { fr.Terms.Add(term) } } - if fr.NumericRanges != nil && other.NumericRanges != nil { + if other.NumericRanges != nil { + if fr.NumericRanges == nil { + fr.NumericRanges = other.NumericRanges + return + } for _, nr := range other.NumericRanges { fr.NumericRanges = fr.NumericRanges.Add(nr) } } - if fr.DateRanges != nil && other.DateRanges != nil { + if other.DateRanges != nil { + if fr.DateRanges == nil { + fr.DateRanges = other.DateRanges + return + } for _, dr := range other.DateRanges { fr.DateRanges = fr.DateRanges.Add(dr) } diff --git a/search/facets_builder_test.go b/search/facets_builder_test.go index b54d87fa5..6815b1ab9 100644 --- a/search/facets_builder_test.go +++ b/search/facets_builder_test.go @@ -15,122 +15,204 @@ package search import ( + "fmt" "reflect" "testing" ) func TestTermFacetResultsMerge(t *testing.T) { + type testCase struct { + // Input + frs1 FacetResults // first facet results + frs2 FacetResults // second facet results (to be merged into first) + fixups map[string]int // {facetName:size} (to be applied after merge) - fr1TypeTerms := &TermFacets{} - fr1TypeTerms.Add( - &TermFacet{ - Term: "blog", - Count: 25, - }, - &TermFacet{ - Term: "comment", - Count: 24, - }, - &TermFacet{ - Term: "feedback", - Count: 1, - }, - ) - fr1 := &FacetResult{ - Field: "type", - Total: 100, - Missing: 25, - Other: 25, - Terms: fr1TypeTerms, + // Expected output + expFrs FacetResults // facet results after merge and fixup } - fr1CategoryTerms := &TermFacets{} - fr1CategoryTerms.Add( - &TermFacet{ - Term: "clothing", - Count: 35, - }, - &TermFacet{ - Term: "electronics", - Count: 25, - }, - ) + tests := []*testCase{ + func() *testCase { + rv := &testCase{} - fr1Only := &FacetResult{ - Field: "category", - Total: 97, - Missing: 22, - Other: 15, - Terms: fr1CategoryTerms, - } - frs1 := FacetResults{ - "types": fr1, - "categories": fr1Only, - } + rv.frs1 = FacetResults{ + "types": &FacetResult{ + Field: "type", + Total: 100, + Missing: 25, + Other: 25, + Terms: func() *TermFacets { + tfs := &TermFacets{} + tfs.Add( + &TermFacet{ + Term: "blog", + Count: 25, + }, + &TermFacet{ + Term: "comment", + Count: 24, + }, + &TermFacet{ + Term: "feedback", + Count: 1, + }, + ) + return tfs + }(), + }, + "categories": &FacetResult{ + Field: "category", + Total: 97, + Missing: 22, + Other: 15, + Terms: func() *TermFacets { + tfs := &TermFacets{} + tfs.Add( + &TermFacet{ + Term: "clothing", + Count: 35, + }, + &TermFacet{ + Term: "electronics", + Count: 25, + }, + ) + return tfs + }(), + }, + } + rv.frs2 = FacetResults{ + "types": &FacetResult{ + Field: "type", + Total: 100, + Missing: 25, + Other: 25, + Terms: func() *TermFacets { + tfs := &TermFacets{} + tfs.Add( + &TermFacet{ + Term: "blog", + Count: 25, + }, + &TermFacet{ + Term: "comment", + Count: 22, + }, + &TermFacet{ + Term: "flag", + Count: 3, + }, + ) + return tfs + }(), + }, + } + rv.fixups = map[string]int{ + "types": 3, // we want top 3 terms based on count + } - fr2TypeTerms := &TermFacets{} - fr2TypeTerms.Add( - &TermFacet{ - Term: "blog", - Count: 25, - }, - &TermFacet{ - Term: "comment", - Count: 22, - }, - &TermFacet{ - Term: "flag", - Count: 3, - }, - ) + rv.expFrs = FacetResults{ + "types": &FacetResult{ + Field: "type", + Total: 200, + Missing: 50, + Other: 51, + Terms: &TermFacets{ + termFacets: []*TermFacet{ + { + Term: "blog", + Count: 50, + }, + { + Term: "comment", + Count: 46, + }, + { + Term: "flag", + Count: 3, + }, + }, + }, + }, + "categories": rv.frs1["categories"], + } - fr2 := &FacetResult{ - Field: "type", - Total: 100, - Missing: 25, - Other: 25, - Terms: fr2TypeTerms, - } - frs2 := FacetResults{ - "types": fr2, - } + return rv + }(), + func() *testCase { + rv := &testCase{} - expectedFr := &FacetResult{ - Field: "type", - Total: 200, - Missing: 50, - Other: 51, - Terms: &TermFacets{ - termFacets: []*TermFacet{ - { - Term: "blog", - Count: 50, + rv.frs1 = FacetResults{ + "facetName": &FacetResult{ + Field: "docField", + Total: 0, + Missing: 0, + Other: 0, + Terms: nil, }, - { - Term: "comment", - Count: 46, + } + rv.frs2 = FacetResults{ + "facetName": &FacetResult{ + Field: "docField", + Total: 3, + Missing: 0, + Other: 0, + Terms: &TermFacets{ + termFacets: []*TermFacet{ + { + Term: "firstTerm", + Count: 1, + }, + { + Term: "secondTerm", + Count: 2, + }, + }, + }, }, - { - Term: "flag", - Count: 3, + } + rv.fixups = map[string]int{ + "facetName": 1, + } + + rv.expFrs = FacetResults{ + "facetName": &FacetResult{ + Field: "docField", + Total: 3, + Missing: 0, + Other: 1, + Terms: &TermFacets{ + termFacets: []*TermFacet{ + { + Term: "secondTerm", + Count: 2, + }, + }, + }, }, - }, - }, - } - expectedFrs := FacetResults{ - "types": expectedFr, - "categories": fr1Only, + } + return rv + }(), } - frs1.Merge(frs2) - frs1.Fixup("types", 3) + for tcIdx, tc := range tests { + t.Run(fmt.Sprintf("T#%d", tcIdx), func(t *testing.T) { + tc.frs1.Merge(tc.frs2) + for facetName, size := range tc.fixups { + tc.frs1.Fixup(facetName, size) + } - for _, v := range frs1 { - v.Terms.termLookup = nil - } + // clear termLookup, so we can compare the facet results + for _, fr := range tc.frs1 { + if fr.Terms != nil { + fr.Terms.termLookup = nil + } + } - if !reflect.DeepEqual(frs1, expectedFrs) { - t.Errorf("expected %v, got %v", expectedFrs, frs1) + if !reflect.DeepEqual(tc.frs1, tc.expFrs) { + t.Errorf("expected %v, got %v", tc.expFrs, tc.frs1) + } + }) } }