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

Support for discriminator tag lookup in referenced schemas #2262

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

z6n9n
Copy link

@z6n9n z6n9n commented May 1, 2023

What issue does this pull request resolve?
#2261

What changes did you make?
Support for discriminator tag lookup in referenced schemas

Is there anything that requires more attention while reviewing?
no

@mefellows
Copy link

Is this PR defunct? If you'd consider merging it with changes, I'd be open to contributing the changes back in. We would love to have it support discriminator in pactflow/swagger-mock-validator#51.

let propSch = sch?.properties?.[tagName]
let hasSubSchRequired = false
if (!propSch && sch?.allOf) {
const {hasRequired, propertyObject} = mapDiscriminatorFromAllOf(propSch, sch)

Choose a reason for hiding this comment

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

Re: the GitHubActions warnings, maybe we can rename this key or avoid the destructuring and just use a constant for the returned object and reference each key in the following lines?

E.g.

const discriminator = mapDiscriminatorFromAllOf(propSch, sch)
hasSubSchRequired = discriminator.hasRequired
propSch = discriminator.propertyObject

@zoidyzoidzoid
Copy link

@mefellows What changes would be necessary to get this merged? I'm happy to send another patch with those included.

@mefellows
Copy link

I'm not a maintainer here, I'm just an interested party and are open helping out with direction from the project.

@zoidyzoidzoid
Copy link

Whoops, my bad. I confused contributor of the pactflow repo with this one!

@epoberezkin would you be able to answer that question?

if (subSchObj instanceof SchemaEnv) subSchObj = subSchObj.schema
propSch = subSchObj?.properties?.[tagName]
}
if (propSch) {

Choose a reason for hiding this comment

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

is it possible to move the check out of for? The reason is that currently the first match is applied and it is better to return the last match. Therefore the last override has priority

@lifeiscontent
Copy link

@epoberezkin any chance we could get this merged / looked at? it's been over a year and for openapi specs that use discriminator, mapping is required in a lot of cases due to how ref can potentially be rewritten by Ajv it's not as simple as plugging in the values to a static JSON Schema itself.

@kand
Copy link

kand commented Nov 19, 2024

@epoberezkin checking in as well, since we'd like to use swagger-mock-validator but mapping is an OpenAPI feature we use quite a bit. Could this be merged?

pactflow/swagger-mock-validator#51

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

Successfully merging this pull request may close these issues.

6 participants