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

fix: remove mapping restriction on polymorphic oneof types #685

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cmars
Copy link
Contributor

@cmars cmars commented Feb 4, 2025

This removes the rule that fails on a defined discriminator mapping.

There isn't a good reason to prevent the use of the mapping keyword in Snyk's OpenAPI specifications.

I think this restriction was placed years ago, when Snyk was developing custom code generators, and these couldn't handle the complexity. These haven't been well-maintained at Snyk though, and the open source code generators we're now using, such as oapi-codegen, seem to handle these quite well.

The language discouraging the use of discriminators also hasn't aged well. There are valid reasons and numerous use cases for union type variants in resource attributes.

The Common Issue Model, assets, and findings all benefit from some conditionally-specific variant attributes.

This removes the rule that fails on a defined discriminator mapping.

There isn't a good reason to prevent the use of the mapping keyword in Snyk's
OpenAPI specifications.

I think this restriction was placed long ago, when Snyk had custom code
generators that couldn't handle it. These haven't been well-maintained at Snyk
though, and popular open source code generators such as oapi-codegen seem to
handle it well.

The language discouraging the use of discriminators also hasn't aged well.
There are valid reasons and numerous use cases for union type variants in
resource attributes.

The Common Issue Model, assets, and findings all benefit from some
conditionally-specific variant attributes.
@cmars cmars requested a review from a team as a code owner February 4, 2025 18:33
@jgresty
Copy link
Member

jgresty commented Feb 4, 2025

The mapping objects type is a record of string to string, however the values are considered references. Since the values are not deserialised as refs by vervet, the reference objects are not copied when compiling the specs which lead to invalid specs. This is an issue with the upstream library and is hard to fix on our side.

If you can resolve the above problem with vervet then this rule can be removed, it was added last month to prevent users from encountering this bug.

As for polymorphic types in apis, they may have their uses but the majority of the time such types would be better modelled as seperate resources. This isn't an absolute rule which is why they are discouraged and not disallowed, there will always be some use case for it.

@cmars
Copy link
Contributor Author

cmars commented Feb 4, 2025

@jgresty Thanks for the context, I was unaware of that particular problem with the refs! I'll investigate this further and get back to you.

@cmars cmars marked this pull request as draft February 4, 2025 23:31
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.

2 participants