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.

Sign up for GitHub

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

Jump to bottom

feat: add discriminatorOpenApi tag to generate discriminator schemas #1572

Merged
merged 2 commits into from
Feb 22, 2023
Merged

feat: add discriminatorOpenApi tag to generate discriminator schemas #1572

merged 2 commits into from
Feb 22, 2023

Conversation

mdesousa
Copy link
Contributor

@mdesousa mdesousa commented Feb 18, 2023 •
edited by github-actions bot
Loading

Fixes #1551 and related to #1376 by @daanboer

This pr adds the ability to select a common property in a union type to act as an open-api discriminator. The discriminator tag is now supported by OpenAPI, Ajv, and other tools. The tag is not officially supported by json-schema... so there are 2 varieties:

  • the discriminator tag will generate an if-then-else schema that complies with the json-schema spec
  • the discriminatorOpenApi tag will generate a discriminator schema that is supported by some tools but not compliant with json-schema.

References:

Version

Published prerelease version: v1.3.0-next.0

Changelog

🎉 This release contains work from new contributors! 🎉

Thanks for all your work!

❤️ Mario DeSousa (@mdesousa)

❤️ Grant Dickinson (@grant-d)

🚀 Enhancement

  • feat: add discriminatorOpenApi tag to generate discriminator schemas #1572 (@mdesousa)

🐛 Bug Fix

🔩 Dependency Updates

Authors: 6

domoritz
domoritz requested changes Feb 18, 2023
View reviewed changes
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Should this be an annotation or a config option? I feel the latter makes more sense so the consumer can decide, no?

@mdesousa
Copy link
Contributor Author

hi @domoritz , i went with the annotation but it's different from the existing discriminator... this one is discriminatorOpenApi. so this allows the consumer to decide what type of schema they want to generate. furthermore, they could make different choices for different types.

@domoritz
Copy link
Member

How can the consumer decide?

@mdesousa
Copy link
Contributor Author

here's an example:

export type Bird = Eagle | Chicken | Penguin;

/**
 * @discriminator insect_type
 */
export type Insect = Ant | Bee | LadyBug;

/**
 * @discriminatorOpenApi pet_type
 */
export type Pet = Fish | Dog | Cat;

The Bird schema is the traditional with anyOf, the Insect schema generates the if-then-else schema (compatible with json-schema), and the Pet schema uses the OpenAPI discriminator schema

@mdesousa
Copy link
Contributor Author

mdesousa commented Feb 21, 2023 •
edited
Loading

Ah, by consumer you are referring to the app reading the json schema... not the schema author. So you mean that it should be up to the calling code to choose, and that based on that choice we should generate all the discriminator schemas consistently, based on that choice.
I think you are right... if I wanted to feed the same schemas to different tools, i'd like to be in control on how the discriminator is produced. Ok, let me work on that... thanks for the feedback @domoritz .

@mdesousa
Copy link
Contributor Author

ok @domoritz please take a look at the latest commit, let me know if that's what you were thinking

domoritz
domoritz approved these changes Feb 22, 2023
View reviewed changes
test/utils.ts
@@ -49,14 +49,16 @@ export function assertValidSchema(
* @default {strict:false}
*/
ajvOptions?: AjvOptions;
}
},
discriminatorType?: Config["discriminatorType"]
Copy link
Member

Choose a reason for hiding this comment

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

We should use an object for all these arguments now. This would remove the need for having so many undefined. Can you do that in a follow up pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @domoritz yes, i was thinking the same thing... sure i'll submit one. thanks for merging

@domoritz domoritz enabled auto-merge (squash) February 22, 2023 01:19
@domoritz domoritz merged commit 8985251 into vega:next Feb 22, 2023
@mdesousa mdesousa deleted the discriminator-openapi branch February 22, 2023 12:48
@github-actions github-actions bot mentioned this pull request Mar 3, 2023
Merged
@github-actions github-actions bot mentioned this pull request Mar 17, 2023
Merged
@github-actions github-actions bot mentioned this pull request Apr 12, 2023
Merged
@github-actions github-actions bot mentioned this pull request May 26, 2023
Merged
@github-actions github-actions bot mentioned this pull request Jun 25, 2023
Merged
@github-actions github-actions bot mentioned this pull request Jul 4, 2023
Merged
@github-actions github-actions bot mentioned this pull request Aug 2, 2023
Merged
@github-actions github-actions bot mentioned this pull request Aug 14, 2023
Merged
@github-actions
Copy link

🚀 PR was released in v1.3.0 🚀

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domoritz domoritz domoritz approved these changes

Assignees
No one assigned
Labels
released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

discriminator: simpler OpenAPI schema
2 participants
@mdesousa @domoritz