-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[HTTP/OAS] zod
support
#186190
[HTTP/OAS] zod
support
#186190
Conversation
packages/kbn-router-to-openapispec/src/generate_oas.test.util.ts
Outdated
Show resolved
Hide resolved
packages/kbn-router-to-openapispec/src/oas_converter/zod/lib.test.ts
Outdated
Show resolved
Hide resolved
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 added a couple of commits (mostly deleting code), otherwise this is looking in pretty good shape!
We'll have to get someone else from Core to do a final final review. But let's make sure CI is happy first :)
packages/kbn-router-to-openapispec/src/__snapshots__/generate_oas.test.ts.snap
Show resolved
Hide resolved
packages/kbn-router-to-openapispec/src/generate_oas.test.shared.fixture.ts
Outdated
Show resolved
Hide resolved
packages/kbn-router-to-openapispec/src/__snapshots__/generate_oas.test.ts.snap
Outdated
Show resolved
Hide resolved
packages/kbn-router-to-openapispec/src/oas_converter/zod/lib.test.ts
Outdated
Show resolved
Hide resolved
/ci |
/ci |
/ci |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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.
Approach looks good to me, no major concerns in the PR's current state
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.
After this PR is merged, I will update our observability API guidelines to use Zod 🎉
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.
LGTM
|
||
import { schema } from '@kbn/config-schema'; | ||
import type { ZodType } from '@kbn/zod'; | ||
import { schema, Type } from '@kbn/config-schema'; |
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 Type can be imported as a type
@@ -119,6 +120,8 @@ export class RouteValidator<P = {}, Q = {}, B = {}> { | |||
): T { | |||
if (isConfigSchema(validationRule)) { | |||
return validationRule.validate(data, {}, namespace); | |||
} else if (isZod(validationRule)) { | |||
return validationRule.parse(data); |
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.
Namespace is not passed 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.
Yeah, this is part of why Zod is not a drop-in replacement for Joi, no passing context down via the parse method.
import type { OpenAPIV3 } from 'openapi-types'; | ||
// eslint-disable-next-line import/no-extraneous-dependencies | ||
import zodToJsonSchema from 'zod-to-json-schema'; | ||
import { KnownParameters } from '../../type'; |
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 can be imported as a type
return new Error(`[Zod converter] ${message}`); | ||
}; | ||
|
||
function assertInstanceOfZodType(schema: unknown): asserts schema is z.ZodTypeAny { |
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.
asserts
? TIL ❤️
if (instanceofZodTypeKind(type, z.ZodFirstPartyTypeKind.ZodLazy)) { | ||
return unwrapZodType(type._def.getter(), unwrapPreprocess); | ||
} | ||
if (instanceofZodTypeKind(type, z.ZodFirstPartyTypeKind.ZodEffects)) { |
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.
Same processing for the 3 scenarios? Seems odd.
Are there any other options that don't require unwrapZodType
?
Perhaps it'd be interesting to highlight / illustrate them somehow.
if (zodSupportsCoerce) { | ||
if (!instanceofZodTypeCoercible(subShape)) { | ||
throw createError( | ||
`Input parser key: "${shapeKey}" must be ZodString, ZodNumber, ZodBoolean, ZodBigInt or ZodDate` |
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.
Neither in instanceofZodTypeCoercible
nor ZodTypeCoercible
(above) you don't have a condition for ZodString
. Is that normal?
There's quite a lot of logic for |
Good catch @gsoldevila ! Yes there is, this code was copy-pasta'd largely from there. Kinda being treated as a "black box" that we can open if/when we need to. Because we want to do a few custom things with our OAS, we just cherry-picked the parts that convert the runtime schema to JSONSchema. |
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.
Pre-approving as most of my remarks are related to copy-pasted code
@gsoldevila Thanks for reviewing this PR! Regarding the OAS convertor, we will need to revisit it when we use Zod in our products. During my investigation, I noticed we are missing some features, and we need to verify the convertor's result in an actual scenario. So, I will merge this PR, and we can check your comments in a follow-up PR. |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
Unreferenced deprecated APIs
History
|
Summary
Enable Core's router to directly accept
@kbn/zod
runtime validation schemas for validation and generate OpenAPI specification (OAS) from them.Rationale
We have enabled a code first approach to OAS based on runtime validation (see epic) and would like to ensure that this platform capability is available to the maximum number of Kibana developers. Kibana developers use runtime validation libs other than
@kbn/config-schema
for a variety of reasons. Generally they fall under these themes:io-ts
'sis
function). Even if, arguably, there are no strong technical reasons for this approach teams have built certain practices and expectations around them.@kbn/config-schema
is not suitable for this due to it's raw size (not a problem server side of course).zod
stands out among runtime validation libraries in Kibana (big 3 are@kbn/config-schema
,zod
andio-ts
).zod
offers a pragmatic API and the ability to derive JSONSchema for generating OAS.zod
is fairly widely adopted in the broader JS ecosystem and gets regular fixes and updates. For these reasonszod
is a good fit for Kibana's API needs and likely the library we'd choose today in some parallel universe.Contributions welcome
This PR was productionized and driven forward largely by @maryam-saeidi 's efforts and investigation into o11y team's adoption of
@kbn/config-schema
and a subsequent investigation (based on this PR) intozod
. If teams have specific needs from either@kbn/config-schema
orzod
please consider making an upstream contribution before picking yet another runtime validation lib!!What about
@kbn/config-schema
?The general advice for (new) developers is to continue using
@kbn/config-schema
.Core's position is that we will continue to recommend
@kbn/config-schema
as the primary, first-class-citizen runtime validation library for all our purposes (SO schema, configuration, API schema) and consider it in long-term maintenance mode. At the same time, we recognise thatzod
is viable alternative for HTTP APIs that can help increase (o11y) team's investment in Kibana platform-provided capabilities like OAS generation from code.Usage
Here is an example of using Zod in SLO delete API.
Risks
@kbn/zod
to Kibana router declarationszod
this seems unlikely and it is always possible for us to contribute tozod
directly. The trade-off with value unlocked for Kibana developers seems worth it in this case. Additionally, we wrapzod
as@kbn/zod
which enables us to shim/fix/improve a large number of aspects ofzod
like any missing OAS features.zod
supports or will support in future (e.g.transform
API).Notes
joi
withzod
is a nice goal, but has proven practically infeasible due to their differences in API and behaviour: unfortunately@kbn/config-schema
is a leaky wrapper aroundjoi
.Follow up work