Problem/Motivation

For date fields, the facets using ranges fail to work as ElasticSearch uses different operators than the defaults provided. The defaults provided use "from" and "to", while ElasticSearch uses "gt", "gte", "lt", "lte", etc.

This requires the Full Date upgrade path fix from #3470901: Upgrade path for full_date data type

Steps to reproduce

  1. Create a date in a content type and add it to the ElasticSearch instance using the Full Date option.
  2. Create a search view with a facet on the date field. Expose the filter and the operator.
  3. Use each filter and you will get an error.

Proposed resolution

Update the FilterBuilder class in ElasticSearch Connector to use the proper operators mentioned above.

Remaining tasks

  1. Write initial patch (done by @Kirk Brown in merge request !98)
  2. Write tests - done by @kdborg and @mparker17 by #9
  3. Review and feedback - done by @kdborg and @mparker17 in #9
  4. RTBC and feedback - done by @kdborg and @mparker17 in #10
  5. Commit - done by @mparker17 in #11
  6. Release - done by @mparker17 in 8.0.0-alpha5

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#2 elasticsearch_connector-range_filter_fix-3535733-1.patch2.21 KBkdborg@gmail.com

Issue fork elasticsearch_connector-3535733

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

kdborg@gmail.com created an issue. See original summary.

kdborg@gmail.com’s picture

kdborg@gmail.com commented
StatusFileSize
new elasticsearch_connector-range_filter_fix-3535733-1.patch2.21 KB

Here's the patch. It has been tested with other data types which can use range filters.

mparker17 made their first commit to this issue’s fork.

mparker17 opened merge request !98

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Issue tags: +Needs tests

@Kirk Brown, thank you for the contribution!

I committed your code to a merge request to make it easier for me to review.

I took a very quick look at your patch. It looks good. As you mention in the Issue Summary, it needs tests.

Could you also update the Issue Summary's "Steps to reproduce" to include the error you get on an unpatched module? People often find issues by searching for the error message, so this is often helpful.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Active » Needs work

Updating issue status

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

I notice that Testbot is detecting some failing tests.

The failures in \Drupal\Tests\elasticsearch_connector\Unit\SearchAPI\Query\FilterBuilderTest are expected, given the changes you made. Basically, FilterBuilderTest looks at the queries that FilterBuilder generates for given inputs, and since your patch changes how the queries are built, test failures here are expected. The upshot is that to fix the tests, you need to change what FilterBuilderTest expects.

The test failures in \Drupal\Tests\elasticsearch_connector\Kernel\ElasticSearchBackendTest might be a bit harder to resolve.

ElasticSearchBackendTest runs queries against an Elasticsearch instance running in the test environment, and checks that the query results make sense. We inherit most of the tests in that file from Search API's \Drupal\Tests\search_api\Kernel\BackendTestBase; which creates, indexes, and searches content created by \Drupal\Tests\search_api\Functional\ExampleContentTrait.

It looks like the part of the test that is failing in your patch is in ElasticSearchBackendTest::searchSuccess(), which is a part that deals with ranges. In particular, it's looking at which entities are returned when querying the date ranges that the entity was created (i.e.: queries on the created field, which are set in \Drupal\Tests\elasticsearch_connector\Kernel\ElasticSearchBackendTest::addTestEntity()). Note that this is completely different than the original test it overrides in BackendTestBase::searchSuccess(), which was testing ranges on the entities' width field (a decimal or float number).

As a bit of historical context, when we ported elasticsearch_connector-8.x-7.x to elasticsearch_connector-8.0.x, I remember several failing tests in that class, and I believe that we overrode some of the failing test functions when we had exhausted all other ideas for why they were failing. Based on the fact that we overrode searchSuccess(), it is likely that range searches weren't working for decimal/float numbers — but if that is true, then I would have expected range queries for created-date-field values to fail as well.

Anyway, the specific part of the failing test is supposed to find entities that were created strictly-after (gt) 2008-06-28; and the test after your changes is returning additional results (i.e.: with created-field dates of 2003-07-10 and 2008-06-28, i.e.: ones that are not strictly-after 2008-06-28), which seems wrong.

On the other hand, it seems like deleting our overrides (i.e.: ElasticSearchBackendTest::searchSuccess() and ElasticSearchBackendTest::addTestEntity(), i.e.: and going back to the original BackendTestBase::searchSuccess() range-searches-by-weight) fixes things? All that being said, because I don't remember the exact circumstances that lead us to overriding searchSuccess() in that way, I would want to know why range queries on the created-date-field seemed to be working before I would be comfortable with deleting our override.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

(sorry for the noise: I rebased onto the latest 8.0.x, so that the phpstan pipeline wouldn't warn us about a deprecation that was fixed in 8.0.x)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

Okay! @kdborg and myself fixed the FilterBuilderTest to account for the new code; and found that the overrides we had been making in ElasticSearchBackendTest were no longer necessary because range filters are now working properly, yay!

We're going to do some manual testing now.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Manual testing worked for me! I think this is ready to merge.

  • mparker17 committed a4fd8f75 on 8.0.x 
    [#3535733] fix: Range Searches

By: kdborg@gmail.com
By: mparker17
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Merged! Thanks everyone!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Released in 8.0.0-alpha5!

dewalt’s picture

dewalt commented
Related issues: +#3551591: Filter does not work with date range fields

This fix leads to other issue - https://www.drupal.org/project/elasticsearch_connector/issues/3551591

dewalt’s picture

dewalt commented

By the way I see that test covering date range filters was failing (and looks like it was because timestamp support for gt/lt filters was lost).
And it is interesting solution just delete the failing method in test and merge O_O. I think with #3551591 fix the test should pass and could be returned back to avoid bugs in future.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

@dewalt sorry about that!

If I recall correctly, while @kdborg and I were working on this issue, we noticed that ElasticSearchBackendTest::searchSuccess() was overriding \Drupal\Tests\search_api\Kernel\BackendTestBase::searchSuccess().

When we compared ElasticSearchBackendTest::searchSuccess() with BackendTestBase::searchSuccess(), it looked at the time as if ElasticSearchBackendTest::searchSuccess() was an older copy of BackendTestBase::searchSuccess()... except that the tests for range searches had been removed from ElasticSearchBackendTest::searchSuccess() so that the test would pass.

When we deleted the overridden method, the tests passed, so we stopped there.