[OAS] Include alerting rule APIs

[jloleysens]

Summary

Includes alerting rule APIs in our OAS snapshots.

How to test

Using bump CLI you can preview the output:

bump preview ./oas_docs/bundle.json
# or
bump preview ./oas_docs/bundle.serverless.json

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[elasticmachine]

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

[jloleysens]

@lcawl previewing this in bump.sh does not render the "Create a rule" operation, any ideas on why that might be 🤔 ?

[lcawl]

@lcawl previewing this in bump.sh does not render the "Create a rule" operation, any ideas on why that might be 🤔 ?

One of the errors returned by the linter is as follows:

1500:24 warning operation-operationId-valid-in-url operationId must not characters that are invalid when used in URL. paths./api/alerting/rule/{id}.post.operationId
2908:24 warning operation-operationId-valid-in-url operationId must not characters that are invalid when used in URL. paths./api/alerting/rule/{id}.put.operationId
...

Per https://docs.stoplight.io/docs/spectral/4dec24461f3af-open-api-rules#operation-operationid-valid-in-url "Seeing as operationId is often used for unique URLs in documentation systems, it's a good idea to avoid non-URL-safe characters."

In the preview, the URL that is derived from the operationId in the OpenAPI document for "get rule details" is "operation-api-alerting-rule-id-0", which seems very likely to be the same as what would be derived from the "create a rule" operationId (/api/alerting/rule/{id?}#0).

Is there some way we can generate a more URL-safe unique operationId for each operation in the document? To ensure that it doesn't change, perhaps the simplest method is to provide it as a value in the code?

[jloleysens]

Thanks for the pointer at @lcawl , I retested with URL encoded operation IDs (c0473d1) and seems to work as expected now.

[lcawl]

Yay, I'm glad the operationId solution worked. There are a few other things picked up by the linter that I think are worth mentioning. Not sure if they need to be resolved in the core functionality or if it's just something incorrect in the alerting API details specifically, but here's what I see:

Empty enum list

error oas3-schema "enum" property must not have fewer than 1 items.

For example:

                                  {
                                    "enum": [],
                                    "nullable": true
                                  }

This missing information shows up in the docs like this:

Optional path parameters

error oas3-schema "required" property must be equal to constant.

For example:

 {
            "description": "The identifier for the rule.",
            "in": "path",
            "name": "id",
            "required": false,
            "schema": {
              "type": "string"
            }
          }

Per https://spec.openapis.org/oas/v3.0.3#parameter-object "If the parameter location is "path", this [required] property is REQUIRED and its value MUST be true" so I suspect that's the cause of this message. The docs preview doesn't seem to mind this, however:

Empty required lists

error oas3-schema "required" property must not have fewer than 1 items.

For example:

                            "required": [],
                            "type": "object"

It looks like this is ignored and non-problematic in the docs, so perhaps it is another case where if we can't omit the empty lists we can just lower the severity of this oas3-schema rule in our linter so it's not considered an error.

[jloleysens]

Empty enum list

Should be fixed 06bc1a3

Optional path parameters

I'm not entirely sure I understand the lint rule in this case. Are optional path parameters not possible in OAS? Kibana does support optional path parameters so not sure how to reconcile those.

Empty required lists

Should be fixed b776259

---

More a q for responseops: In bump and in the diff I noticed that certain fields that are nullable are being marked as required:

vs.

Could it be that these should both be schema.maybe instead of one them being nullable?

---

Would you mind taking another look @lcawl ?

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: ad075c5

Failed CI Steps

Metrics [docs]

✅ unchanged

History

• 💚 Build #226734 succeeded 1303bfe
• 💚 Build #226310 succeeded 6bb2a3a
• 💔 Build #226206 failed c0473d1
• 💛 Build #226028 was flaky e112605

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

cc @jloleysens

[lcawl]

Optional path parameters

I'm not entirely sure I understand the lint rule in this case. Are optional path parameters not possible in OAS? Kibana does support optional path parameters so not sure how to reconcile those.

Yeah, you basically have to have two copies defined: one with the parameter and one without. Per OAI/OpenAPI-Specification#93 (comment) it seems like this will be fixed in the next OAS version and since Bump.sh doesn't seem to have a problem with it, it's probably not worth "fixing" to align with OAS v3.

Would you mind taking another look @lcawl ?

Those fixes all look great, thanks! There is one more that I must have missed in the previous lint results related to some empty responses, so I've created #190265

[lcawl]

IMO this PR can wait for #190265 or I can regenerate the bundles thereafter.

[pmuellr]

Could it be that these should both be schema.maybe instead of one them being nullable? (throttle)

I'm a little surprised to see schema.nullable() used here. We built it specifically for our saved objects, as we can't use maybe because then it's valid to not pass the field, or otherwise call it with undefined. But we need to massage it to null in those cases. We do this as we sometimes do non-full updates, so if the value is undefined, but the persisted value isn't null, that persisted value will stick around even though we wanted it "nulled out".

I would think on an API definition it would basically mean optional, or maybe T | null | undefined. Since I doubt many folks are familiar with schema.nullable(), it would be best to not use it for the APIs ...

[jloleysens]

T | null | undefined

Yeah, the main concern I wanted to raise is cases where T | null where it should be T | undefined or even T | null | undefined. Thanks for sharing that context about SOs. I get the desire to reuse but agree that it may be the incorrect type for the HTTP APIs. Bad JS design decisions live on 😅

FWIW I don't think this is a blocker, but happy to view it as one if you feel differently @pmuellr

[jbudz]

Reverted with a9c4d2f. See https://buildkite.com/elastic/kibana-on-merge/builds/48864

[lcawl]

IMO all of those "errors" should be warnings so I've created a PR (#190470) that changes the severity level of those linting rules (though it's cool to see them incorporated into an automated check)!