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.

Sign up for GitHub

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

Jump to bottom

Fix keepZeroFacets on disjunctive facet search #918

Merged
merged 3 commits into from
Dec 15, 2022
Merged

Fix keepZeroFacets on disjunctive facet search #918

merged 3 commits into from
Dec 15, 2022

Conversation

bidoubiwa
Copy link
Contributor

@bidoubiwa bidoubiwa commented Dec 12, 2022 •
edited
Loading

Since introducing the multi-index search here #888 the setting keepZeroFacets was broken.

This PR introduces the fix to this issue

Dec-13-2022.17-12-15.mp4

What was done in this pr:

  • keepZeroFacet management has been moved to the response adapter instead of the request adapter
  • Filtering methods and filtering tests were removed as the new facet distribution management has completely changed
  • defaultFacetDistribution is renamed initialFacetDistribution and contains the distribution of an index without any other requests
  • Initialization of facet distribution was moved from client to its own file

Playgrounds:

  • New SingleMovieIndex playground to create a new environment for testing
  • Updated SingleIndex with keepZeroFacets
  • Updated MultiIndex with keepZeroFacets

bidoubiwa
bidoubiwa commented Dec 12, 2022
View reviewed changes
src/adapter/search-request-adapter/search-resolver.ts
Comment on lines -45 to -54
if (searchContext.keepZeroFacets) {
const cachedFacets = extractFacets(searchContext, searchParams)

// Add missing facets back into facetDistribution
searchResponse.facetDistribution = addMissingFacets(
cachedFacets,
searchResponse.facetDistribution
)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved as it should be in the response adapter

@bidoubiwa bidoubiwa force-pushed the fix_keep_zero_facets branch from 290371a to a4a845a Compare December 13, 2022 16:11
bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/client/instant-meilisearch-client.ts
Comment on lines -86 to -95
// Cache first facets distribution of the instantMeilisearch instance
// Needed to add in the facetDistribution the fields that were not returned
// When the user sets `keepZeroFacets` to true.
if (defaultFacetDistribution === undefined) {
defaultFacetDistribution = await cacheFirstFacetDistribution(
searchResolver,
searchContext
)
searchContext.defaultFacetDistribution = defaultFacetDistribution
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed to its own file init-facet-distribution.ts

bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/client/instant-meilisearch-client.ts Outdated
let defaultFacetDistribution: FacetDistribution
let initialFacetDistribution: Record<string, FacetDistribution>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed name to better understand what it contains

bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/adapter/search-response-adapter/search-response-adapter.ts
Comment on lines -48 to +64
return {
results: [adaptedSearchResponse],
}
return adaptedSearchResponse
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return value was changed as this is legacy that made the process weird

bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/client/instant-meilisearch-client.ts
searchContext,
initialFacetDistribution[searchRequest.indexName]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed initialFacetDistribution from the searchContext as it is not correlated to the current search request but as value for the whole instance

bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/contexts/search-context.ts
@@ -33,7 +31,6 @@ export function createSearchContext(
sort: sortByArray.join(':') || '',
indexUid,
pagination: paginationState,
defaultFacetDistribution: defaultFacetDistribution || {},
Copy link
Contributor Author

@bidoubiwa bidoubiwa Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed defaultFacetDistribution from the searchContext as it is not correlated to the current search request but as value for the whole instance.

bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/types/types.ts
export type SearchContext = Omit<InstantSearchParams, 'insideBoundingBox'> &
InstantSearchParams & {
defaultFacetDistribution: FacetDistribution
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed defaultFacetDistribution from the searchContext as it is not correlated to the current search request but as value for the whole instance.

bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/types/types.ts
@@ -73,9 +70,10 @@ export type InstantSearchPagination = {
nbPages: number
}

export type Facets = string | string[] | undefined
Copy link
Contributor Author

@bidoubiwa bidoubiwa Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creation of own Facets type as the one from instant-search does not represent the reality.

instant-search may provide only a string as a Facets but the type states that it only accepts string[] | undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this issue

@bidoubiwa bidoubiwa force-pushed the fix_keep_zero_facets branch from cf5ffe0 to 0d1b9ea Compare December 13, 2022 17:47
bidoubiwa
bidoubiwa commented Dec 13, 2022
View reviewed changes
src/adapter/search-request-adapter/search-resolver.ts
@@ -5,7 +5,6 @@ import {
SearchCacheInterface,
MeiliSearchParams,
} from '../../types'
import { addMissingFacets, extractFacets } from './filters'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed as facet managements has changed and moved to the response adapter

@bidoubiwa bidoubiwa force-pushed the fix_keep_zero_facets branch 3 times, most recently from 63cc33a to 8f1e01c Compare December 13, 2022 19:00
@bidoubiwa bidoubiwa marked this pull request as ready for review December 14, 2022 15:13
@bidoubiwa bidoubiwa force-pushed the fix_keep_zero_facets branch from 8f1e01c to 78d8870 Compare December 14, 2022 15:13
brunoocasali
brunoocasali approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments that can be done in a new PR maybe, I don't want to block the merge :)

LGTM!

src/adapter/search-response-adapter/facet-distribution-adapter.ts Outdated Show resolved Hide resolved
src/adapter/search-response-adapter/facet-distribution-adapter.ts Show resolved Hide resolved
@bidoubiwa bidoubiwa force-pushed the fix_keep_zero_facets branch from 2927583 to 40a13a7 Compare December 15, 2022 11:55
@bidoubiwa
Copy link
Contributor Author

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Dec 15, 2022

Build succeeded:

@meili-bors meili-bors bot merged commit f836516 into beta/multi-index-search Dec 15, 2022
@meili-bors meili-bors bot deleted the fix_keep_zero_facets branch December 15, 2022 12:00
@bidoubiwa bidoubiwa mentioned this pull request Dec 15, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunoocasali brunoocasali brunoocasali approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bidoubiwa @brunoocasali