Skip to content
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

[Security Solution][Rules] Require all fields to be accounted for in internal schema to response conversion #137628

Merged

Conversation

marshallmain
Copy link
Contributor

Summary

Closes #136910
Related to #136774

A major goal of the security solution rules schema refactor was to catch rule schema mistakes through static typing. Static types now identify many places where new fields need to be added: once a field is added in one place, e.g. the internal rules schema, type errors will appear in other places where the field needs to be handled. However, because the HTTP response schema defined optional fields as truly optional instead of required but possibly undefined, it was still possible to add an optional rule parameter to the rules schema without adding it to the conversion function between internal and response schemas.

This PR solves the issue by implementing a bit of custom logic to replace the use of t.partial when building the response schema. Instead, we take each of the optional fields and replace them with t.union([<optionalFieldSchema>, t.undefined]) so the runtime effect is the same - they're optional - but they must be explicitly set to undefined (or the valid field type) when building a response object.

To test this, try removing one of the optional params, e.g. event_category_override from typeSpecificCamelToSnake in rule_converters.ts. Prior to this PR there was no type error, but with this PR TS reports Property 'event_category_override' is missing in type ....

@marshallmain marshallmain added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Alerts Security Detection Alerts Area Team v8.5.0 labels Jul 29, 2022
@marshallmain marshallmain marked this pull request as ready for review August 1, 2022 18:03
@marshallmain marshallmain requested a review from a team as a code owner August 1, 2022 18:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Detection Alerts Security Detection Alerts Area Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.5.0
Projects
None yet
5 participants