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.

Sign up for GitHub

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 SO query for searching across spaces #83025

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Fix SO query for searching across spaces #83025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/core/server/saved_objects/service/lib/search_dsl/query_params.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,11 @@ describe('#getQueryParams', () => {
if (registry.isMultiNamespace(type)) {
const array = [...(namespaces ?? [DEFAULT_NAMESPACE_STRING]), ALL_NAMESPACES_STRING];

const namespacesClause = { terms: { namespaces: array } };
return {
bool: {
must: namespaces?.includes(ALL_NAMESPACES_STRING)
? expect.not.arrayContaining([namespacesClause])
: expect.arrayContaining([namespacesClause]),
? [{ term: { type } }]
: [{ term: { type } }, { terms: { namespaces: array } }],
must_not: [{ exists: { field: 'namespace' } }],
},
};
Expand Down
14 changes: 10 additions & 4 deletions src/core/server/saved_objects/service/lib/search_dsl/query_params.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,19 @@ function getClauseForType(
const searchAcrossAllNamespaces = namespaces.includes(ALL_NAMESPACES_STRING);

if (registry.isMultiNamespace(type)) {
const namespacesFilterClause = searchAcrossAllNamespaces
? {}
: { terms: { namespaces: [...namespaces, ALL_NAMESPACES_STRING] } };
const typeFilterClause = { term: { type } };

const namespacesFilterClause = {
terms: { namespaces: [...namespaces, ALL_NAMESPACES_STRING] },
};

const must = searchAcrossAllNamespaces
? [typeFilterClause]
: [typeFilterClause, namespacesFilterClause];

return {
bool: {
must: [{ term: { type } }, namespacesFilterClause],
must,
Comment on lines -87 to +93
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, overlooked that too. So we were using [{ term: { type } }, {}] in bool.must?

  • What was the exact consequences?
  • I thought you added a FTR test in the initial PR. Why wasn't this catched by it? Should we add a new one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what was happening. The consequence was a query parse exception:

{
  "name": "ResponseError",
  "meta": {
    "body": {
      "error": {
        "root_cause": [
          {
            "type": "x_content_parse_exception",
            "reason": "[1:120] [bool] failed to parse field [must]"
          }
        ],
        "type": "x_content_parse_exception",
        "reason": "[1:120] [bool] failed to parse field [filter]",
        "caused_by": {
          "type": "x_content_parse_exception",
          "reason": "[1:120] [bool] failed to parse field [should]",
          "caused_by": {
            "type": "x_content_parse_exception",
            "reason": "[1:120] [bool] failed to parse field [must]",
            "caused_by": {
              "type": "illegal_argument_exception",
              "reason": "query malformed, empty clause found at [1:120]"
            }
          }
        }
      },
      "status": 400
    },
    "statusCode": 400,
    "headers": {
      "content-type": "application/json; charset=UTF-8",
      "content-length": "541"
    },
    "meta": {
      "context": null,
      "request": {
        "params": {
          "method": "POST",
          "path": "/.kibana/_search",
          "body": "{\"seq_no_primary_term\":true,\"query\":{\"bool\":{\"filter\":[{\"bool\":{\"should\":[{\"bool\":{\"must\":[{\"term\":{\"type\":\"ml-job\"}},{}],\"must_not\":[{\"exists\":{\"field\":\"namespace\"}}]}}],\"minimum_should_match\":1}}]}}}",
          "querystring": "size=1&from=0&rest_total_hits_as_int=true",
          "headers": {
            "user-agent": "elasticsearch-js/7.10.0-rc.1 (darwin 19.6.0-x64; Node.js v10.22.1)",
            "x-elastic-product-origin": "kibana",
            "content-type": "application/json",
            "content-length": "201"
          },
          "timeout": 30000
        },
        "options": {
          "maxRetries": 0,
          "ignore": [
            404
          ]
        },
        "id": 20
      },
      "name": "elasticsearch-js",
      "connection": {
        "url": "http://localhost:9200/",
        "id": "http://localhost:9200/",
        "headers": {},
        "deadCount": 0,
        "resurrectTimeout": 0,
        "_openRequests": 4,
        "status": "alive",
        "roles": {
          "master": true,
          "data": true,
          "ingest": true,
          "ml": false
        }
      },
      "attempts": 0,
      "aborted": false
    }
  },
  "isBoom": true,
  "isServer": false,
  "data": null,
  "output": {
    "statusCode": 400,
    "payload": {
      "message": "[1:120] [bool] failed to parse field [filter]: x_content_parse_exception",
      "statusCode": 400,
      "error": "Bad Request"
    },
    "headers": {}
  }
}

I thought you added a FTR test in the initial PR. Why wasn't this catched by it? Should we add a new one?

The FTR test verified single namespace types, not multi-namespace types. I can't organically exercise this via the FTR because OSS doesn't have any multi-namespace types, and the default distribution won't allow the * namespace to make its way to the repository.

I'd either have to create an FTR config without spaces or security enabled in the default distribution, or create a test plugin which exposed a route that we could call in test to exercise this path directly. Both options seemed overkill to me, but I'm happy to revisit if you think it'd be worthwhile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 'amazing' that we don't have a SO FTR testsuite in x-pack/test/api_integration/apis tbh... I guess :thisisfine:, ml should introduce 'proper' coverage when they'll leverage the feature.

must_not: [{ exists: { field: 'namespace' } }],
},
};
Expand Down