[Dashboards as code] Add conversion functions from/to API filters interface and stored filters#242043
Conversation
…le-filters-transforms
…ma' into simple-filters-schema
This allows both `@kbn/es-query` and `@kbn/es-query-server` to use the same constants and avoids circular dependency issues
…le-filters-transforms
…Schema" to "filterSchema"
…le-filters-schema
…le-filters-transforms
ThomThomson
left a comment
ThomThomson
left a comment
There was a problem hiding this comment.
I have a few questions about the deprecated items in the schema. If at all possible I'd very much like to avoid our new schema for as-code filters having any deprecated fields or any fields that exist solely to retain BWC information.
|
|
||
| **Special Cases**: | ||
|
|
||
| - Pinned filters (globalState) are skipped → returns `undefined` |
There was a problem hiding this comment.
This is great for now, but eventually we'd want to make use of this in the Unified search plugin, which will require pinning. We'll think of that when we get to it, but I'm imagining a simple type-only extension to add a top level pinned boolean. This wouldn't leak into the schemas because pinned filters are never stored.
|
|
||
| **Smart Type Detection**: | ||
|
|
||
| - Uses preserved `filterType` if available |
There was a problem hiding this comment.
How much drift does it introduce when filterType is dropped during the conversion? I'd really like to avoid releasing our new definition with deprecated fields. Seems overly cautious perhaps?
There was a problem hiding this comment.
Starting with dd93a49 I've modified the schemas to use the new discriminated union type from @kbn/config-schema with a type property rather than trying to infer the type from the shape of the As Code Filter. This also means we can have a spatial as code filter type so that we can remove the deprecated fields (key, filter_type) that previously maintained round-trip fidelity for stored spatial filters.
This new discriminated union simplifies the transform logic and should make the OpenAPI specs cleaner and easier to use for API consumers.
|
|
||
| **Group Optimization**: | ||
|
|
||
| - OR group with same-field IS conditions → `phrases` filter (compact representation) |
…stored filter meta
ThomThomson
left a comment
ThomThomson
left a comment
There was a problem hiding this comment.
Excellent work and response to / incorporation of all the previous feedack. Looked through the code and schemas again, and tested locally by running this PR and everything is looking and feeling great. Seeing filters come back in the network tab with a clean schema is huge, and I'm starting to feel like this is a step towards fixing one of the biggest irks of Kibana in general. Very nice set of changes, LGTM!
| export const asCodeConditionFilterSchema = basePropertiesSchema.extends( | ||
| export const asCodeConditionFilterSchema = commonBasePropertiesSchema.extends( | ||
| { | ||
| type: schema.literal(ASCODE_FILTER_TYPE.CONDITION), |
There was a problem hiding this comment.
This new type concept is great!
lukasolson
left a comment
lukasolson
left a comment
There was a problem hiding this comment.
Left a few minor comments below, overall this is looking really good!
| throw new FilterConversionError( | ||
| 'AsCodeFilter must have exactly one of: condition, group, dsl, or spatial' | ||
| ); |
There was a problem hiding this comment.
Is it possible to get to this code?
| const conditionSchema = schema.oneOf( | ||
| [singleConditionSchema, oneOfConditionSchema, rangeConditionSchema, existsConditionSchema], |
There was a problem hiding this comment.
Can we use schema.discriminatedUnion for these as well?
| group: schema.object( | ||
| { | ||
| type: schema.oneOf([schema.literal('and'), schema.literal('or')]), | ||
| type: schema.oneOf([ |
There was a problem hiding this comment.
Nit: Can we rename to something like operator? Reusing type seems like it may add to confusion, and operator is nicely aligned with the condition operator (and makes sense for things like AND and OR)
| * Discriminated union schema for simple filter conditions with proper operator/value type combinations | ||
| * Discriminated union schema for filter conditions | ||
| */ | ||
| const conditionSchema = schema.oneOf( |
There was a problem hiding this comment.
Seems like another great use case for schema.discriminatedUnion('operator', .... Is there any reason we wouldn't want to use it here?
| export const asCodeFilterSchema = schema.oneOf( | ||
| [asCodeConditionFilterSchema, asCodeGroupFilterSchema, asCodeDSLFilterSchema], | ||
| { meta: { description: 'A filter which can be a condition, group, or raw DSL' } } | ||
| export const asCodeFilterSchema = schema.discriminatedUnion( |
There was a problem hiding this comment.
Just leaving a thought here, no need to change anything, but it seems to me like we could simplify things here and just treat each condition as a different type instead of somewhat arbitrarily grouping them in a condition schema (when in reality each of these filters is basically its own "condition").
There was a problem hiding this comment.
I'm not sure I understand, but maybe this is addressed by resolving your other suggestion?
There was a problem hiding this comment.
I think mostly, I guess it just feels somewhat arbitrary to have it broken down as
- asCodeConditionFilterSchema
- singleConditionSchema
- oneOfConditionSchema
- rangeConditionSchema
- existsConditionSchema
- asCodeGroupFilterSchema
- asCodeDSLFilterSchema
- asCodeSpatialFilterSchema
vs. having it flat:
- singleConditionSchema
- oneOfConditionSchema
- rangeConditionSchema
- existsConditionSchema
- groupFilterSchema
- dSLFilterSchema
- spatialFilterSchema
Does that make sense? I'm just not sure the benefit we get from having that extra layer.
There was a problem hiding this comment.
Thanks. I understand now. I'm not strictly opposed to a flatter structure. But I also like have just four top-level filter types for users to choose from, especially if we decide later to add more types of conditions. It feel like it might be easier for the user flow.
flowchart TD
id1{What kind of filter?}
id1 --> id2[Condition]
id1 --> id3[Group]
id1 --> id4[DSL]
id1 --> id5[Spatial]
id2 --> id6{What kind of condition?}
id6 --> id7[is]
id6 --> id8[exists]
id6 --> id9[is one of]
id6 --> id10[range]
id3 --> id11{and/or}
id11 --> id2
id11 --> id3
| /** | ||
| * Type guard to check if condition is a nested AsCodeGroupFilter | ||
| */ | ||
| export function isNestedFilterGroup( |
There was a problem hiding this comment.
Nit: nested with regards to filters can be associated with the nested field type... Can we rename this to avoid confusion?
lukasolson
left a comment
lukasolson
left a comment
There was a problem hiding this comment.
LGTM! Thanks for pushing through these last few changes.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
History
|
6 participants
Fixes #241852
Note: Parts of this code were generated or assisted by Copilot. I have reviewed and validated all code for correctness, security, and compliance with Elastic’s standards.
Summary
Adds bidirectional conversion functions between stored filters (used in saved objects and URL state) and the new AsCodeFilter API format for the Dashboards as Code initiative. This enables seamless migration between the existing filter infrastructure and the new simplified API.
The conversion functions introduced in this PR are not yet used in production. Integrations with Kibana applications like Dashboards, Maps, Lens will be introduced in subsequent PRs.
This PR adds three new packages for As Code Filters.
AsCodeFiltersSchemafrom@kbn/es-query-serverto a new@kbn/as-code-filters-schemapackage.@kbn/as-code-filters-transformspackage. We separate the packages as the schemas are only intended to be used by server code, while the transforms can be used by either server or public code. This separation allows us to avoid bundling large dependencies likeJoiin client side code.@kbn/as-code-filters-constantspackage allows both the@kbn/as-code-filters-transformsand@kbn/as-code-filters-schemapackages to use the same constants without worrying about circular dependencies and forbidden cross-boundary imports.Implementation
Type-First Routing Approach:
meta.typefrom theFILTERSenum for deterministic routing to appropriate conversion functionsmeta.typeas DSL to prevent data lossConversion Functions:
fromStoredFilter- Converts StoredFilter → AsCodeFilter (condition, group, or DSL)toStoredFilter- Converts AsCodeFilter → StoredFilter (preserving metadata for backwards compatibility)Key Features
Complete Filter Type Coverage:
Legacy Filter Migration:
range,match_phrase,exists)migrateFilter()Metadata Preservation:
isMultiIndex(spatial filters),field/params(scripted filters)Date Range Support:
strict_date_optional_time_nanos)now-15mtonow)Advanced Filter Handling:
isMultiIndexand geo_shape query structurenegate: trueReviewers
Since this PR does not make any user-facing changes, one way to test the conversion is by testing the Dashboard integration PR which is a child of this branch specifically created for supporting AsCodeFilters in Dashboards. These conversion functions are used in that PR to return AsCodeFilters in the API response as well as converting AsCodeFilters to legacy Filter objects for backwards compatibility with the Unified Search bar in Dashboards.