[core.savedObjects] Update esKuery imports to pull from new package instead of data plugin

[lukeelmers]

Blocked by #51659

Edit: The scope of this issue has been reduced -- right now the plan is to wait for the app services team to address #51659, which will move esKuery into a dedicated package again. Once that's complete, all we will need to do in Core is update our imports to use the new package instead.

In #42012 it was decided that we would move the @kbn/es-query package into the data plugin, which was merged in #51014.

In #41136, functionality was added to Saved Objects Client which allowed filtering using a KQL string. Internally, core was using the former @kbn/es-query package to parse the KQL.

Since es-query is no longer a standalone package, this means a dependency has been created between core (which owns saved objects), and data (which owns KQL). Having core depend on any other plugins runs counter to the overall design of the Kibana platform, so we need to go back and remove this dependency.

The plan discussed with @rudolf, @joshdover, and @Bargs is as follows:

- [ ] 1. @elastic/kibana-platform - Expose a generic ES filter api for Saved Objects find
- [ ] 2. @elastic/kibana-app-arch - #51659 After (1) is complete, create a server API in the Data Query Service that can take Saved Object Client & a KQL query and validate/compile the KQL to an ES Filter which is passed to the SO client. We should be able to reuse a lot of the existing code that does this from /src/core/server/saved_objects/service/lib/filter_utils.ts for this.

This particular issue is here to track (1) above, which blocks us from starting on (2 / #51659).

[kobelb]

Exposing a generic ES filter API for Saved Objects find comes with some complications. Currently, we've tried to hide from consumers of the SavedObjectsClient the way in which these saved-objects are persisted in documents in Elasticsearch.

For example, the following saved-object

{
  "attributes": {
    "description": "",
    "kibanaSavedObjectMeta": {
      "searchSourceJSON": "{\"query\":{\"language\":\"kuery\",\"query\":\"\"},\"filter\":[],\"indexRefName\":\"kibanaSavedObjectMeta.searchSourceJSON.index\"}"
    },
    "title": "wave height",
    "uiStateJSON": "{}",
    "version": 1,
    "visState": "..."
  },
  "id": "b1b4e010-c79f-11ea-b1d7-095f7b70c412",
  "migrationVersion": {
    "visualization": "7.8.0"
  },
  "namespaces": [
    "waves"
  ],
  "references": [
    {
      "id": "88877e50-c79f-11ea-b1d7-095f7b70c412",
      "name": "kibanaSavedObjectMeta.searchSourceJSON.index",
      "type": "index-pattern"
    }
  ],
  "type": "visualization",
  "updated_at": "2020-07-17T16:22:37.129Z",
  "version": "WzMxMiwxXQ=="
}

is mapped to the following ES document

{
    "visualization" : {
      "title" : "wave height",
      "visState" : "..",
      "uiStateJSON" : "{}",
      "description" : "",
      "version" : 1,
      "kibanaSavedObjectMeta" : {
        "searchSourceJSON" : """{"query":{"language":"kuery","query":""},"filter":[],"indexRefName":"kibanaSavedObjectMeta.searchSourceJSON.index"}"""
      }
    },
    "type" : "visualization",
    "references" : [
      {
        "name" : "kibanaSavedObjectMeta.searchSourceJSON.index",
        "type" : "index-pattern",
        "id" : "88877e50-c79f-11ea-b1d7-095f7b70c412"
      }
    ],
    "namespace" : "waves",
    "migrationVersion" : {
      "visualization" : "7.8.0"
    },
    "updated_at" : "2020-07-17T16:22:37.129Z"
  }

Consumes of SavedObjectsClient#find shouldn't have to know that they shouldn't write an ES DSL predicate using attributes.title and should instead use visualization.title. Using KQL allows us to transparently rewrite these predicates to hide this implementation detail from consumers.

We could theoretically only support a subset of the ES DSL predicates which we can rewrite. However, this would end up solving a very similar problem to the one that KQL is trying to solve.

[lukeelmers]

This issue just came up in discussion on #78348, and @kobelb mentioned:

To address #55485, I thought we were just going to pull the "es_query" code out into a place where both core and the data plugin can import it.

The only issue with re-creating the es_query package is that we will run into the same issue that drove us to merge it into the data plugin in the first place: It depends on type interfaces from data like IIndexPattern, IFieldType, Query, plus also now some from the kibana_utils plugin.

So we'd need to sort out how to deal with this if we decide to move back to a package:

1. Duplicate types (annoying for maintenance, risks future regressions)
2. Remove types (decreases type safety)
3. Relocate dependent types to the package (makes little sense, blurs domain boundaries)
4. Something else?

[pgayvallet]

The only issue with re-creating the es_query package is that we will run into the same issue that drove us to merge it into the data plugin in the first place: It depends on type interfaces from data like IIndexPattern, IFieldType, Query

I don't know the functional of the es_query package in detail, but why is the package relying on types such as IIndexPattern? Did we took some shortcuts by using our already existing types instead of adding an abstraction layer, or all the actual types 'really' fully used?

Relocate dependent types to the package (makes little sense, blurs domain boundaries)

An alternative would be to have an additional package, such as data-types that would be used by both the data plugin and the es-query package.

[kobelb]

We can also take advantage of TypeScript's structural subtyping so there are two definitions of the interface, the one that es_query "owns" and the one that the data plugin "owns", they just happen to be compatible.

[lukeelmers]

We can also take advantage of TypeScript's structural subtyping so there are two definitions of the interface, the one that es_query "owns" and the one that the data plugin "owns", they just happen to be compatible.

This is basically how it was handled before (option 1), where es_query had its own definitions for index patterns, queries, filters, etc. And while structural subtyping meant we still had reasonable type safety, the concern with having them duplicated was simply that it would be easy to forget to update both if the interface ever needed to change.

I don't know the functional of the es_query package in detail, but why is the package relying on types such as IIndexPattern?

@lukasolson would probably have more history on this, but in general es_query is using index patterns to be more intelligent about how to construct queries, because it can look at scripted fields, whether fields are nested, etc.

Right now it seems the least-worst options are reverting to how we used to do things (option 1 above, where types are re-defined in a separate package), or perhaps the additional data-types package that @pgayvallet suggests. Though my only concern with a data-types package is where you would/should draw the line on what to include. I could easily see that creeping into another utils-like slush bucket of random stuff.

[lukeelmers]

One other thing I just thought of: If we were to make a separate package for es-query and data-types, it would have the benefit of addressing the main issue we need to resolve for #80510

[ppisljar]

what's the minimum set of es-query that core requires ? does that specific parts have dependencies on types from data plugin ? if not, could we extract only that part into a package (so we don't bring any dependency on data plugin over) ? could be that we just need to split the query plugin into multi parts ?

[pgayvallet]

what's the minimum set of es-query that core requires

I have no idea to be honest. As we are allowing consumers to pass a KueryNode to find, I would assume we might need it 'all'?

[lukeelmers]

The new plan is for @elastic/kibana-app-services to move esKuery back into a package to remove the circular dependency. That work will be tracked in #51659.

I will update the original issue description here, but the new scope for Core in this issue will be pretty straightforward -- basically updating our imports once the new package is ready.

[joshdover]

@lukeelmers @ppisljar Will there really be any work for Core to do here? I suspect for CI to pass, we'll need to update the imports in the same PR as the work in #51659. Seems like we could close this?

[lukeelmers]

@joshdover We wouldn't need to update imports at the same time if the package contents were still re-exported by the data plugin (so that downstream users don't need to know about its existence unless they have a reason to). But that's a small enough thing to change that maybe app services would be willing to do it all at once? 😉

[joshdover]

@ppisljar any update here on moving esKuery?

[lukeelmers]

I believe @lizozom is working on this currently, splitting the work into a couple PRs. Not sure how much work is remaining, though.