Merge multiple facet conditions using query's conjunction

@kir lazur, thank you very much!

I've created a merge request from your patch and credited you as the author.

Are you comfortable writing automated tests? If not, I'd be happy to help!

Are you comfortable writing automated tests?

I should probably clarify: I fixed the test that broke after applying your patch (i.e.: changing tag to tags in the constructed query).

... But a test for the exact scenario you were running into in the issue description doesn't exist yet, and we should create one, so that the behavior that you rely on doesn't regress in the future!

(housekeeping: now that there's an issue fork, I'm hiding the old patch; and I'm moving the issue to Needs work and adding the tag Needs tests)

I'm not sure that collect "terms" key is universal solution. It definitely works with simple string facet, but it could have issues if "exclude" options is enabled in facet or it is range or match facet.

I propose to collect all the filters and apply it to "agg" syntax. If filters works with "query" section so it should work here too.

Need to be tested with complex, negation facets and with ones with "exclude" option configured.

@kir lazur, @dewalt, something similar to this was done in #3465988: Error with facets "Expected [START_OBJECT] under [filter], but got a [START_ARRAY]..." which was just released in 8.0.0-alpha5.

May I trouble you to review that issue and/or try out the latest version of the module, and tell me if it works for you now?

If it works, let me know so I can close this issue.

If it doesn't work, let me know and I'll try to make some progress on this: it's marked as a blocker for a beta release.

Thank you very much in advance!

Thanks @mparker17, it really looks similar to fix that is needed. We still use beta4 and due our updates cycle I think I can check it in the end of month.

Things didn't seem to work entirely for my project with 8.0.0-alpha5 and the last comment in #3465988: Error with facets "Expected [START_OBJECT] under [filter], but got a [START_ARRAY]..." suggests a fix, i created a patch that seems to work in my case. Feel free to test it in your own projects if you run into the same problem.

It sounds like this is still an issue in the latest 8.0.x. version, so I'm moving this back to "Needs work".

***

Comparing merge request !76 (which is based on the patch in #4) with the patch in #10 and the patch in #13 ... all three of these patches seem to take different approaches to solving the problem. I'm not sure why this is: it could be that there are multiple problems with the same symptoms, site-specific code/config/patches on your site interfering with the module, or something else.

I tried running the Issue summary's Steps to reproduce on 8.0.x (at commit b43a683 from 2026-01-05), which I set up using the steps to Set up a local 8.0.x environment [for Developing Elasticsearch Connector 8.0.x] (i.e.: an empty database)... and I was not able to reproduce the issue. It's possible I could not reproduce the issue because the Steps to reproduce are not detailed enough. For reference, I have attached a configuration export (3497968-14--test-site--config-export.tar.gz) and a screenshot (3497968-14--test-site--screenshot.png).

***

Moving forward, I think once I understand how to reproduce the problem, then I will understand which of the three patches I want to move forward with, and/or if I need to file new issue(s), and how to write tests for the changes in the patch to ensure that the desired behavior doesn't regress.

To help us get to a solution that I can merge...

1. @kir lazur, could you test the patches in #10 and #13 to see if either (or both) of them fix the issue you were seeing on your site, and report back? (if neither of those solutions solve the issue on your site, then it is possible that we have multiple problems that display similar symptoms, and we should file separate issue(s) for the problem(s) experienced by @dewalt in #10 and @yaartse in #13)
2. @dewalt, could you review and/or test the patch in #13, and report back if it fixes the issue you were seeing on your site? (if it does, do you have any concerns with the patch in #13?)
3. @yaartse, could you briefly clarify if you tried the patch in #10 before writing your own patch?
4. Could someone update/replace the Steps to reproduce in the issue summary with more detail? Ideally, start from the standardized test environment described in the documentation on [Setting] up a local 8.0.x environment).

Thank you very much in advance, everyone!

oops, I forgot to upload the configuration export and screenshot I mentioned in the previous patch.

@mparker17 We were using a patch created from the MR (so based on the patches in #2 and #4) with 8.0.0-alpha4, but it didn't apply anymore after updating to 8.0.0-alpha5. I did try the patch in #10, but that one too didn't apply. I checked if the problems were solved as suggested in #11, but there was still something wrong with the filtering of multiple facets. I then read latest comment in https://www.drupal.org/project/elasticsearch_connector/issues/3465988#co... and tried that approach, which indeed seemed to work.

The problem we had can be reproduced by creating a view page with multiple facets. The facets themself should have the OR-operator, so if you select multiple options in the same facet, the values are filtered on whether they apply to one of the options and not all. In the view the operator between the facets should be set to AND, so if you select multiple facets the values are filtered on whether they apply to all the active facets and not just one of them. Without the patch only the operator inside the facets is used, not the operator between the facets. Therefor the filtering is incorrect.

@mparker17 sorry for delay, I've just updated to 8.0.0-alpha5 locally, and looks like know cases works fine without the patch. I'll give update when I deliver updates and QA makes regression, but it could take a time.

@yaartse Good to know!

I didn't receive a notification about the comment on the (closed) merge request in #3465988: Error with facets "Expected [START_OBJECT] under [filter], but got a [START_ARRAY]..." so I didn't know there was a bug!

I'll take another look soon!

@mparker17, i confirm the behavior described in #16.
Here is a patch that applies to 8.0.0-alpha5

@olivier.br, thank you for confirming, and for the patch that includes tests! That is very helpful!

***

I am going to read and update the issue summary to try to get a deeper understanding of the problem.

Then I am going to try to figure out which of the patches attached to this issue solve the problem described in the issue summary (sometimes separate problems present the same symptoms). I may split out some of the patches into a separate issue.

I apologize in advance for the delay in my responses to this ticket; and I apologize for the delays in getting to this point.

I have updated the issue summary; now to review the comments and patches.

I forgot to mark this as "Needs review"!

I've started to review this issue, but haven't finished yet, so I'm leaving this assigned to myself.

Notes to self (will update as I figure this out):

1. Patch elasticsearch_connector-OR-filterBuilder-3497968.patch from #2: duplicates work from #3549367: "Fielddata is disabled on [field] in [index]" error when trying to add a facet on a String field in an index managed by Search API (merged in commit 0d711e3d) and #3465988: Error with facets "Expected [START_OBJECT] under [filter], but got a [START_ARRAY]..." (merged in commit 3924e53a) — credited @kir lazur in both of those issues. Removed description of this approach from issue summary. Merge request !76 contained this approach; I've closed it.
2. Patch elasticsearch_connector-OR-filterBuilder-3497968-3.patch from #4: duplicates work from #3549367: "Fielddata is disabled on [field] in [index]" error when trying to add a facet on a String field in an index managed by Search API (merged in commit 0d711e3d) and #3465988: Error with facets "Expected [START_OBJECT] under [filter], but got a [START_ARRAY]..." (merged in commit 3924e53a) — credited @kir lazur in both of those issues. Merge request !76 contained this approach; I've closed it.
3. Patch elasticsearch_connector-or_facet_agg-3497968-10.patch from #10: new approach A (i.e.: if it finds exactly 1 filter, it does what it did before; otherwise, it joins the filters together by wrapping them in a 'bool' -> 'should', i.e.: "OR"). Does not apply to 3d4d1be. After rebasing, several automated tests fail: I see a malformed query response in ElasticSearchBackend.php:481 ("[1:180] [bool] failed to parse field [must]","caused_by":{"type":"parsing_exception","reason":"[category] query malformed, no start_object after query name"), and two instances of us trying to count strings in FacetParamBuilderTest::testOneAndFacetQuery() and FacetParamBuilderTest::testOneOrFacetQuery()
4. Patch 3497968-use-condition-group-conjunction.patch from #13: new approach B2 (i.e.: get a conjunction from the query's conjunction group, and pass the conjunction to FacetParamBuilder::buildTermBucketAgg). Does not apply to 3d4d1be. After rebasing, several tests fail with a "Call to a member function getConjunction() on null" in FacetParamBuilderTest's ::testOneAndFacetQuery(), ::testOneOrFacetQuery(), ::testUnknownFacetFieldLogsWarning(), ::testTextFieldUsesKeywordSubfield(), and ::testNonFulltextFieldDoesNotUseKeywordSubfield(), which makes sense since the patch didn't update any tests.
5. Patch 3497968-19.patch from #19: new approach B2 (i.e.: pass the query to FacetParamBuilder::buildTermBucketAgg, and get a conjunction from the query's conjunction group inside that function). Does not apply to 3d4d1be. After rebasing, several tests fail: I'm seeing Call to a member function getConjunction() on null in FacetParamBuilderTest::testOneAndFacetQuery() (which makes sense because we started mocking the query in that test the patch in #19 was originally created) and FacetParamBuilderTest::testOneOrFacetQuery(), and Call to a member function getFulltextFields() on null in FacetParamBucketAggregationBuilderTest::testLimitOptions() (it looks like these two tests were failing when the patch was originally created, so it looks like the patch hadn't been completed yet)

In terms of approach, think I like the idea of passing a reference to the query object to FacetParamBuilder::buildTermBucketAgg() (i.e.: approach B2, i.e.: from the patch in #19) instead of passing the conjunction (i.e.: approach B1 from the patch in #13); because if buildTermBucketAgg() ever needs additional context from the query in the future, we don't have to add another parameter to its signature (which is important for backwards compatibility).

I also like the approach of getting the conjunction from the query, instead of assuming that when there are multiple filters, they should be ORed together (i.e.: approach A from the patch in #10).

So I think I'm going to move forward with the patch in #19.

I will update the issue summary, create new merge requests off 9.0.x and 8.0.x, mark this issue for backport, apply my rebased version of the patch from #19, and fix the outstanding test issues.

I will post another comment when there is something to test. Thank you!

(As an aside, as a maintainer, it truly saves me a lot of time when we collaborate on a fix! Thank you for understanding!)

(Updating the issue title before creating branches/merge-requests)

Okay, I've fixed the tests and added some. For convenience, I've attached patches for both the 9.0.x and 8.0.x branches to this comment.

@kir lazur, @dewalt, @yaartse, @olivier.br, @karolinam, would you be able to test one of these patches and tell me if it solves the issue you were experiencing?

I was able to finally reproduce this. Part of my confusion trying to reproduce this was the ability to set the Operator in multiple places:

Views filter "global"

This of course is using Facets in views.

At the individual facet level

It seemed like when I was consistent and set all operators to OR or all to AND things worked as expected without needing the patch. Either way, the patch does seem to make things work consistently. I did NOT test more complex filter groups Views.

The patch for 9.0.x also fixes Facet counts are incorrect after selecting multiple facets - regression from issue #3465988

+1 MR-185 fixed facet count error reported on our site

I haven't heard back from the original reporters of the issue, but given that it's working for @joelsteidl and @fathershawn, I'm going to merge it.

Preparing to merge to 8.0.x

Merged to 8.0.x. I will update this issue when I've released the changes in this issue.