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

[OAS] Support lazy runtime types #184000

Merged
merged 14 commits into from
May 23, 2024

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented May 22, 2024

Summary

Close #182910

Add the ability to declare recursive schemas. Updates @kbn/config-schema to support recursive types. This design follows the underlying pattern provided by Joi: https://joi.dev/api/?v=17.13.0#linkref:

      const id = 'recursive';
      const recursiveSchema: Type<RecursiveType> = schema.object(
        {
          name: schema.string(),
          self: schema.lazy<RecursiveType>(id),
        },
        { meta: { id } }
      );

Since using the .link API we are also using .id which enables us to leverage this mechanism OOTB with joi-to-json for OAS generation (thus we could delete a lot of code there).

I chose to avoid using id originally because I thought it would be simpler if we control more of the conversion in config-schema's case but for recursive schemas and references I think this is a favourable trade off. Open to other ideas!

@jloleysens jloleysens self-assigned this May 22, 2024
@jloleysens jloleysens added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.15.0 Feature:OAS Work or issues related to Core-provided mechanisms for generating OAS labels May 22, 2024
@jloleysens
Copy link
Contributor Author

/ci

Comment on lines 30 to 37
const id = 'recursive';
const object: Type<RecursiveType> = schema.object(
{
name: schema.string(),
self: lazy<RecursiveType>(id),
},
{ meta: { id } }
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit verbose, I would have liked to do something like Zod but with the way Joi.link works this just seemed simplest.

packages/kbn-config-schema/src/types/type.ts Outdated Show resolved Hide resolved
@jloleysens
Copy link
Contributor Author

/ci

.object()
.keys(schemaKeys)
.default()
.optional()
.unknown(unknowns === 'allow')
.options({ stripUnknown: { objects: unknowns === 'ignore' } });

if (options.meta?.id) {
schema = schema.id(options.meta.id);
Copy link
Contributor Author

@jloleysens jloleysens May 22, 2024

Choose a reason for hiding this comment

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

Note:

  • To keep things simple, id is only supported for object types, for now
  • Since using the joi's built-in .id() mechanism we can actually delete quite a bit of code from the downstream OAS generation because joi-to-json understands it and correctly extracts references for us

@jloleysens
Copy link
Contributor Author

/ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test/snapshot, but mostly I was able to delete code from kbn-router-to-openapispec

Comment on lines -23 to -31
it('can convert and record refs', () => {
const ctx = createCtx({ refs: true });
const obj = schema.object({}, { meta: { id: 'foo' } });
const parsed = joi2JsonInternal(obj.getSchema());
const result = ctx.processRef(parsed);
expect(result).toEqual({ $ref: '#/components/schemas/foo' });
expect(ctx.sharedSchemas.get('foo')).toMatchObject({ type: 'object', properties: {} });
expect(metaFields.META_FIELD_X_OAS_REF_ID in ctx.sharedSchemas.get('foo')!).toBe(false);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is handled by joi-to-json now.

@jloleysens jloleysens marked this pull request as ready for review May 22, 2024 13:46
@jloleysens jloleysens requested a review from a team as a code owner May 22, 2024 13:46
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM, great to finally have recursive types/schemas being supported by config-schema!


describe('lazy', () => {
const id = 'recursive';
const object: Type<RecursiveType> = schema.object(
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed: the explicit Type<RecursiveType> typing can be removed 🚀

@jloleysens jloleysens enabled auto-merge (squash) May 23, 2024 06:40
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 2320 2321 +1
serverlessSearch 322 323 +1
synthetics 978 979 +1
total +3

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
@kbn/config-schema 139 141 +2

Async chunks

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

id before after diff
enterpriseSearch 2.7MB 2.7MB -39.0B
serverlessSearch 474.1KB 474.1KB -39.0B
synthetics 1.0MB 1.0MB +104.0B
total +26.0B

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
@kbn/config-schema 19 20 +1
Unknown metric groups

API count

id before after diff
@kbn/config-schema 142 144 +2

History

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

cc @jloleysens

@jloleysens jloleysens merged commit 1cc878f into elastic:main May 23, 2024
17 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 23, 2024
@jloleysens jloleysens deleted the kbn-config/support-recursive-types branch May 24, 2024 11:40
jloleysens added a commit that referenced this pull request May 27, 2024
rshen91 pushed a commit to rshen91/kibana that referenced this pull request May 30, 2024
## Summary

Close elastic#182910

Add the ability to declare recursive schemas. Updates
`@kbn/config-schema` to support recursive types. This design follows the
underlying pattern provided by Joi:
https://joi.dev/api/?v=17.13.0#linkref:

```ts
      const id = 'recursive';
      const recursiveSchema: Type<RecursiveType> = schema.object(
        {
          name: schema.string(),
          self: schema.lazy<RecursiveType>(id),
        },
        { meta: { id } }
      );
```

Since using the `.link` API we are also using `.id` which enables us to
leverage this mechanism OOTB with `joi-to-json` for OAS generation (thus
we could delete a lot of code there).

I chose to avoid using `id` originally because I thought it would be
simpler if we control more of the conversion in config-schema's case but
for recursive schemas and references I think this is a favourable trade
off. Open to other ideas!

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
rshen91 pushed a commit to rshen91/kibana that referenced this pull request May 30, 2024
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 Feature:OAS Work or issues related to Core-provided mechanisms for generating OAS release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for recursive schemas
5 participants