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] Implement a Zod transformation from snake_case to camelCase and vice versa #180121

Open
Tracked by #179907
jpdjere opened this issue Apr 5, 2024 · 4 comments · Fixed by #181581
Open
Tracked by #179907
Labels
Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture

Comments

@jpdjere
Copy link
Contributor

jpdjere commented Apr 5, 2024

Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174168
Needed for: #180124

Summary

We have API schemas where we use snake_casing for object properties, and we have internal BE-side schemas for rule parameters and saved object attributes where we use camelCasing for object properties. Because of that, we manually transform objects between the two schemas, and every time we add a new rule field, we need to write two schemas for it and two transformations between them

export type AlertSuppression = z.infer<typeof AlertSuppression>;
export const AlertSuppression = z.object({
group_by: AlertSuppressionGroupBy,
duration: AlertSuppressionDuration.optional(),
missing_fields_strategy: AlertSuppressionMissingFieldsStrategy.optional(),
});
export type AlertSuppressionCamel = z.infer<typeof AlertSuppressionCamel>;
export const AlertSuppressionCamel = z.object({
groupBy: AlertSuppressionGroupBy,
duration: AlertSuppressionDuration.optional(),
missingFieldsStrategy: AlertSuppressionMissingFieldsStrategy.optional(),
});

export const convertAlertSuppressionToCamel = (
input: AlertSuppression | undefined
): AlertSuppressionCamel | undefined =>
input
? {
groupBy: input.group_by,
duration: input.duration,
missingFieldsStrategy: input.missing_fields_strategy,
}
: undefined;
export const convertAlertSuppressionToSnake = (
input: AlertSuppressionCamel | undefined
): AlertSuppression | undefined =>
input
? {
group_by: input.groupBy,
duration: input.duration,
missing_fields_strategy: input.missingFieldsStrategy,
}
: undefined;

To simplify writing and maintaining schemas, we should try to implement a reusable Zod transformations from snake_case to camelCase, something like that:

export type SnakeCasedObject = z.infer<typeof SnakeCasedObject>;
export const SnakeCasedObject = z.object({
  nested_object: z.object({
    some_property: z.string(),
  }),
});

export type CamelCasedObject = z.infer<typeof CamelCasedObject>;
export const CamelCasedObject = SnakeCasedObject.transform(toCamelCase);

// Should yield:

/*
type CamelCasedObject = {
  nestedObject: {
    someProperty: string;
  };
};
*/

Notes

Some ideas how to do that can be found in a GitHub issue: colinhacks/zod#486 (comment).

This transformation should:

During implementation, please evaluate if it would be helpful to have a vice versa toSnakeCase Zod transformation, in addition to toCamelCase.

Note that toCamelCase would be in fact a schema and type transformation that would make it easier to define internal schemas that correspond to API schemas. It wouldn't be a data transformation (something we have in rule_converters.ts). To make it easier to write and maintain functions like convertAlertSuppressionToCamel and convertAlertSuppressionToSnake we might additionally want to write reusable functions for transforming data. Functions from lodash could be useful for that: _.camelCase, _.snakeCase, _.mapKeys.

Background

Context from the RFC:

#### Internal rule schema
**The internal rule schema** needs to represent that the `immutable` and the new `ruleSource` field may not always exist, so they must be optional.
_Source: [x-pack/plugins/security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts)_
```ts
export type BaseRuleParams = z.infer<typeof BaseRuleParams>;
export const BaseRuleParams = z.object({
// [...]
immutable: IsRuleImmutable.optional(),
ruleSource: RuleSource.transform(camelize).optional(),
// [...]
});
```
> Notice that in the internal schema we cannot simply reuse the `RuleSource` attribute defined for the API schema, since it should have CamelCase and the API schema uses snake_case. We need to apply a transformation to our Zod type. See this [issue](https://github.com/colinhacks/zod/issues/486#issuecomment-1567296747).

@jpdjere jpdjere added triage_needed Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area labels Apr 5, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@banderror banderror added technical debt Improvement of the software architecture and operational architecture 8.15 candidate and removed triage_needed Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area labels Apr 17, 2024
xcrzx added a commit that referenced this issue May 2, 2024
…181581)

**Resolves: #180121
**Resolves: #180122
**Resolves: #180124

## Summary

As part of the preparatory changes for the work in Milestone 3, we want
to add the new `rule_source` field to the API schema.

- Added `rule_source` as an **optional** property to `RuleResponse`, by
introducing it as an optional property in the `ResponseFields` schema.
- For now, all endpoints should return `undefined` for the `rule_source`
field.
- Added `rule_source` as an **optional** property to `RuleToImport`,
which defines the schema of required and accepted fields when importing
a rule.
- For now, the new `rule_source` field should be ignored in the endpoint
logic.
- Added the `ruleSource` field to the `BaseRuleParams` schema, as an
optional field.
- Implemented a Zod transformation from `snake_case` to `camelCase` for
object keys to reduce code duplication.
@xcrzx
Copy link
Contributor

xcrzx commented May 22, 2024

The approach proposed in the ticket didn't work as expected, see #184004 for more detail. So I am reopening it. We need to investigate other options for automatic case conversation in schemas

@xcrzx xcrzx reopened this May 22, 2024
xcrzx added a commit that referenced this issue May 23, 2024
…Source (#184004)

## Summary

This PR replaces the incorrect Zod schema for the `ruleSource` rule
param.

Previously, the rule source field schema was implemented using a Zod
transformation that automatically converted the snake-cased
`is_customized` attribute to its camel-cased version `isCustomized`.

```ts
const RuleSourceCamelCased = RuleSource.transform(convertObjectKeysToCamelCase);

const RuleSource = z.object({
  type: z.literal('external'),
  is_customized: IsExternalRuleCustomized,
});
```

However, this meant that the expected input type for the schema was
snake-cased, as the transformation happened only after validation.

**Valid payload before:**

```json5
{
  "ruleSource": {
    "type": "external",
    "is_customized": false // <- it should be camel cased
  }
}
```

To overcome this issue, the rule source schema was implemented without
using the transformation (revert
#180121).

**Valid payload after:**

```json5
{
  "ruleSource": {
    "type": "external",
    "isCustomized": false
  }
}
```

### Important Note

This rule param schema change is considered safe because we do not
currently use this field in the code. All values of this field are
currently `undefined`. However, to ensure a Serverless release rollout
without breaking changes, we need to release this schema change before
we start writing any actual data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants