Skip to content

Add discriminator support for #35 #390

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

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

FWiesner
Copy link
Contributor

@FWiesner FWiesner commented Mar 23, 2021

Attempt to implement #35 for anyOf and allOf

@stevehu
Copy link
Contributor

stevehu commented Mar 24, 2021

@FWiesner I looked into the implementation and haven't gone through the details on the OneOfValidator for all the logic yet. One thing that I want to point out is we might not need to create a new spec version OPENAPI_V3_0_3. The OpenAPI validator can use either V7 or V201909 for the validation. Is there any reason you want to create a new spec version? Thanks.

@FWiesner
Copy link
Contributor Author

FWiesner commented Mar 24, 2021 via email

@FWiesner FWiesner force-pushed the dev/FWiesner/35 branch 2 times, most recently from 53adf9d to 86c9923 Compare March 25, 2021 20:36
@FWiesner FWiesner marked this pull request as ready for review March 25, 2021 20:46
@FWiesner FWiesner changed the title WIP: Add discriminator support for #35 Add discriminator support for #35 Mar 25, 2021
}
}
allErrors.addAll(errors);
// allErrors.add(renderMissingMatchValidationMessage(at));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevehu I wonder whether we can comment this line in - see the log renderer at the end of the file. At the moment failed anyOf validation doesn't give you an indication that no schema matched

Copy link
Contributor

Choose a reason for hiding this comment

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

We have an issue opened to add more context information to the error messages to ensure they are understandable. If I am correct, the commented line will add an extra error message with more detailed info. Do you think it would be better to change the errors.add(buildValidationMessage(at, DISCRIMINATOR_REMARK)); with the context info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that code line will specifically log an additional line saying that none of the candidates matched. Richer context is certainly nice and might make my commented line obsolete. The question just is when that will come and whether then we could simply remove the line again.

Btw I've commented the line as it has impact on test conditions on bunch of tests

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Let's leave it this way for now. We need to resolve this issue in one shot for all validators.

@FWiesner
Copy link
Contributor Author

FWiesner commented Mar 26, 2021

the implementation has two limitations:
1.) I have not yet implemented a custom keyword to validate the discriminator
2.) the discriminator spec specifies that inline schemas get ignored. So,

Schema:
  properties:
    foo:
      type: string
  required: ["foo"]
  allOf:
    - $ref: "#/components/schemas/ExtensionBaseWithDiscriminator"
    - type: object
       properties:
         bar:
           type: string
       required: ["bar"]

should ignore foo, but enforce bar. Right now foo is not ignored

@stevehu
Copy link
Contributor

stevehu commented Mar 26, 2021

@FWiesner I think the limitations are OK for now as there is still something in discussion from the spec team. Could you please write a small document on how to use the new feature. I am sure a lot of people will start using it after upgrading to the latest specification. Thanks.

@FWiesner
Copy link
Contributor Author

Sure. What format?

@stevehu
Copy link
Contributor

stevehu commented Mar 29, 2021

We have a doc folder and all files are in markdown format and there is a link from the README.md to each feature.

@stevehu stevehu merged commit 415980e into networknt:master Mar 30, 2021
@stevehu
Copy link
Contributor

stevehu commented Mar 30, 2021

@FWiesner
Copy link
Contributor Author

FWiesner commented Mar 30, 2021 via email

FWiesner added a commit to FWiesner/json-schema-validator that referenced this pull request Apr 1, 2021
FWiesner added a commit to FWiesner/json-schema-validator that referenced this pull request Apr 1, 2021
stevehu pushed a commit that referenced this pull request Apr 1, 2021
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