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

[elasticsearch-connector] Implement basefilters #768

Merged
merged 12 commits into from
Jun 23, 2022
Merged

[elasticsearch-connector] Implement basefilters #768

merged 12 commits into from
Jun 23, 2022

Conversation

joemcelroy
Copy link
Member

@joemcelroy joemcelroy commented Jun 20, 2022 •
edited
Loading

Fixes #768

Observations:

  • App Search when configured with global filters (for example states: california) and one of the global filters is a disjunctive facet, the "disjunctive facet call" incorrectly removes the global filter, which means the facet is incorrectly showing all possible options, including aggregations to bring back results which do not have the state.
  • app search does allow adding filters that are not facets

Proposal

  • Changes to search-ui

    • No longer remove the filters from queryConfig so the connector can access the filters applied outside of the UI

  • Changes to app-search connector

    • Continue the merging of the queryConfig.filters + requestState.filters and mutate requestState.filters
    • Do not use queryConfig.filters in the onSearch event handler (as the filters are already present in the requestState.filters)

  • Changes to elasticsearch-connector

    • queryConfig.filters are transformed into lucene queries and placed into searchkit as a baseFilter
    • requestState.filters will be pruned where the filters exist in the queryConfig.filters (due to the mergedFilters behaviour in search-ui) and then are added as normal searchkit filters

  • create a ticket to address the App search disjunctive + global filters issue

@joemcelroy joemcelroy marked this pull request as ready for review June 21, 2022 02:52
joemcelroy
joemcelroy commented Jun 21, 2022
View reviewed changes
packages/search-ui-app-search-connector/src/__tests__/AppSearchAPIConnector.test.ts
@@ -386,7 +386,17 @@ describe("AppSearchAPIConnector", () => {
{
all: [
{
date_made: "yesterday"
title: "Acadia"
Copy link
Member Author

@joemcelroy joemcelroy Jun 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This change seems a little odd but i believe its the correct behaviour here. The test case here has two filters in the request state and one filter in the queryConfig. Previously it was only providing the filter in the queryConfig to the API. Now its passing all three filters to the API. I believe it should be passing all three filters to the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it looks like we're missing the date_made: "yesterday" filter from the call. Is this expected?

Copy link
Member Author

@joemcelroy joemcelroy Jun 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the spot! So the testcase is incorrect here, the date_made: "yesterday" should of been present in requestState.filters too, as searchDriver runs mergeFilter which mutates the requestState.filters with filters from the queryConfig.filters

Ive updated the testcase to better reflect the state.

joemcelroy
joemcelroy commented Jun 21, 2022
View reviewed changes
packages/search-ui/src/__tests__/SearchDriver.test.ts
@@ -334,9 +334,9 @@ describe("searchQuery config", () => {
]);
});

it("will remove filters parameter from queryConfig since its merged into state", () => {
it("will not remove filters parameter from queryConfig", () => {
Copy link
Member Author

@joemcelroy joemcelroy Jun 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this test was changed as the queryConfig.filters still remain as its needed by the connectors. See PR description for more detail on the flow.

yakhinvadim
yakhinvadim approved these changes Jun 22, 2022
View reviewed changes
Copy link
Contributor

@yakhinvadim yakhinvadim left a comment

Choose a reason for hiding this comment

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

Working as expected now! Great work, Joe! 👍

@joemcelroy joemcelroy merged commit 38f111e into elastic:master Jun 23, 2022
@joemcelroy joemcelroy deleted the base-filters-es-connector branch June 23, 2022 00:43
@JasonStoltz JasonStoltz mentioned this pull request Jul 18, 2022
Closed
joemcelroy added a commit to joemcelroy/search-ui that referenced this pull request Jan 10, 2023
@joemcelroy
[elasticsearch-connector] Implement basefilters (elastic#768)
0295cfc 
* Implement basefilters

* update test to assert the filters set in config is not removed

* update app search test to assert both filters from queryConfig + UI state are sent in API request

* exclude filters that are present in baseFilters

* update test name

* remove the filter configuration

* remove unneeded rename from spread

* update test to better reflect the state of queryConfig and requestState

* support range filters

Co-authored-by: Vadim Yakhin <vadim.yakhin@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yakhinvadim yakhinvadim yakhinvadim 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
@joemcelroy @yakhinvadim