feat(elasticsearch-plugin): Add facetFilters input for search query

[Izayda]

Related #726

This PR hasn't e2e test (only unit tests), because i don't know how to safely extend generated-e2e-shop-types.ts with new field.

[michaelbromley]

The implementation looks good, but I'm not sure about the API - I feel it might be confusing that the SearchInput now has both facetValueIds and facetFilters fields, which essentially do the same thing (filter by facet value).

I understand why you did it this way, because we need the existing SearchInput API to still be compatible with the DefaultSearchPlugin API. I have been trying to think of ways to use the existing API in a backwards-compatible way, e.g.

extend input SearchInput {
  facetValueIds: [ID!] | [[ID]]
}

but this is not valid in GraphQL unfortunately.

I guess in the docs we should highlight the improved facet-filtering capabilities of the ES plugin and advise devs to favour the use of the facetFilters input field, and explain that the facetValueIds and facetValueOperator fields are mainly there for backwards-compatibility with the DefaultSearchPlugin.

[Izayda]

Yes, totally agree with you. Probably, i can extend DefaultSearchPlugin with new input and we can mark old input as deprecated. Seems, it would be more consistent and moreover, allow to write e2e tests.

[michaelbromley]

If you have the time to do that I would gladly accept it! I would also suggest renaming to facetValueFilters to make it clear we are filtering by FacetValue and not Facet IDs.

[Izayda]

Ok, i will do that in couple of days and update PR.

One moment. How to mark field as deprecated?

[Izayda]

Or is there reason to divide implementation on separate PRs?

[michaelbromley]

You can keep it in the same PR. To mark as deprecated use the @deprecated(reason: " ... ") directive in the GraphQL SDL.

[Izayda]

Ok, probably done. Please review :)

Probably this might be added to docs somewhere:

* # FacetFilters input
 * Moreover, the [SearchInput](/docs/graphql-api/admin/object-types/#searchinput) type is extended with
 * FacetFilters parameter to put multiple conditions of facet filtering.
 *
 * ```SDL
 * extend input SearchInput {
 *    facetFilters: [FacetFilterInput!]
 * }
 * input FacetFilterInput {
 *     facetId: ID
 *     facetIds: [ID!]
 * }
 * ```
 *
 * # Simple OR filter
 * This example translate as facetID=1 OR facet_id=2
 * ```JSON
 * {facetFilters: [{facetIds:[1,2]}]}
 * ```
 * # Simple AND filter
 * This example translate as facetID=1 AND facet_id=2
 * ```JSON
 * {facetFilters: [{facetId:1]},{facetId:2}]}
 * ```
 * # AND and OR filter combination
 * This example translate as facetID=1 AND (facet_id=2 OR facet_id=3)
 * ```JSON
 * {facetFilters: [{facetId:1]},{facetIds:[2,3]}]}
 * ```

[michaelbromley]

This is great. I have 1 last bit of feedback:

When I was manually testing out the filters, I found it difficult to remember whether I should be using facetValueIds or facetValueId in order to get the desired boolean filter expression.

Consider the current naming:

This example translate as facetID=1 OR facet_id=2
{ facetValueFilters: [{ facetValueIds:[1,2] }]}

This example translate as facetID=1 AND facet_id=2
{ facetValueFilters: [{ facetValueId:1 }, { facetValueId:2 }]}

This example translate as facetID=1 AND (facet_id=2 OR facet_id=3)
{ facetValueFilters: [{ facetValueId:1 }, { facetValueIds:[2,3] }]}

What do you think of a more explicit naming, such as replacing facetValueId with and, and facetValueIds with or:

This example translate as facetID=1 OR facet_id=2
{ facetValueFilters: [{ or:[1,2] }]}

This example translate as facetID=1 AND facet_id=2
{ facetValueFilters: [{ and:1 }, { and:2 }]}

This example translate as facetID=1 AND (facet_id=2 OR facet_id=3)
{ facetValueFilters: [{ and:1 }, { or:[2,3] }]}

[Izayda]

Great idea! Need 20 minutes to make this rename

[michaelbromley]

Did you generate the types with the ES plugin configured?

[Izayda]

Yes, ma fault :(

[michaelbromley]

OK. Push again with corrected codegen and it's ready to merge 👍

[michaelbromley]

Thank you!