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

[PoC] Add router-based OpenAPI schema generation #164257

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Aug 20, 2023

Summary

I would like to propose an alternative approach to how an OpenAPI spec is generated by Kibana.
In my opinion, there is no better source of our API spec than the API itself ;) So in my opinion we should be relying on our router (https://hapi.dev/) to tell us what routes we have registered in our API (https://hapi.dev/api/?v=21.3.2#-servertablehost) instead of trying to replicate it with custom schemas.
And because our router is responsible for the validation of the requests and responses, again, I believe it's the best source of truth in terms of requests/responses schema.

In this PoC, I have tried to exercise if any tools available today would be able to meet this assumption and I have decided to give it a try https://github.com/hapi-swagger/hapi-swagger Which is dedicated to our API framework.

https://github.com/patrykkopycinski/kibana/blob/feat/router-based-openapi/packages/core/capabilities/core-capabilities-server-internal/src/routes/resolve_capabilities.ts#L25
Zrzut ekranu 2023-08-20 o 12 55 40

It works out of the box for the path that is not using the VersionedRouter, so definitely we would need to make some adjustments to make sure the plugin supports also the rest of the routes.
We could either fork it or rewrite this tool taking into consideration that we have already started adopting https://zod.dev/ for the schema validation, and that hapi-swagger doesn't support hapi-swagger/hapi-swagger#804

That would be the first step, later we could think about ingesting the generated OpenAPI schema on the client side,
and for example, automate the process of generation of react-query hooks by using tools like https://orval.dev/

For more ideas/solutions I highly recommend checking https://openapi.tools/

@tomsonpl
Copy link
Contributor

This is awesome!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Aug 20, 2023

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #7 / CapabilitiesService #preboot() registers the capabilities routes
  • [job] [logs] FTR Configs #12 / cases security and spaces enabled: trial push_case incident recorder server "after each" hook for "should map the fields and add the backlink to Kibana correctly"
  • [job] [logs] Jest Tests #15 / Router Options should default output: "stream" and parse: false when no body validation is required but not a GET
  • [job] [logs] Jest Tests #15 / Router Options should NOT default output: "stream" and parse: false when no body validation is required and GET
  • [job] [logs] Jest Tests #15 / Router Options should NOT default output: "stream" and parse: false when the user has specified body options (he cares about it)
  • [job] [logs] Jest Tests #15 / Router Options throws if validation for a route is declared wrong

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 595.3KB 596.8KB +1.5KB
enterpriseSearch 2.6MB 2.6MB +1.5KB
total +2.9KB

History

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

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @patrykkopycinski ! Very impressive that you got some OAS output with minimal changes 😎

Some qs/comments on the current approach:

  • Kibana routes currently use a custom validation system but hapi-swagger requires we use the hapi router for validation. This will require a refactor or somehow making it work to avoid possible double validation. We also need to apply our pre-validation logic. I'm also unsure how this might work for non-Joi schemas.

  • Existing routes do not have enough info to generate quality OAS docs :( stuff like content type, status code and response schema are all missing. FWIW, the versioned router adds these and can be used to generate more complete OAS, but it won't work OOTB with hapi-swagger as far as I can tell.

  • Adding a route that serves OAS is a great approach! It solves a lot of the setup/startup complexity that exists for some plugins when registering their routes 👏🏻 (e.g., the alert params)

My thoughts on moving forward: it would be useful to integrate this effort with Core's. We'd like to get to providing a way for all plugins to get OAS "for free". The first step is probably to write a document explaining code-first vs spec-first and then breakdown technical details of the approach (IMO code-first is more feasible for covering all of Kibana as you also stated).

Unfortunately we have not started other than these PoCs Pierre and I started a while ago also taking a "code-first" approach to OAS:

Perhaps we can cherry-pick the Joi -> OAS part from hapi-swagger and integrate these PoCs?

@marshallmain
Copy link
Contributor

This is a pretty cool way to extract OpenAPI schemas from JOI-based validation! What would the developer experience look like if we need to provide OpenAPI specs to other teams, e.g. the docs team? Do we have to run Kibana in order to build the OpenAPI schemas?

Also, what would the next steps look like to get complete representations of our schemas as OpenAPI specs? Do we need to refactor most of our schemas? In particular I'm thinking of cases like https://github.com/elastic/kibana/blob/main/packages/kbn-securitysolution-io-ts-types/src/non_empty_string_array/index.ts where we use io-ts and also have custom validation code to verify that the array is not empty. Ideally IMO we'd have some way of encoding the "non empty" requirement for that field in the OpenAPI spec so we can document any extra validation requirements in the public facing docs. Min/max requirements are a common validation pattern in the security solution and elsewhere in Kibana (e.g. rRuleSchema in alerting) so I think it'd be good to have those requirements documented publicly.

Finally, what's the level of effort to add OpenAPI generation capability for the other schema validation libraries that are in use already? io-ts is used in a variety of places, we're starting to work with zod, and kbn-schema is a variant of joi (maybe there are more as well?).

I tend to agree with a number of the comments in #82587 that using OpenAPI specs as the source of truth leads to a more straightforward implementation overall and helps limit our coupling to a specific schema library. In the work so far by @xcrzx to generate schemas with OpenAPI specs as the source of truth, we found that the script to generate code from OpenAPI specs is fairly straightforward (x-pack/plugins/security_solution/scripts/openapi) and can be run without needing to run Kibana. Creating TS schemas for different validation libraries would mostly be a matter of creating a new template like x-pack/plugins/security_solution/scripts/openapi/template_service/templates/schema_item.handlebars for the new library. It's also made it easy to integrate the code generation with CI so that the OpenAPI specs and runtime schemas always stay in sync.

Would it be possible to use this work to create the initial OpenAPI specs for all routes and have it fill in as much information about the fields as possible, but then copy those specs into the Kibana source? We could then manually complete the OpenAPI specs with anything that couldn't be automatically generated from the router (whether because of custom validations, or io-ts usage, or otherwise) and use the OpenAPI specs as the source of truth to generate the runtime schemas moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants