Faceted Search: fix infinite recursion on duplicate facets

[nl0]

• Unit tests
• Changelog entry (skip if change is not significant to end users, e.g. docs only)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3773477) 34.71% compared to head (098bb5d) 89.15%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3799       +/-   ##
===========================================
+ Coverage   34.71%   89.15%   +54.43%     
===========================================
  Files         709       79      -630     
  Lines       31063     9899    -21164     
  Branches     4638        0     -4638     
===========================================
- Hits        10783     8825     -1958     
+ Misses      19131     1074    -18057     
+ Partials     1149        0     -1149     

Flag	Coverage Δ
api-python	91.22% <ø> (ø)
catalog	?
lambda	86.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fiskus]

What is a source of this conflict? Can we make it so it doesn't happen?

---

Few suggestions to make code more readable for me, if you care:

• don't use abbreviations when not necessary: KTree → KeyedTree, it requires additional energy to keep in mind what K means
• const children = new Map(a.children) → const aChildren = new Map(a.children), to not confuse with b.children
• resolveFacetConflict → mergeDuplicateFacets. Resolve - is a function under-the-hood, but merge duplicates is what function's intention

[nl0]

@fiskus

What is a source of this conflict? Can we make it so it doesn't happen?

not sure tbh. like, it shouldn't be happening, but for some reason it does -- i wasn't able to determine the root cause on the back-end yet. anyways, it's better to not crash on it, just record the fact and continue normal operation.

const children = new Map(a.children) → const aChildren = new Map(a.children), to not confuse with b.children

it's not a's children, it's resulting children, which is initialized with the value of a, and then merged with b, so it will be incorrect to label it aChildren

resolveFacetConflict → mergeDuplicateFacets. Resolve - is a function under-the-hood, but merge duplicates is what function's intention

that's not quite true, because 1) this function is called "resolve" in Tree's api -- it's used to resolve conflicts (entries with conflicting keys) while merging the trees, and 2) it's purpose is not to merge duplicate facets (this is only an edge case that shouldn't be happening at all), it is to rebuild the [part of the] tree representing the hierarchy of facets to expose differently-typed facets with the same path.