[* as Code] Migrate to @kbn/zod#268329
Conversation
8e810b9 to
ade88c6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
nickofthyme
left a comment
There was a problem hiding this comment.
Comments for reviewers
| params: zod.object({ | ||
| x: zod.coerce.number(), | ||
| y: zod.coerce.number(), | ||
| z: zod.coerce.number(), | ||
| }), |
There was a problem hiding this comment.
The behavior of schema.number is more aligned with z.coerce.number but in many use cases this does not actually matter. This is only necessary for params and query schemas as these are passed as strings.
| Value | z.number() |
z.coerce.number() |
schema.number() |
|---|---|---|---|
0 |
0 |
0 |
0 |
'0' |
Error | 0 |
0 |
1 |
1 |
1 |
1 |
'1' |
Error | 1 |
1 |
'1.23' |
Error | 1.23 |
1.23 |
1.23 |
1.23 |
1.23 |
1.23 |
'two' |
Error | Error | Error |
false |
Error | 0 |
Error |
true |
Error | 1 |
Error |
| // For now, we use `z.any()` to allow flexibility in the params field. | ||
| params: z.any().optional(), |
There was a problem hiding this comment.
Using schema.any() inside a schema.object() actually optionalizes that property where it is not required. But doing so with zod requires you explicitly pass undefined as a value.
// @kbn/config-schema
const testType1 = schema.object({ a: schema.any() });
type TestType1 = TypeOf<typeof testType1>; // { a?: any; }
// @kbn/zod
const testType2 = z.object({ a: z.any() });
type TestType2 = z.output<typeof testType2>; // { a: any; }So schema.any() used as a prop is replaced with z.any().optional().
| color: z | ||
| .union([colorByValueSchema, noColorSchema, autoColorSchema]) | ||
|
|
||
| .default(AUTO_COLOR) | ||
| .optional() |
There was a problem hiding this comment.
The .optional() modifier always comes after .default() otherwise the type is not inferred as undefined/optional.
const testType1 = z.number().default(0).optional(); // number | undefined
const testType2 = z.number().optional().default(0); // numberReally these should not be mixed at all because .optional() wipes away the applied .default() but for now this is to best match the current schemas.
| }) | ||
| .meta({ | ||
| id: 'gaugeNoESQL', | ||
| title: 'Gauge Chart (DSL)', | ||
| description: 'Gauge configuration using a data view.', | ||
| }); |
There was a problem hiding this comment.
The options from schema.object are inherited when using .extends such that...
const mySchema = schema.object({}, { unknowns: 'forbid' });
const mySchemaCopy = mySchema.extends({}); // still forbids unknownsThis is not true with zod, the strictness is not passed on using .extend.
const mySchema = z.object({}).strict();
const mySchemaCopy = mySchema.extend({}); // no longer strictBecause of this we need to explicitly carry forward the strictness/looseness of any extended object schema.
const mySchema = z.object({}).strict();
const mySchemaCopy = mySchema.extend({}).strict(); // also strict| // TODO: enforce Serializable type, see https://github.com/elastic/kibana/pull/269196 | ||
| config: z.object({}).loose() as z.ZodType<{}>, |
There was a problem hiding this comment.
The loose equivalent in zod actually has more correct typings than config-schema as shown here...
// @kbn/config-schema
const testType1 = schema.object({}, { unknowns: 'allow' }); // {}
// @kbn/zod
const testType2 = z.object({}).loose(); // { [x: string]: unknown; }The issue is that these type feed into Serializable and get passed around as generic object type.
I'll address this in a follow up PR. See #269196 for more details.
| export type VisualizationConfig = ReturnType< | ||
| ChartTypeRegistry[SupportedChartType]['schema']['validate'] | ||
| >; | ||
| export type VisualizationConfig = z.output<ChartTypeRegistry[SupportedChartType]['schema']>; |
There was a problem hiding this comment.
This type was previously any as typeof chartTypeRegistry was explicitly defined as Record<SupportedChartType, ChartTypeRegistryEntry> so none of the schemas were inferred.
We could use satisfies on chartTypeRegistry but that overly narrows the type for each supported chart type where it would be missing undefined values from ChartTypeRegistryEntry. Defining the explicit type is the best alternative.
| export function mergeAllMetricsWithChartDimensionSchema<T extends Props>( | ||
| baseSchema: T, | ||
| context: string | ||
| ) { | ||
| return schema.oneOf([ | ||
| mergeWithSimpleMetrics(baseSchema, context), | ||
| formulaOperationDefinitionSchema.extends( | ||
| baseSchema, | ||
| ctxMeta(context, 'Formula', METRIC_OP_TITLES.formula) | ||
| ), | ||
| export function getMetricsWithChartDimensionSchema(context: string) { | ||
| return z.union([ | ||
| getSimpleMetricsSchema(context), | ||
| formulaOperationDefinitionSchema.meta(ctxMeta(context, 'Formula', METRIC_OP_TITLES.formula)), |
There was a problem hiding this comment.
These metric schemas were a large strain on the TS inferred type depth limit with the nested extending of a generic baseSchema. This simplifies it and relies on the .and method of zod to intersect each of these.
I think this will also help to optimize the OAS output such that this...
const testSchema = z.object({ a: z.string() }).meta({ id: 'test-schema' });
const extendedSchema = testSchema.and(z.object({ n: z.number() }))...outputs extendedSchema as this...
{
"allOf": [
{ "$ref": "#/components/schemas/test-schema" },
{
"type": "object",
"properties": {
"n": { "type": "number" }
},
"required": ["n"]
}
]
}Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>
nreese
left a comment
There was a problem hiding this comment.
kibana-presentation changes LGTM - Thanks for taking on such a large project. Great to see no changes in public code for generated types.
Code review only
Did not review filters schemas - @nickpeihl can you review those?
There was a problem hiding this comment.
Reviewed the @kbn/config-schema → @kbn/zod migration. Most systemic concerns (.strict() parity, z.coerce/stringbool for query params, z.any() optionality, strict-intersection issues in chart schemas) are already well-covered by prior review threads. One additional concrete finding on the drilldown registry, inline.
Generated by Claude Reviewer for #268329 · 511.2 AIC · ⌖ 11.7 AIC · ⊞ 4.6K
| ), | ||
| type: z.literal(type), | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Strict .and() intersection rejects valid drilldown configs at runtime — same failure mode already flagged for the chart schemas (getMetrics...().and(...)), but here it hits registered drilldowns.
Zod validates both sides of an intersection against the full input. drilldownSetup.schema can be a .strict() object — urlDrilldownSchema in x-pack/platform/plugins/private/drilldowns/url_drilldown/server/schemas.ts is defined with .strict(). So when this combined schema validates a real drilldown value { url, encode_url, open_in_new_tab, label, trigger, type }, the strict left-hand side rejects label, trigger, and type as unrecognized keys before the right-hand object can accept them, so parsing throws even for valid input.
The previous drilldownSetup.schema.extends({ label, trigger, type }) merged the keys into a single object, so no key was ever "unrecognized". To preserve that behavior, merge the extra keys into the drilldown object's shape (as was done for the chart schemas) rather than intersecting with a strict object — e.g. build the combined schema from drilldownSetup.schema extended with the { label, trigger, type } shape, or ensure the intersected object is non-strict so keys owned by the other side aren't rejected.
AlexGPlay
left a comment
There was a problem hiding this comment.
LGTM, code review only. Thank you for the change 😄!
| - [request body.layout.0]: expected value to equal [horizontal]\n\ | ||
| - [request body.layout.1]: expected value to equal [vertical]' | ||
| expect(response.body.message).toMatch( | ||
| /layout.*horizontal|layout.*vertical|horizontal|vertical/i |
There was a problem hiding this comment.
The trailing |horizontal|vertical disjuncts drop the coupling to the layout field, so this assertion passes on any error message containing just the word "horizontal" or "vertical" — including one from an unrelated field or endpoint. Recommend tightening so the message must also mention the layout field.
See details
The first two disjuncts (layout.*horizontal|layout.*vertical) already cover the intended zod message. The fallback tail is over-permissive: if the route regressed so a different validation fired (e.g. a chart-type or orientation field elsewhere in the request/response pipeline that happens to mention horizontal), this test would still pass without ever exercising the layout enum error path.
Suggested tightening:
expect(response.body.message).toMatch(/layout.*(horizontal|vertical)/i);The same pattern appears at src/platform/plugins/private/links/test/scout/api/tests/update_links.spec.ts:144 — recommend updating both spots.
Share feedback in the #appex-qa Slack channel.
smith
left a comment
There was a problem hiding this comment.
Service map embeddable changes look good
Converts allowHiddenIndicesSchema from @kbn/config-schema (unimported) to z.boolean().optional().meta(), fixing a ReferenceError on startup that caused all discover/as-code jobs to fail. Claude-Session: https://claude.ai/code/session_0132ugcMCNCJZSqUGDsh8pPM
There was a problem hiding this comment.
One additional occurrence of the strict-.and() intersection issue already discussed elsewhere in this PR — this one is on the Lens embeddable's by-value panel schema, which is the schema actually returned to the dashboard panel transform path. Left inline. The rest of the migration matches the documented conversion rules.
Generated by Claude Reviewer for #268329 · 506.9 AIC · ⌖ 11.7 AIC · ⊞ 464
| }); | ||
| export const getLensByValuePanelSchema = (getDrilldownsSchema: GetDrilldownsSchemaFnType) => { | ||
| return lensApiConfigSchema | ||
| .and(getSharedPanelSchema(getDrilldownsSchema)) |
There was a problem hiding this comment.
🟠 Strict .and() intersection rejects every by-value Lens panel at runtime — same failure mode already flagged on gauge.ts, registry.ts, and shared.ts, but it also lands here and this is the schema returned by getSchema for the Lens embeddable (getLensPanelSchema → used in the dashboard panel transform .parse() path).
getSharedPanelSchema(...) is defined with a trailing .strict() (line 85). Zod validates both sides of an intersection against the entire input, so the strict right-hand object rejects any key it doesn't declare. The chart-config keys on the left side (type, metrics, styling, dataSource.*, breakdown_by, ...) are not part of getSharedPanelSchema's shape, so a real by-value Lens config fails validation with Unrecognized key before the union branch can match.
The old extendLensApiConfigSchema(getSharedPanelSchema(...)) merged the shared props into each union branch, so the branch objects (which are non-strict and strip) accepted the combined shape. To preserve that, either drop .strict() from getSharedPanelSchema for the by-value case, or build the union of per-branch extensions rather than intersecting with a strict object. Note getLensByRefPanelSchema (line 93) is unaffected because it uses .extend(...).strict() on a single object.
markov00
left a comment
There was a problem hiding this comment.
This is a pure mechanical port to zod, I don't see discrepancies between the schemas on visualization owned files
| stripUnknownKeys: true, | ||
| } | ||
| ); | ||
| transformedPanelConfig = transforms.schema.parse(transformedPanelConfig); |
There was a problem hiding this comment.
As discussed offline with @nickofthyme, Zod does not support any type of stripUnknownKeys option. On a quick check of all our integration dashboards, I'm seeing a slight increase in cases where transformed panels are leaking invalid or unknown properties from saved object state to the transformedPanelConfig. This causes the .parse method to throw an error.
Fortunately, #270300 (🙏 @Heenawter ) captures these transform errors and drops invalid panels from dashboard rather than throwing a 500 error. In the UI the dashboard loads and shows a warning toast about dropped panels.
So with this change, users may see a slight increase in panel warnings with hand-modified or otherwise munged saved objects. But I think we can look at fixing leaky transforms in subsequent PRs.
cc @nreese
nickpeihl
left a comment
There was a problem hiding this comment.
This is huge and amazing work @nickofthyme! LGTM! Code review and I tested 1371 integration dashboards seeing only a slight increase in panel transform warnings. Usually these saved objects are hand-munged so we can discuss fixing those with the integrations team. None of the integration dashboards returned a 500 error, all return HTTP 200.
| @@ -79,6 +79,14 @@ export const APPROVED_TRIGGER_DEFINITIONS: Array<{ id: string; schemaHash: strin | |||
| id: 'alerting.ruleEnabled', | |||
| schemaHash: '95d5d9bc425bbc2ef5391ee67d280aa42c1acfab230920a482e91d01a02c4a13', | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Apparently this PR triggered a missed task from CI. So good for us 🎉
There was a problem hiding this comment.
Is this a good thing? 🤔 😅
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
cc @nickofthyme |
Summary
Closes #263825
Migrate as-code schemas to
@kbn/zodNotes
Why change all schemas?
With dashboard schemas being the primary consumer of these as-Code schemas, we cannot change parts of the schema without changing all at once or creating a custom schema-config/zod conversion layer. Doing it altogether is tough but necessary for future-proofing as-code.
Reviewing
Guide to reviewing this PR.
There should be minimal changes to the actual schemas as this is meant to be as close to a 1:1 replacement as possible. That said there are a few functional differences namely:
paramsandqueryschemasschema.number()->z.coerce.number(), see [* as Code] Migrate to@kbn/zod#268329 (comment)schema.boolean()->z.stringbool(), see [* as Code] Migrate to@kbn/zod#268329 (comment)schema.object({ a: schema.any() })toz.object({ a: z.any() }), see [* as Code] Migrate to@kbn/zod#268329 (comment).default().optional()vs.optional().default(), see [* as Code] Migrate to@kbn/zod#268329 (comment)schema.nullable()coerces missing orundefinedtonull. But zod's bare.nullable()only allowsX | nulland requires the field to be present. Thusschema.nullable()->z.nullable().default(null).schema.object()is strict by default where it will reject unknowns unless explicitly relaxed. Butz.object()defaults to ignore, so a 1:1 replacement would beschema.object()->z.object().strict().z.object({}).loose(), see [* as Code] Migrate to@kbn/zod#268329 (comment)schema.object(...)with only optional properties, see [* as Code] Migrate to@kbn/zod#268329 (comment)I tried my best to leave everything in order as to limit the diff, but zod syntax is much more concise so git/github has some trouble nicely aligning the diffs.
If there is a mistake in the syntax, it would be a huge help if you comment with the suggested code fix so it can be easily applied.
Checklist
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.
@kbn/config-schema