-
-
Notifications
You must be signed in to change notification settings - Fork 240
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
Handle example keyword validation #472
Conversation
@philsturgeon, could you take a look if you find the time? Thanks! |
@lag-of-death sadly this does not solve the problem. You have added an extra conditional which does help hide the symptom of the bug, but it does not solve the feature. We talked about different types of rules because in OAS2 and OAS3 there are various different places the
Your code will simply ignore this, and not do anything about it. In OAS2 only some parameters have the |
@philsturgeon, given this spec (that has your example included): openapi: "3.0.0"
info:
version: 1.0.0
title: Swagger Petstore
description: Swagger Petstore
license:
name: MIT
contact:
name: Test
servers:
- url: http://petstore.swagger.io/v1
paths:
/pets:
get:
summary: Info for a specific pet
description: pets
operationId: showPetById
tags: [pets]
parameters:
- in: query
name: petId
required: true
description: The id of the pet to retrieve
schema:
type: integer
example: "3"
responses:
'200':
description: Expected response to a valid request I get:
But.. hm, yeah, I think I'll try to narrow the validation to places where |
One comment to add to this is as far as I remember the original request asked for |
@chris-miaskowski, thanks, yes, it should be reported as errors |
src/rulesets/oas3/index.json
Outdated
@@ -83,7 +83,20 @@ | |||
"message": "{{error}}", | |||
"recommended": true, | |||
"type": "validation", | |||
"given": "$..[?(@.example)]", | |||
"given": "$..[?(@.example && !@.type)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to suggest that you could try narrowing it down by schema
but then I realized it won't work out of the box. There would still be a corner case where someone has an example but didn't provide a schema.
On a second thought I will suggest it :)
"given": "$..[?(@.example && !@.type)]", | |
"given": "$..[?(@.example && !@.schema)]", |
If I understand this query correctly it will match all 'nodes' that have example
but doesn't have schema
next to it. This could mean two things:
- either the
example
you found is inside theschema
- or the
example
should be next toschema
but thatschema
was not provided!
If it was not provided you will get undefined
but that's fine because you should be able to check that in the actual rule implementation.
src/rulesets/oas3/index.json
Outdated
"summary": "Examples must be valid against their type.", | ||
"message": "{{error}}", | ||
"type": "validation", | ||
"given": "$..[?(@.example && @.type)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
The problem with type
is that I'm not sure there isn't a nasty corner case where that would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lag-of-death very good write up, helped me grasp the context of this PR very quickly!
Re the code I don't see why it wouldn't work but I don't have a prove that it works because it lacks tests ;)
I'm rejecting until tests are added.
I would like to see coverage for the following cases
parameters:
- in: query
name: petId
required: true
schema:
type: integer
example: "3"
^ should give error
parameters:
- in: query
name: petId
required: true
example: "3"
^ should be OK
parameters:
- in: query
name: petId
required: true
schema:
type: integer
example: "3"
^ should give error
parameters:
- in: query
name: petId
required: true
schema:
example: 123
^ should be ok (?)
parameters:
- in: query
name: petId
required: true
schema:
type: object
required:
- a
- b
example: { a: 1 }
^ should give error
parameters:
- in: query
name: petId
required: true
schema:
type: object
required:
- a
- b
example: { a: 1 }
^ should give error
Also there is something interesting I found about MediaTypeObject but I don't think we need to care about this now (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#media-type-object)
Furthermore, if referencing a schema which contains an example, the example value SHALL override the example provided by the schema.
thanks @chris-miaskowski, I will add the tests 👍
I have stumbled upon this, but my understanding was rather that |
src/rulesets/oas3/index.json
Outdated
@@ -83,7 +83,20 @@ | |||
"message": "{{error}}", | |||
"recommended": true, | |||
"type": "validation", | |||
"given": "$..[?(@.example)]", | |||
"given": "$..[?(@.example && !@.type)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we are at it, is there any way to make the json path expression more specific?
I understand $..[?(@.example && !@.type)]
feels very handy, yet it's utterly slow to the point where we have that rule turned off in Studio.
The other issue I see with that expression is the use of function filter expression that accesses the properties (@.example && @.type
). This will pretty much impossible to support when #437 becomes a thing.
If there is no way to make it less general, I'm okay with leaving it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was trying to get more specific and less greedy rules like intentionally pretending with “parameters” so we know we’re looking st parameters and can make the rules more specific seeing as different types of parameters may or may not have a schema keyword, etc.
src/rulesets/oas3/index.json
Outdated
} | ||
} | ||
}, | ||
"valid-type-example": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be a recommended rule, or no?
I'd rather disabled by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make it not recommended until we are more confident with it, and bump it up in v5?
Bang on I need to double check the contents of the message @chris-miaskowski wrote I’m pretty sure it’s not entirely correct. We are definitely not talking about media type objects so don’t worry about that. |
475f2bc
to
6b13f3c
Compare
6b13f3c
to
4ae8cc3
Compare
# Conflicts: # src/rulesets/oas/index.json
|
||
// @oclif/test packages requires @types/mocha, therefore we have 2 packages coming up with similar typings | ||
// TS is confused and prefers the mocha ones, so we need to instrument it to pick up the Jest ones | ||
declare var test: jest.It; | ||
|
||
describe('valid-example', () => { | ||
const s = new Spectral(); | ||
s.setRules({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setRules is a handy new way of doing things that simplifies the API a little, and addRules is deprecated now.
Also the convention here has been for __tests__/{foo}.ts
to match the rule name it is testing, but this test file is now testing two different rules.
@P0lip can explain a bit more about the recent changes in the spectral develop branch if you are not aware of what has happened in PRs like #430.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I don't know how to load a single rule with loadRuleset
, is it possible @P0lip ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said setRules not loadRuleset. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yes, sorry. My bad
Bummers! Yes, you are right I looked at the incorrect doc! OAS3 says
|
@chris-miaskowski, from the spec:
from this PR's description:
bottom line: didn't care about Also, I think that |
@chris-miaskowski The Encoding Object lives inside the Media Type Object which does have example and examples but remember that this PR is not about Media Type Object example/examples. That example is the This is about the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we end up with >5 rules? 🤔
@@ -78,6 +78,111 @@ | |||
} | |||
} | |||
}, | |||
"valid-oas-example-in-parameters": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoah, whoah, whoah. Why so many rules.
We could create a custom function and utilize functionOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philsturgeon, I only added rules, like you said - no new functions. But maybe again I'm mistaken? cc @P0lip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have a valid-oas-example
rule and simply instrument it using options, like so
# your ruleset
valid-oas-example: [error, {
in: [parameters, headers, content] # etc
}]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry @P0lip, I don't quite follow, where ^ would go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip there are a lot of different ways in which examples can show up, so we were trying to get them done sharing the same generic schemaPath function, as the only difference between most of the types of differences are example and schema are siblings, or example is nested inside schema, or both, and that's just parameters.
This PR ignores Media Type Object example
or examples
as explained elsewhere.
@P0lip this PR has been going on for a long time and people have wandered in and started asking lots of questions, then we've explained it, then we have started the process over again with another person. You should definitely get caught up but doing it all over text would take a fairly long time. If you two can have a call (or I can jump in too tomorrow) then it might be easiest.
parameters:
- in: query
name: petId
required: true
description: The id of the pet to retrieve
schema:
type: integer
example: "3"
example: "5"
Check the tests to see all sorts of examples of parameters in different places.
If you think it'll be drastically better for performance we can switch approaches another time and make osOas3ParameterExamples
look for example
in parameter objects and osOas3SchemaExamples
that will go through all schema's that have examples nested anywhere in them or whatever.
There's a million ways to do this we just gotta pick one that works and that isn't slow. The performance issues were discovered after this approach was decided upon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance issues were discovered after this approach was decided upon.
I have known for at least a month or so. I might have not communicated that though, so it's on me.
Yeah, I understand the background behind this PR and I and don't really want to prevent this PR from being merged. I am aware lots of folks are waiting for that. On the other hand, we are introducing plenty of new rules that will be disabled in Studio anyway until the performance thingy is completed, which, by the way, might require these rules to be rewritten anyway.
Let's sync tomorrow in the morning or so.
I believe a custom function based approach shouldn't take longer than half of a day / day, so we should be fine after all. Either way, as I said, I won't block this PR.
If you all really want to ship it, I'm fine with it. I'll just create a tech debt ticket to be addressed in 4.2 or 5.0 in such a case.
"recommended": true, | ||
"severity": 0, | ||
"type": "validation", | ||
"given": "$..parameters..[?(@.example && @.schema)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paths are too loose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip, what path for OAS3 schema
next to example
in parameters
you'd rather see? I can't think of anything better atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know, but this is going to be utterly slow.
@philsturgeon can you think of anything better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering about this. Would it be better performance wise to:
- run on all parameters then check for example and schema keys
- run on all parameters that have an example then check for schema
Or is it the $..parameters
bit that's the problem? There is an issue where #272 points out there are different types of parameters in the Link Object so we don't want to double up on that mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it the $..parameters bit that's the problem?
Yeah, it's $..parameters
that is the problem, if we could narrow it down a bit, that would be great.
I don't think we are interested in parameters under info object, are we?
If I am not mistaken, parameters are bound to an endpoint/path, so we could at least have paths there, i.e. $.paths..parameters
.
I truly doubt we aim to validate every property with parameters
key, so let's limit the scope as much as we can.
hi @P0lip For OAS2: Parameters can be found in:
For OAS3: Parameters can be found in:
So, it seems like 3 rules (or two, one with What I could do for this PR is to have these two rules instead of one. But I think it would be best to create that tech debt card and handle performance issues there, not here. Maybe we can take a look at spectral/src/rulesets/oas/index.json Line 345 in 129a23d
LMK :) |
|
@P0lip, tests:
|
@P0lip let's guide this one in for a landing and improve in v5.0 with cleverly made performant OAS2 and OAS3-specific custom functions. Can you make a ticket for that? |
Coolio. Will do today. |
@P0lip, @philsturgeon, @chris-miaskowski, can anyone approve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is good to go.
I'll leave the call to @philsturgeon once he addresses the comments I left.
"valid-example-in-parameters": { | ||
"description": "Examples must be valid against their defined schema.", | ||
"message": "{{error}}", | ||
"recommended": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the rule have to be recommended? @philsturgeon
It's expensive, so if we can have it disabled by default, that'd be great.
If you find it particularly useful, I'm fine with leaving it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty important feature, people have reported spectral as broken when its not run, so leaving it to only people who need to know about it and turn it on seems... unideal. People can disable slow rules if they want but honestly Studio is probably one of the few scenarios where speed matters? (That comment may not age well, but for right now...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can disable slow rules if they want but honestly Studio is probably one of the few scenarios where speed matters?
Yeah, it's because of the UI. In case of CI it usually doesn't really matter whether linting takes 5 seconds or 20 seconds... unless you run out of memory which is what the issue is all about.
So in the end, if one lints a huge document, such as Sendgrid or Stripe, it will matter, as you will inevitably run out of memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, lets get this feature in and it can be a bit slow, as we know we are working on #437 anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, let's just merge it.
I'll create a follow up ticket for this feature today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone has their computer catch on fire im not paying the bill, but hopefully they can give us some beefy description docs to play with and we can use them for benchmarking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone has their computer catch on fire im not paying the bill, but hopefully they can give us some beefy description docs to play with and we can use them for benchmarking.
😆 we should be fine.
4.2 or 5.0 will certainly come with optimized performance, and in the meantime one can always disable the rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks okay to me.
I know it must have been a tough journey for you Matt, but you did it, so great job 💪
@chris-miaskowski, can you approve / can I dismiss the request for changes? |
@lag-of-death yeah you can dismiss old reviews if you're confident you've done the thing. |
the concerns are addressed
* feat: validate example keyword * tests: adjust tests * test: fix tests * test: fix tests * test: add tests * test: add tests * refactor: change it to test * refactor: remove tests
* feat: validate example keyword * tests: adjust tests * test: fix tests * test: fix tests * test: add tests * test: add tests * refactor: change it to test * refactor: remove tests
Fixes #223 .
Checklist
Does this PR introduce a breaking change?
Additional context
TL;DR
This PR adds all that is needed to validate
example
against itsschema
in both OAS2 and OAS3. Validation ofexamples
is not handled. Also validation ofexample
against encoding (OAS3) is also not handled.Findings:
I have not found a need for separate rules for example(s) in different places (like in a header, in body etc). The added rule and the adjusted one handle
example
validation against its schema. But, OAS3 talks about validatingexample(s)
against itsencoding
. This is vary vague. Encoding, it seems, is defined inEncoding Object
andEncoding Object
is only to be found inMedia Type Object
(so in Response/Request Object(s)). This might need further investigation and maybe should be handled later (not in this PR).If we are to validate
examples
, then there is a possible need for additional rules. Couldn't work out anything to validateexamples
with the current rules andjson-path-plus
magic.OAS2
example
keywordThe only place that allows
example
keyword is inside ofSchema Object
. Schema Object also allowsrequired
keyword. But thisrequired
keyword is an array, so we have no problem withspectral/src/functions/schema.ts
Line 36 in 4307a0c
because the required attribute will never be a boolean. It will always be an array (just because it is Schema Object, which is a set of JSON Schema.
Required attribute is also found in Parameter Object, but Parameter Object does not allow an example keyword. So there is no conflict there.
So, it seems to me, that the greed of
spectral/src/rulesets/oas2/index.json
Line 98 in 027664f
does not pose a problem and it's OK to leave it as is.
Links:
parameter object: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields-7
schema object: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#schema-object
examples
keywordTo be found only in Response Object.
from: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#example-object
I couldn't find a way to validate
examples
in any way with the existing rules.A spec
I'm attaching an OAS2 example with some comments to further clarify, that there is no issue with the existing OAS2 rule:
OAS3
example
key to be found in:Not sure about "encoding properties" ?
from: https://swagger.io/docs/specification/media-types/
required
key to be found in:the conflict
Parameter object (and header object), where
required
field is a Boolean andexample
is allowed. This PR solves this.examples
keywordAll
examples
are built withExample Object
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#example-objectTo be found in:
parameter object ( https://swagger.io/docs/specification/adding-examples/ and https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#parameter-object )
Media Type object https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#media-type-object in:
a) requestBody ( https://swagger.io/docs/specification/adding-examples/ )
b) response body: ( https://swagger.io/docs/specification/adding-examples/ )
The same as with OAS2, I couldn't find a way to validate examples in any way with the existing rules (for OAS3).