[Security Solution] Refactor duplicated rule schemas #135479
Conversation
…ackaged immutability and default enabled status
…fix patch schema unit tests
marshallmain
added
release_note:skip
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
There was a problem hiding this comment.
Thanks, @marshallmain, fantastic work 👍 Checked and tested locally, and everything works as expected. The schema and API routes code now look so much nicer 🚀
| @@ -130,7 +111,7 @@ describe('patch_rules_type_dependents', () => { | |||
| ], | |||
| }, | |||
| }; | |||
| const errors = patchRuleValidateTypeDependents(schema); | |||
| const errors = patchRuleValidateTypeDependents(schema as PatchRulesSchema); | |||
There was a problem hiding this comment.
Is it possible to adjust the return type of getPatchRulesSchemaMock to avoid schema as PatchRulesSchema type castings?
There was a problem hiding this comment.
Good point, I added an individual type for each rule type patch schema (ThresholdPatchSchema here) and a mock for ThresholdPatchSchema so we can remove this type of cast
| try { | ||
| // TODO: Fix these either with an is conversion or by better typing them within io-ts |
There was a problem hiding this comment.
Awesome, this function became much more readable 🚀
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
| }, | ||
| group: 'default', | ||
| const rulesClient = rulesClientMock.create(); | ||
| const params = getCreateRulesSchemaMock(); |
There was a problem hiding this comment.
Nit, but I think { actions, ...params } is preferable to delete here because it doesn't mutate the returned object.
| if (errors != null || validated === null) { | ||
| throw new PatchError(`Applying patch would create invalid rule: ${errors}`, 400); | ||
| } | ||
| const patchedRule = convertPatchAPIToInternalSchema(params, rule); |
Removes duplicated HTTP detection rules API schemas and builds them instead from a shared schema in
rule_schemas.ts. This removes ~25 places where new fields needed to be added manually as parameters to various functions and destructuring operations. The replacement functionconvertPatchAPIToInternalSchemacentralizes the conversion process between HTTP and backend param formats and also results in a type error if a new field is added to the internal schema but not handled in the conversion function.The result should be much less effort to pass rule params through various functions since they're passed as a single object instead of a set of 40-50 individual fields. It should also be clearer when special logic is being applied to specific rule params since they'll stand out more (e.g.
exceptions_listhas special processing inimportRules()).The shared schema does not set default values as part of the io-ts schema. This is in part because io-ts does not have good support for separating the expected input type of the schema (before applying defaults) from the output type (after applying defaults). Default values come from the
convertCreateAPIToInternalSchemafunction in the new schema system, and are enforced by Typescript becauseconvertCreateAPIToInternalSchemareturns the internal server rule type. This change affects theAddPrepackagedandImportschemas most since they applied default values in the schema previously.