-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Dashboards as code] Define schemas for As Code API filter interface #241198
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
base: main
Are you sure you want to change the base?
Conversation
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
|
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
7f78e6a to
5037bbe
Compare
davismcphee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the overall structure, and the new schemas are definitely a lot simpler than the existing ones! Just left a few questions and suggestions, and requesting an additional review from @lukasolson too for an extra set of expert eyes on this.
src/platform/packages/shared/kbn-es-query-server/src/filter/simplified_filter.ts
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-es-query-server/src/filter/simplified_filter.ts
Show resolved
Hide resolved
src/platform/packages/shared/kbn-es-query-server/src/filter/simplified_filter.ts
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-es-query-server/src/filter/simplified_filter.ts
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-es-query-server/src/filter/simplified_filter.ts
Outdated
Show resolved
Hide resolved
| export const simplifiedDSLFilterSchema = schema.object( | ||
| { | ||
| ...baseFilterPropertiesSchema, | ||
| dsl: rawDSLFilterSchema, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| dsl: rawDSLFilterSchema, | |
| query: rawDSLFilterSchema, |
I think we only ever support the query DSL property, don't we? I see we now expect an object containing query for the dsl prop, but I'm not sure anything else is actually valid. Maybe not even necessary to change the prop name, but should we get rid of that layer of nesting and only accept a query object directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. I wonder though if keeping the dsl property name would help us discriminate for possible future filter types such as support for ES|QL filters? In that case, I might see us introducing another schema like
export const simplifiedESQLFilterSchema = schema.object({
...baseFilterPropertiesSchema,
esql: esqlFilterSchema,
})We could still get rid of the unnecessary nesting of the query property. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping dsl and removing the nesting works for me! The prop name argument makes sense, it's mainly the extra layer of nesting I think could be confusing. And the prospect of ES|QL based filters is both intriguing and makes my head spin 😁
…ma' into simple-filters-schema
davismcphee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes look great, thanks! So much simpler now, I wish we could use it in our app code 🙂
| export const simplifiedDSLFilterSchema = schema.object( | ||
| { | ||
| ...baseFilterPropertiesSchema, | ||
| dsl: rawDSLFilterSchema, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping dsl and removing the nesting works for me! The prop name argument makes sense, it's mainly the extra layer of nesting I think could be confusing. And the prospect of ES|QL based filters is both intriguing and makes my head spin 😁
| export const appStateSchema = schema.literal('appState'); | ||
| export const globalStateSchema = schema.literal('globalState'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I might have approved prematurely. Didn't notice the type errors until now. Is it related to this change, maybe we need to stick with FilterStateStore here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the FilterStateStore enum causes a circular reference error since that would mean @kbn/es-query and @kbn/es-query-server depend on each other. Maybe we can export that enum (with future room for any others) from a new package (ex. @kbn/es-query-constants). Or is there an existing constants package we should use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of another package that would be good for this, so the new constants package sounds good 👍
| /** | ||
| * Schema for simple condition filters (Tier 1) | ||
| */ | ||
| export const simpleConditionFilterSchema = schema.object( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simpleConditionFilterSchema and simpleFilterConditionSchema are very similar sounding names that do not distinguish between the 2.
| * Main discriminated union schema for SimpleFilter | ||
| * Ensures exactly one of: condition, group, or dsl is present | ||
| */ | ||
| export const simpleFilterSchema = schema.oneOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I like the simple prefix. What is the plan with these schemas? If this is the preferred shape for filters moving forward, then the existing filter schemas should be prefixed with legacy or stored and then the schemas in this file should just be filter without a prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current filters interface is very ubiquitous around Kibana as it is has historically only validated in runtime with a concrete TypeScript type. A few months ago I introduced a server-side validations schema that is nearly syntactically identical to the concrete Filter type to support server-side validation. Although these concrete and inferred filter types are nearly identical now, they might diverge as TypeScript runtime validation and server validation may have different requirements. Or maybe the inferred type from stored filter schema can replace the concrete TypeScript type without too much interference for developer experience. But that is likely outside the scope of the as-code efforts.
I see the schema introduced in this PR as a high level API interface over the stored filters specifically for as-code endpoints. It provides stricter validation on the API endpoints and is intended to be easier for API consumers. To that end, I think it makes sense to rename the filterSchema to storedFilterSchema and drop the simple prefix in the API schemas. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To that end, I think it makes sense to rename the filterSchema to storedFilterSchema and drop the simple prefix in the API schemas. WDYT?
That sounds like a good plan.
| /** | ||
| * Schema for range values used in numeric and date filters | ||
| */ | ||
| export const rangeValueSchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just rangeSchema?
| /** | ||
| * Schema for all possible filter values | ||
| */ | ||
| export const filterValueSchema = schema.oneOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just valueSchema
| /** | ||
| * Base properties shared by all simplified filters | ||
| */ | ||
| const baseFilterPropertiesSchema = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just baseProperties. Its not a schema.object, its just a plain of javascript object. Its in the filters file so I don't think it needs the word filters in the name.
| /** | ||
| * Schema for 'is' and 'is_not' operators with single value | ||
| */ | ||
| const filterConditionIsSingleSchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just singleConditionSchema?
| /** | ||
| * Schema for 'is_one_of' and 'is_not_one_of' operators with array values | ||
| */ | ||
| const filterConditionIsOneOfSchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about oneOfConditionSchema?
| /** | ||
| * Schema for 'range' operator with range value | ||
| */ | ||
| const filterConditionRangeSchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about rangeConditionSchema?
| /** | ||
| * Schema for 'exists' and 'not_exists' operators without value | ||
| */ | ||
| const filterConditionExistsSchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about existsConditionSchema?
| @@ -0,0 +1,261 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see type or isMultiIndex in the schema. Maps uses this to signal that the filter does not target any index. For example, install flights and web logs sample data. Create a map with layers from both. Draw a shape filter. Below is the created filter. isMultiIndex drives the UI so that an data view selector is not displayed in the DSL editor. type is used by editor for help message. See "Editing Elasticsearch Query DSL prevents filter geometry from displaying in map." in the screen shot. Maps also uses type to know which filters to display on the map extractFeaturesFromFilters
{
"meta": {
"type": "spatial_filter",
"negate": false,
"alias": "intersects shape",
"disabled": false,
"isMultiIndex": true
},
"query": {
"bool": {
"should": [
{
"bool": {
"must": [
{
"exists": {
"field": "DestLocation"
}
},
{
"geo_shape": {
"ignore_unmapped": true,
"DestLocation": {
"relation": "intersects",
"shape": {
"coordinates": [
[
[
-116.98646,
49.95363
],
[
-69.4932,
36.01698
],
[
-89.978,
27.62399
],
[
-114.50929,
36.07558
],
[
-116.98646,
49.95363
]
]
],
"type": "Polygon"
}
}
}
}
]
}
},
{
"bool": {
"must": [
{
"exists": {
"field": "geo.coordinates"
}
},
{
"geo_shape": {
"ignore_unmapped": true,
"geo.coordinates": {
"relation": "intersects",
"shape": {
"coordinates": [
[
[
-116.98646,
49.95363
],
[
-69.4932,
36.01698
],
[
-89.978,
27.62399
],
[
-114.50929,
36.07558
],
[
-116.98646,
49.95363
]
]
],
"type": "Polygon"
}
}
}
}
]
}
}
]
}
},
"$state": {
"store": "appState"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the isMultiIndex and type properties to the schema to support spatial filters. But I wonder if we want to have a higher level of abstraction for API users to create spatial filters? Currently, this schema only supports spatial filters via a DSL query. I suppose that could always be a later enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, would be nice to have a better abstraction for those filters, especially if they're the reason for all the new properties in the schema. Doesn't have to be in this PR though.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Page load bundle
History
|
nreese
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schemas look good. Thanks for researching the problem.
code review only
| */ | ||
|
|
||
| /** | ||
| * Validation Schemas for Simplified Filter Interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Validation Schemas for "As code" Filter Interface
davismcphee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a bit more feedback based on the latest changes.
| "name": "@kbn/es-query-constants", | ||
| "private": true, | ||
| "version": "1.0.0", | ||
| "license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0" | |
| "license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0", | |
| "sideEffects": false |
Probably worth disabling side effects for this package even if it's tiny atm.
| export const appStateSchema = schema.literal('appState'); | ||
| export const globalStateSchema = schema.literal('globalState'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of another package that would be good for this, so the new constants package sounds good 👍
| filterType: schema.maybe( | ||
| schema.string({ | ||
| meta: { | ||
| description: | ||
| 'Filter type from legacy filters (e.g., "spatial_filter", "query_string") for backwards compatibility', | ||
| }, | ||
| }) | ||
| ), | ||
| key: schema.maybe( | ||
| schema.string({ | ||
| meta: { | ||
| description: 'Field name metadata from legacy filters for backwards compatibility', | ||
| }, | ||
| }) | ||
| ), | ||
| value: schema.maybe( | ||
| schema.string({ | ||
| meta: { | ||
| description: 'Value metadata from legacy filters for backwards compatibility', | ||
| }, | ||
| }) | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these all related to the spatial filter?
| @@ -0,0 +1,261 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, would be nice to have a better abstraction for those filters, especially if they're the reason for all the new properties in the schema. Doesn't have to be in this PR though.
Part of #237535
Fixes #241850
Summary
Introduce validation schemas for an API Filter interface in the
@kbn/es-query-serverpackage, enabling "* as Code" API consumers to work with filters using a human-readable, type-safe format instead of the complex and weakly typedFilterinterface that is currently stored.The current
Filterinterface, while extensible, is unsuitable for "* as Code" API consumers:meta.key,meta.field,meta.params) that duplicates information already in the query$stateRecord<string, any>in query structuresAdvantages
Our proposed API Filter schema offers more advantages for API consumers:
value, range filters requireRangeValue)@kbn/config-schemapackageReviewer notes
This PR only introduces new schemas and does not make any behavioral changes to code. Subsequent PRs will be created:
Technical details
Schema structure
introduces discriminated union schemas ensuring exactly one of:
See Examples below
Validation benefits
@kbn/config-schemavalidates input at API boundariesExamples of the new API filter schemas compared to the legacy filter
API Filter
Legacy format
{ "$state": { "store": "appState" }, "meta": { "alias": null, "disabled": false, "negate": false, "key": "host.keyword", "field": "host.keyword", "type": "phrase", "params": { "query": "www.elastic.co" }, "index": "90943e30-9a47-11e8-b64d-95841ca0b247" }, "query": { "match_phrase": { "host.keyword": "www.elastic.co" } } }API format
{ "pinned": false, "disabled": false, "dataViewId": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "condition": { "field": "host.keyword", "operator": "is", "value": "www.elastic.co" } }Grouped filter
Legacy format
[ { "meta": { "type": "combined", "relation": "AND", "params": [ { "query": { "match_phrase": { "host.keyword": "www.elastic.co" } }, "meta": { "negate": false, "key": "host.keyword", "field": "host.keyword", "type": "phrase", "params": { "query": "www.elastic.co" }, "index": "90943e30-9a47-11e8-b64d-95841ca0b247", "disabled": false } }, { "meta": { "negate": true, "index": "90943e30-9a47-11e8-b64d-95841ca0b247", "key": "machine.os.keyword", "field": "machine.os.keyword", "params": [ "ios", "osx" ], "value": [ "ios", "osx" ], "type": "phrases", "disabled": false }, "query": { "bool": { "minimum_should_match": 1, "should": [ { "match_phrase": { "machine.os.keyword": "ios" } }, { "match_phrase": { "machine.os.keyword": "osx" } } ] } } } ], "disabled": false, "negate": false, "alias": null, "index": "90943e30-9a47-11e8-b64d-95841ca0b247" }, "query": {}, "$state": { "store": "appState" } } ]API format
[ { "pinned": false, "disabled": false, "dataViewId": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "group": { "type": "and", "conditions": [ { "field": "host.keyword", "operator": "is", "value": "www.elastic.co" }, { "field": "machine.os.keyword", "operator": "is_not_one_of", "value": ["ios", "osx"] } ] } } ]Nested group filter
Legacy format
[ { "$state": { "store": "appState" }, "meta": { "alias": "My nested filter", "disabled": false, "negate": false, "type": "combined", "relation": "AND", "params": [ { "query": { "match_phrase": { "host.keyword": "www.elastic.co" } }, "meta": { "index": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "key": "host.keyword", "field": "host.keyword", "type": "phrase", "params": { "query": "www.elastic.co" } } }, { "meta": { "index": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "type": "combined", "relation": "or", "params": [ { "query": { "match_phrase": { "index.keyword": "kibana_sample_data_logs" } }, "meta": { "index": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "key": "index.keyword", "field": "index.keyword", "type": "phrase", "params": { "query": "kibana_sample_data_logs" } } }, { "query": { "bool": { "should": [ { "match_phrase": { "extension.keyword": "css" } }, { "match_phrase": { "extension.keyword": "gz" } } ], "minimum_should_match": 1 } }, "meta": { "index": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "key": "extension.keyword", "type": "phrases", "params": [ "css", "gz" ] } } ] } } ], "index": "90943e30-9a47-11e8-b64d-95841ca0b247" } } ]API format
[ { "pinned": false, "disabled": false, "dataViewId": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "label": "My nested filter", "group": { "type": "and", "conditions": [ { "field": "host.keyword", "operator": "is", "value": "www.elastic.co" }, { "type": "or", "conditions": [ { "field": "index.keyword", "operator": "is", "value": "kibana_sample_data_logs" }, { "field": "extension.keyword", "operator": "is_one_of", "value": ["css", "gz"] } ] } ] } } ]DSL filter
Legacy format
[ { "meta": { "type": "custom", "disabled": false, "negate": false, "alias": null, "key": "query", "index": "90943e30-9a47-11e8-b64d-95841ca0b247" }, "query": { "match": { "agent": { "query": "Mozilla", "fuzziness": "AUTO" } } }, "$state": { "store": "appState" } } ]API format
[ { "pinned": false, "disabled": false, "dataViewId": "90943e30-9a47-11e8-b64d-95841ca0b247", "negate": false, "dsl": { "query": { "match": { "agent": { "query": "Mozilla", "fuzziness": "AUTO" } } } } } ]