Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Security Solution] Swap rule unions out for discriminated unions to …
…improve validation error messages (elastic#171452) **Epics:** elastic/security-team#8058, elastic/security-team#6726 (internal) **Partially addresses:** elastic/security-team#7991 (internal) ## Summary The main benefit of this PR is shown in `rule_request_schema.test.ts`, where the error messages are now more accurate and concise. With regular unions, `zod` has to try validating the input against all schemas in the union and reports the errors from every schema in the union. Switching to discriminated unions, with `type` as the discriminator, allows `zod` to pick the right rule type schema from the union and only validate against that rule type. This means the error message reports that either the discriminator is invalid, in any case where `type` is not valid, or if `type` is valid but another field is wrong for that type of rule then the error message is the validation result from only that rule type. To make it possible to use discriminated unions, we need to switch from using zod's `.and()` for intersections to `.merge()` because `.and()` returns an intersection type that is incompatible with discriminated unions in zod. Similarly, we need to remove the usage of `.transform()` because it returns a ZodEffect that is incompatible with `.merge()`. Instead of using `.transform()` to turn properties from optional to possibly undefined, we can use `requiredOptional` explicitly in specific places to convert the types. Similarly, the `RequiredOptional` type can be used on the return type of conversion functions between API and internal schemas to enforce that all properties are explicitly specified in the conversion. Future work: - better alignment of codegen with OpenAPI definitions of anyOf/oneOf. https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#oneof oneOf requires that the input match exactly one schema from the list, which is different from z.union. anyOf should be z.union, oneOf should be z.discriminatedUnion - flatten the schema structure further to avoid `Type instantiation is excessively deep and possibly infinite`. Seems to be a common issue with zod (microsoft/TypeScript#34933) Limiting the number of `.merge` and other zod operations needed to build a particular schema object seems to help resolve the error. Combining `ResponseRequiredFields` and `ResponseOptionalFields` into a single object rather than merging them solved the immediate problem. However, we may still be near the depth limit. Changing `RuleResponse` as seen below also solved the problem in testing, and may give us more headroom for future changes if we apply techniques like this here and in other places. The difference here is that `SharedResponseProps` is only intersected with the type specific schemas after they're combined in a discriminated union, whereas in `main` we merge `SharedResponseProps` with each individual schema then merge them all together. - combine other Required and Optional schemas, like QueryRuleRequiredFields and QueryRuleOptionalFields ```ts export type RuleResponse = z.infer<typeof RuleResponse>; export const RuleResponse = SharedResponseProps.and(z.discriminatedUnion('type', [ EqlRuleResponseFields, QueryRuleResponseFields, SavedQueryRuleResponseFields, ThresholdRuleResponseFields, ThreatMatchRuleResponseFields, MachineLearningRuleResponseFields, NewTermsRuleResponseFields, EsqlRuleResponseFields, ])); ``` --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
- Loading branch information
1 parent
4048464
commit 6cc415c