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.

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

Fix Elasticsearch 7 double array issue #2119

Open
wants to merge 1 commit into
base: 8.x
Choose a base branch
from

Conversation

farzadjafari
Copy link

What does this PR do?

This throws an exception when you try to build an ES query with a deprecated format since ES 7.7.

This would be a breaking change for anybody who uses ES prior to version 7.7.

This PR will give developers the opportunities to find the breaking change by the line number in their code via the exception and prevent future queries in the old format.

Opening PR early can add a test if it seems like a reasonable approach.

Here is the demonstration of other people running into the same issue:
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/32813

This fixes an issue in Elasticsearch 7.7+ versions (#218324 (closed)). Previously the query parser was forgiving about nested arrays and would just automatically flatten them for you but now we need to flatten them ourselves. I did read about this in https://discuss.elastic.co/t/must-not-boolean-query-in-7-7/233269 but sadly they do not consider this a breaking change so it was not obvious the cause initially.

# Example request
GET /addresses/_search
{
  "query": {
    "bool": {
      "should": [
        [
          {
            "terms": {
              "Example": [
                1,
                2
              ]
            }
          }
        ]
      ]
    }
  }
}

# Example response
{
  "error" : {
    "root_cause" : [
      {
        "type" : "x_content_parse_exception",
        "reason" : "[5:9] [bool] failed to parse field [should]"
      }
    ],
    "type" : "x_content_parse_exception",
    "reason" : "[5:9] [bool] failed to parse field [should]",
    "caused_by" : {
      "type" : "illegal_state_exception",
      "reason" : "expected value but got [START_ARRAY]"
    }
  },
  "status" : 400
}

@ruflin
Copy link
Owner

ruflin commented Sep 22, 2022

Thanks for the contribution @farzadjafari For my understanding:

  • Anyone using >= 7.7 and using such a query will have this issue anyways
  • Anyone using <7.7 will not have the issue but if we get this PR in, Elastica will break / notify these users to adjust their queries?

@stchr stchr mentioned this pull request Sep 23, 2022
@ruflin
Copy link
Owner

ruflin commented Sep 28, 2022

Can you rebase on master because #2121 just got merged to fix some of the failing tests.

@farzadjafari farzadjafari force-pushed the Elastica-non-associative-bool-queries branch from 43acccb to 2009cd2 Compare September 30, 2022 17:58
@farzadjafari
Copy link
Author

Thanks for the contribution @farzadjafari For my understanding:

  • Anyone using >= 7.7 and using such a query will have this issue anyways
  • Anyone using <7.7 will not have the issue but if we get this PR in, Elastica will break / notify these users to adjust their queries?

Yes. Correct for both.

@deguif
Copy link
Collaborator

deguif commented Oct 5, 2022

Or maybe we could use the polyfill for PHP 8.1 (symfony/polyfill-php81) in order to use the array_is_list.
Using the polyfill will ensure we can use the function safely, and if users want to suppress the polyfill because they are running on PHP version >= 8.1 they can use replace section of composer to avoid installing this package.

@ruflin
Copy link
Owner

ruflin commented Oct 10, 2022

I'm not a big fan of adding too many dependencies but if what @deguif proposes work, it would be great to follow this path as it would mean we don't break any users as far as I understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants