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

fix #894: oas3-valid-(content-)schema-example has problem with nullable #914

Merged
merged 10 commits into from
Jan 20, 2020
Merged

fix #894: oas3-valid-(content-)schema-example has problem with nullable #914

merged 10 commits into from
Jan 20, 2020

Conversation

m-mohr
Copy link
Contributor

@m-mohr m-mohr commented Jan 14, 2020 •
edited by P0lip
Loading

Fixes #894.

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

This PR tries to solve #894.
As the documentation for schemaPath states the sentence below, I guess it's fine to just turn on nullable in ajv without thinking about implications for non OpenAPI documents:

This may never be useful for anything other than the use-case of checking OpenAPI examples are valid

@m-mohr
Copy link
Contributor Author

m-mohr commented Jan 14, 2020 •
edited
Loading

Can someone help and explain why the change results in an additional "meta-schema not available" in the console? That's why the tests are failing, but I don't see a connection between nullable and meta schemas.

Edit: Maybe spectral should use the official OpenAPI schemas as meta schemas when validating? Otherwise you'll probably run in more problems as JSON Schema draft 4 != OpenAPI Schema...

@P0lip
Copy link
Contributor

P0lip commented Jan 14, 2020

@m-mohr
Try setting meta to true

spectral/src/functions/schema.ts

Line 37 in ead745a

meta: false,
.
You might also need to remove a couple of addMetaSchema calls.
LMK how it goes!

@m-mohr
Copy link
Contributor Author

m-mohr commented Jan 15, 2020 •
edited
Loading

Thanks, @P0lip. I tried to even use the OpenAPI Schemas to validate the schemas (see commit 31fc677) as OpenAPI Schemas != JSON Schema, but couldn't properly test it due to #919 so dropped that effort.

Now I'm trying the simpler approach and to just get the JSON Schemas / ajv working with nullable, which seems to succeed. I tried to leave the schema function working also for non OpenAPI stuff by incorporating a version switch. I think this is required anyway in the long run when you really want to validate OpenAPI schemas separately from JSON Schemas as they are not the same, are different between OAS versions and I guess there will be more issues coming up for OAS schema validation in the future.

Let me know what you think about all this.

PS: I'm never used TypeScript before, I hope things are not too "javascripty".
PPS: Build is failing due to something unrelated. Will probably be fixed by running it again.

P0lip
P0lip reviewed Jan 15, 2020
View reviewed changes
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Just one comment regarding usage of 0.
Besides that, looking very solid.
Thanks a lot for the contribution ❤️

src/functions/schema.ts Outdated Show resolved Hide resolved
src/functions/schema.ts Outdated Show resolved Hide resolved
src/functions/schema.ts Show resolved Hide resolved
src/functions/schema.ts Outdated Show resolved Hide resolved
src/functions/schema.ts Outdated
@@ -13,6 +12,8 @@ const betterAjvErrors = require('better-ajv-errors/lib/modern');

export interface ISchemaOptions {
schema: object;
// The oasVersion, either 2 or 3 for OpenAPI Spec versions, or undefined / 0 for JSON Schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The oasVersion, either 2 or 3 for OpenAPI Spec versions, or undefined / 0 for JSON Schema
// The oasVersion, either 2 or 3 for OpenAPI Spec versions, or null / 0 for JSON Schema

Let's go with null instead of 0.
Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose 0 so that it can be used in the ajvInstances object as key (and because I was confused with how to enforce null as default value in TS).

Copy link
Contributor Author

@m-mohr m-mohr Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@P0lip I couldn't get your suggestions working and I'm not familiar with how Optional works, so I tried a bit and the solution committed now works without 0. Please check whether that's okay for you.

Now I need to get #915 fixed so that my CI doesn't fail all the time. ;-)

@m-mohr
Copy link
Contributor Author

m-mohr commented Jan 16, 2020 •
edited
Loading

Please fix the build yourself - this annoying lint rule ("Import sources within a group must be alphabetized.") doesn't tell me how I have to order and it always fails regardless of what I try. I tried 20 different cases now!!

Okay, just ungrouping them helps. What a silly linting rule?!

nulltoken
nulltoken reviewed Jan 16, 2020
View reviewed changes
Copy link
Contributor

@nulltoken nulltoken left a comment

Choose a reason for hiding this comment

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

@m-mohr Wow! You're on a roll!

src/functions/schema.ts
@@ -65,7 +81,8 @@ function getSchemaId(schemaObj: JSONSchema): void | string {
}

const validators = new (class extends WeakMap<JSONSchema, ValidateFunction> {
public get(schemaObj: JSONSchema) {
public get(schemaObj: JSONSchema, oasVersion?: Optional<number>) {
const ajv = getAjv(oasVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip how cheap is it to recreate a new instance in that layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only three instances are created, one for each "schema version" (i.e. oas2, oas3 and json schema). So overhead should be minimal and better than creating one ajv instance per example validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it should be cheap.

@m-mohr
Copy link
Contributor Author

m-mohr commented Jan 17, 2020 •
edited
Loading

@m-mohr Wow! You're on a roll!

I only want spectral to be part of our CI workflow ;-) - still worried about #915 though.

P0lip
P0lip previously approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Looks good!

src/functions/schema.ts Outdated Show resolved Hide resolved
@m-mohr m-mohr mentioned this pull request Jan 17, 2020
Closed
4 tasks
@P0lip P0lip added the t/bug Something isn't working label Jan 20, 2020
@P0lip P0lip merged commit 4725569 into stoplightio:develop Jan 20, 2020
@m-mohr m-mohr deleted the patch-3 branch January 21, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nulltoken nulltoken nulltoken left review comments

@P0lip P0lip P0lip approved these changes

Assignees

@m-mohr m-mohr

Labels
t/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

oas3-valid-(content-)schema-example: Problem with nullable
3 participants
@m-mohr @P0lip @nulltoken