Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faceted Search: fix infinite recursion on duplicate facets #3799

Merged
merged 4 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions catalog/app/containers/Search/model.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import * as KTree from 'utils/KeyedTree'

import * as model from './model'

describe('containers/Search/model', () => {
describe('groupFacets', () => {
it('should group the facets without exceeding recursion limit', () => {
const f1: model.PackageUserMetaFacet = {
__typename: 'KeywordPackageUserMetaFacet',
path: '/a/b',
extents: {
__typename: 'KeywordExtents',
values: ['first'],
},
}
const f2: model.PackageUserMetaFacet = {
__typename: 'KeywordPackageUserMetaFacet',
path: '/a/b',
extents: {
__typename: 'KeywordExtents',
values: ['second'],
},
}
const facets = [f1, f2]
const [grouped] = model.groupFacets(facets)
expect(grouped).toEqual(
KTree.Tree([
KTree.Pair('path:a', KTree.Tree([KTree.Pair('path:b', KTree.Leaf(f1))])),
]),
)
})
})
})
20 changes: 20 additions & 0 deletions catalog/app/containers/Search/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as dateFns from 'date-fns'
import * as R from 'ramda'
import * as React from 'react'
import * as RR from 'react-router-dom'
import * as Sentry from '@sentry/react'

import * as Model from 'model'
import * as GQL from 'utils/GraphQL'
Expand Down Expand Up @@ -780,7 +781,26 @@ function normalizeFacetNode(node: FacetNode): FacetTree {
)
}

const facetId = (f: PackageUserMetaFacet) => `${f.path}:${f.__typename}`

function resolveFacetConflict(existing: FacetNode, conflict: FacetNode): FacetNode {
if (existing._tag === 'Leaf' && conflict._tag === 'Leaf') {
const existingId = facetId(existing.value)
const conflictId = facetId(conflict.value)
// duplicate facet, should not happen
// this would cause an infinite recursion if not handled
if (existingId === conflictId) {
Sentry.withScope((scope) => {
const depth = JSONPointer.parse(existing.value.path).length
const type = PackageUserMetaFacetTypeDisplay[existing.value.__typename]
scope.setExtras({ depth, type })
Sentry.captureMessage('Duplicate facet', 'warning')
})
// keep the facet encountered first
return existing
}
}

return KTree.merge(
normalizeFacetNode(existing),
normalizeFacetNode(conflict),
Expand Down
1 change: 1 addition & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Entries inside each section should be ordered by type:
* [Fixed] Fix code sample for package push ([#3499](https://github.com/quiltdata/quilt/pull/3499))
* [Fixed] Make bookmarks optional (and fix Embed listings broken in #3697) ([#3705](https://github.com/quiltdata/quilt/pull/3705))
* [Fixed] Disable opening file picker on metadata click, and fix dropping JSON as metadata ([#3707](https://github.com/quiltdata/quilt/pull/3707))
* [Fixed] Faceted Search: crash due to infinite recursion on duplicate facets ([#3799](https://github.com/quiltdata/quilt/pull/3799))
* [Added] Add filter for users and buckets tables in Admin dashboards ([#3480](https://github.com/quiltdata/quilt/pull/3480))
* [Added] Add links to documentation and re-use code samples ([#3496](https://github.com/quiltdata/quilt/pull/3496))
* [Added] Show S3 Object tags ([#3515](https://github.com/quiltdata/quilt/pull/3515))
Expand Down