ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.26k stars 736 forks source link

Fix Elasticsearch 7 double array issue #2119

Open farzadjafari opened 2 years ago

farzadjafari commented 2 years ago

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 commented 2 years ago

Thanks for the contribution @farzadjafari For my understanding:

ruflin commented 2 years ago

Can you rebase on master because https://github.com/ruflin/Elastica/pull/2121 just got merged to fix some of the failing tests.

farzadjafari commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.