Initial batch of minor schema fixes #1744
Conversation
There is no way to tell a validator that a schema is using draft-wright-json-schema-validation-00, which is the only draft with a "uriref" format. Validators either interpret schemas as the previous draft (fge-04), which does not have this concept at all, or the following draft (wright-01) which has "uri-reference". Since there are no options that will automatically work correctly, using the later form seems to be the best. Many validators allow registering extensions, and we can just document that we are using the wright-01+ syntax. You often need to register format handlers to get *any* format validation with many validators anyway, so this does not seem overly burdensome.
Constructs such as:
type: object
not:
type: object
will always fail, so remove the type inside the not.
The "not" usage around "^x-" patterns for responses is
unnecessary, as the existing
patternProperties:
"[1-5]..."
additionalProperties: false
prevents all property that do not start with 1, 2, 3, 4, or 5
from being allowed already.
While draft-wright-json-schema-00 forbids JSON References except where a schema is expected, no validators implement this until draft-writght-json-schema-01. wright-00 schemas are in practice processed as draft-04 schemas, meaning that "$ref" is always considered a JSON Reference. Or, at least, some validators do that. We can work around this by using a pattern that only matches "$ref". There is no problem with the $ref in the "required" keyword, as only "$ref" as a property name is recognized as a reference.
|
@webron or anyone, is this travis-ci failure something I need to worry about? It's not immediately obvious to me what's going on from the logs, if I'm even looking in the right place. |
|
That CI build error can safely be ignored. On this branch the only meaningful thing being validated is the markdown (links etc) and you're not touching that. Thanks for this BTW! |
|
@handrews Do you need each individual commit / combination testing or just the tip of the commits each time? |
|
@MikeRalphson tip of the commits should be fine. It should work at each step if you feel so inspired, but I can't see that that matters too much. |
|
Ideally, you can use an implementation that lets you register extended formats and register a handler for |
|
Yep, I'm using |
|
@MikeRalphson let me know if you would like for me to provide some examples that would have caused problems under things like the |
|
That would be very helpful, as I haven't hit one yet. |
|
@MikeRalphson Here are some ideas- if you have things you can easily tweak to try these, they should fail quickly. If you do not, I can create some actual files tomorrow. Do you have any files that use a non-bearer HTTP security scheme? I don't see how that ever could have validated anything if used. If you have one of those, please paste in the yaml section here of point me to it or send it to me, and I will try to figure it out. Anything like type: http
scheme: Basicshould have failed. For the I'm not exactly sure how to test the change from "uriref" to "uri-reference". If you have any that failed "uriref", they should still fail. Otherwise, what is actually an invalid URI reference? Nearly everything I can think of is still technically valid, just not useful (e.g. it should be a path component but it gets interpreted as a scheme, etc.). Maybe an IRI in its original character set? If the validator will even read one? |
|
@MikeRalphson if we're reasonably certain that this PR hasn't made things worse, can it get merged into the schema branch by Monday? I would like to spend the non-holiday part of next week wrapping this up, and the rest of the PRs should be mostly independent (meaning I can post them all without blocking on each other or stacking up huge numbers of commits in one PR) |
|
I'll try and get to that point by tomorrow's call, I just need to knock together a script to compare results across schemas. |
|
Looking good to me so far (no regressions across my actual OpenAPI 3.0.x definitions). I just need to add in 2.0 to 3.0.x conversion then I will have a lot more samples to throw at this. That need not hold up progression. |
|
@handrews all three schema pass this example: openapi: 3.0.1
info:
version: 1.0.0
title: API
paths: {}
components:
securitySchemes:
mySS:
type: http
scheme: BasicDo I have the wrong end of the stick / wrong stick / no stick? |
|
@MikeRalphson I'm heading out of town for the weekend but I'll look into this when I get back. |
|
@MikeRalphson I'm back from traveling now although I won't be actively working on anything until tomorrow or possibly wednesday. But go ahead and add the fixes (here or on a new PR, whichever seems easier to you). |
…ue across all properties
|
'Tis done! Comments appreciated. |
|
@MikeRalphson I figured out why the security scheme thing passes- the I still recommend keeping the contents of With that solved, I think this is ready to go if you or @webron could merge it to the oas3-schema branch. And then I'll start adding more PRs that can be done in parallel. I've had a horrible cold this whole week so I did not end up working on this as originally planned, which is why it took so long for me to get back and test this out. |
|
@MikeRalphson I thought we had an approval on this. Can we merge it instead of pushing really long commits on top of it? Keeping this open is blocking all further work for me. |
|
Really want an ack/merge from @webron |
|
@MikeRalphson that's totally reasonable, but let's stop making his work harder :-) I know that last commit is structurally simple but now I'm pretty sure it's far more lines on its own than the whole PR was before. I would actually prefer if it could be removed and submitted separately to make this easier to review. |
|
Although I won't insist on it. |
|
Happy to, I just basically wanted to keep the ball moving. When reviewing you can basically skip that commit. |
MikeRalphson
force-pushed
the
schema-fixes
branch
from
775da45 to
0684d65
Compare
|
@handrews I've reset back one commit. |
|
@MikeRalphson thanks! |
|
@MikeRalphson ugh, the build is failing, can you please look into it? This shouldn't affect merging this PR so I'm going ahead with it. |
|
This branch is pre- a number of fixes/simplifications to the Travis build, it can safely be ignored. |
|
Shout if you want a PR to fix Travis on this branch. |
Several small fixes to get started, you can look at each as a separate commit if you want, but here are all of the commit messages for convenience (apologies for the duplication around draft support weirdness, I wrote the messages to each make sense on their own). I tested this by validating it against the draft-04 meta-schema, but have not tried using it to validate any OAS documents yet.
Use "uri-reference", not "url" or "uriref"
There is no way to tell a validator that a schema is
using draft-wright-json-schema-validation-00, which is the
only draft with a "uriref" format. Validators either
interpret schemas as the previous draft (fge-04), which does
not have this concept at all, or the following draft (wright-01)
which has "uri-reference".
Since there are no options that will automatically work correctly,
using the later form seems to be the best. Many validators allow
registering extensions, and we can just document that we are
using the wright-01+ syntax. You often need to register format
handlers to get any format validation with many validators
anyway, so this does not seem overly burdensome.
Fix usage of "not"
Constructs such as:
type: object
not:
type: object
will always fail, so remove the type inside the not.
The "not" usage around "^x-" patterns for responses is
unnecessary, as the existing
patternProperties:
"[1-5]..."
additionalProperties: false
prevents all property that do not start with 1, 2, 3, 4, or 5
from being allowed already.
Use patternProperties for $ref
While draft-wright-json-schema-00 forbids JSON References except
where a schema is expected, no validators implement this until
draft-writght-json-schema-01. wright-00 schemas are in practice
processed as draft-04 schemas, meaning that "$ref" is always
considered a JSON Reference. Or, at least, some validators do
that.
We can work around this by using a pattern that only matches "$ref".
There is no problem with the $ref in the "required" keyword, as
only "$ref" as a property name is recognized as a reference.