[RAM] Rule API Type Versioning POC - Create Rule

[JiaweiWu]

Summary

Resolves: #157883

Summary:

This PR acts as a POC for how we would want to version our rule API types in preparation for the versioning of our HTTP endpoints.

There is now an ES persisted type (called RuleAttributes, akin to the old RawRule type). This type should never be used at the business logic level as we want the ability not to reference saved object attributes directly in the application. Instead, we should transform this saved-object to its corresponding domain object type.

HTTP types (CreateBodyParams, RuleResponse) are now located in x-pack/plugins/alerting/common/routes/rule with a versioning structure like:

And follows the guideline here: https://docs.elastic.dev/kibana-dev-docs/versioning-interfaces

Domain object types (rule for example) are derived from the config-schema schemas using TypeOf, this was done to facilitate the reuse of validation schemas that we might want to run for strict IO validation, potentially at the rulesClient level.

API:

Only the createRule endpoint has been converted for the sake of this POC, other endpoints might have broken types that will be fixed once we have a finalized pattern for dealing with versioned types.

At the API route level, I think it would be wise to import versioned types in our APIs, this way, it forces the developer to deal with broken types when we have a breaking change to our rule domain object.

The API route level is also responsible for transforming domain objects to response types, usually, this just entails renaming property names from camel case to snake case.

in create_rule_route.ts

import type { CreateRuleRequestBodyV1, CreateRuleRequestParamsV1 } from '../../../../common/routes/rule/create';
import type { RuleParamsV1 } from '../../../../common/routes/rule/rule_response';

...

// In the Handler:
const rulesClient = (await context.alerting).getRulesClient();

const createRuleData: CreateRuleRequestBodyV1<RuleParamsV1> = req.body;
const params: CreateRuleRequestParamsV1 = req.params;

const createdRule: Rule<RuleParamsV1> = (await rulesClient.create<RuleParamsV1>({
  data: transformCreateBodyV1<RuleParamsV1>(createRuleData),
  options: { id: params?.id },
})) as Rule<RuleParamsV1>;

const response: CreateRuleResponseV1<RuleParamsV1> = {
  body: transformRuleToRuleResponseV1<RuleParamsV1>(createdRule),
};

return res.ok(response);

RulesClient -> Create

At the rules client level, we now need to transform saved-objects to the domain objects after we perform CRUD operations on them. We can also consider validating schemas here too since other solutions are using our rules client directly instead of through the API.

I don't think we need to version rules client input and output types since the route level types should deal with breaking changes already. Therefore, we can freely import the latest of any domain object types (Rule, for example)

import { Rule, RuleDomain, RuleParams } from '../types';

The flow is the following:

1. calling rulesClient.create() with API body and params.
2. perform schema validation on params
3. perform other validation not achievable using config-schema
4. perform rule create -> returns a `savedObject` type
5. convert `savedObject` ES type to the domain object type (`RuleDomain`)
6. convert the domain object type to a public domain object type (`Rule`)
7. We can also validate the created rule using config-schema

Open questions:

• Should we validate input params at both the route and the rules client level? I think we should since our rules client is shared.

• How do we want to version and validate rule params? Right now they're typed as generics.

• Should we leave all date fields as string, even for domain objects? Since config-schema is unable to define a Date type.

• Should we support partial rule domain object types at all? I can't really think why we should even for updates, since we need to reconstruct the overall rule to send back to the client.

• Should we have undefined | null types in the domain object? I know we need null specifically for the ES types since there is a different between updating a field as undefined or null, but this shouldn't manifest itself in the business logic.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[pmuellr]

My take on the open questions:

Should we validate input params at both the route and the rules client level? I think we should since our rules client is shared.

I sort of feel like we should. We haven't in the past because we figured TS would protect us, but there are lot of folks casting stuff in Kibana, prolly not going to be LESS of that with versioned APIs :-).

How do we want to version and validate rule params? Right now they're typed as generics.

Would anything that Mike has done with task state help here?

Should we leave all date fields as string, even for domain objects? Since config-schema is unable to define a Date type.

Would Zod help? :-) (I like Zod and I think it's under consideration here). I think worse case we can define these as any, then add a custom validation. We've also extended config-schema before (for nullable), so extending config-schema could be an option. OTOH, I have a dim memory of trying this in the past, and it was problematic - don't remember why.

Should we support partial rule domain object types at all? I can't really think why we should even for updates, since we need to reconstruct the overall rule to send back to the client.

if "domain" only includes types in our public APIs, I think that's fine. Internally, we do need to do some partial stuff, for example when updating runtime status of rules - but I don't think that really matters in this discussion.

Should we have undefined | null types in the domain object? I know we need null specifically for the ES types since there is a different between updating a field as undefined or null, but this shouldn't manifest itself in the business logic.

Might be nice to have no undefineds in the domain objects, but not sure if we might be making use of the difference. Except maybe the partial case I alluded to before.

[JiaweiWu]

@elasticmachine merge upstream

[JiaweiWu]

@elasticmachine merge upstream

[elasticmachine]

Pinging @elastic/response-ops (Team:ResponseOps)

[XavierM]

As expected!!!!

[XavierM]

I think you can delete this one!!!

[JiaweiWu]

Oh so this is being used in the new create_rule file

[XavierM]

instead of using saved_object.ts in the file name we can just do so.ts

[JiaweiWu]

@elasticmachine merge upstream

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 89a5828

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
alerting	595	627	+32

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
alerting	47	48	+1

Unknown metric groups

API count

id	before	after	diff
alerting	619	651	+32

ESLint disabled line counts

id	before	after	diff
enterpriseSearch	14	16	+2
securitySolution	410	414	+4
total			+6

References to deprecated APIs

id	before	after	diff
alerting	101	111	+10

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	15	17	+2
securitySolution	489	493	+4
total			+6

History

• 💔 Build #140429 failed 3fbc107
• 💚 Build #139670 succeeded 587cd34
• 💛 Build #139666 was flaky dcba4c4
• 💔 Build #139665 failed 76af943
• 💔 Build #139625 failed 99ddd04

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