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

fix(oas): improve examples validation #1284

Merged
merged 7 commits into from Aug 24, 2020
Merged

Conversation

P0lip
Copy link
Member

@P0lip P0lip commented Jul 16, 2020

Fixes #901

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@P0lip P0lip added the t/bug Something isn't working label Jul 16, 2020
@P0lip P0lip self-assigned this Jul 16, 2020
@P0lip P0lip marked this pull request as draft Jul 16, 2020

function isObject(value: unknown): value is Dictionary<any> {
// todo: expose a util, so it can be used everywhere
return value !== null && typeof value === 'object';
Copy link
Contributor

@nulltoken nulltoken Jul 20, 2020

Choose a reason for hiding this comment

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

@P0lip or maybe just use lodash isObjectLike?

Copy link
Member Author

@P0lip P0lip Jul 23, 2020

Choose a reason for hiding this comment

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

Yeah, I considered doing it, but hm, tree-shaking feels shaky 😆
Let me see how it impacts the size of the bundled fn.
It's the difference is negligible, I'll go for isObjectLike

@philsturgeon
Copy link
Contributor

philsturgeon commented Jul 21, 2020

These rules have always been hell, due to the incredibly confusing nature of all the different types of examples that can show up anywhere in OAS2, OAS3, and JSON Schema.

It honestly might need custom functions.

Using https://github.com/stoplightio/studio/issues/381 as a reference to all the bajillion different combinations of rule, can we think about how we'd implement what combination of examples to handle:

Schema Objects (for OAS2, OAS3, JSON Schema) when there could be x-example or example or examples or x-examples depending on whatever type of thing is there, and the value is always a bare value, or an array of bare values.

Then there's Media Types, Parameters, etc which are sometimes "Schema like" but not, or might actually have a "Schema" inside them...

No one rule could do everything, and we keep getting bug reports when we miss a certain specific combination, so whatever we do needs to work for everything.

P.S No need to validate externalValue.

@P0lip P0lip force-pushed the fix/support-examples-validation branch from 6a0ba75 to a9658a6 Compare Aug 10, 2020
@P0lip P0lip requested a review from philsturgeon Aug 10, 2020
@P0lip P0lip marked this pull request as ready for review Aug 10, 2020
@P0lip P0lip force-pushed the fix/support-examples-validation branch from 05911df to 97a6b0f Compare Aug 10, 2020
@P0lip P0lip force-pushed the fix/support-examples-validation branch from 97a6b0f to 3db38c2 Compare Aug 10, 2020

const result = this.functions.schema.call(
this,
exampleValue.value,
Copy link
Contributor

@philsturgeon philsturgeon Aug 11, 2020

Choose a reason for hiding this comment

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

Related to my other comment: schema examples can be multiple: true but do not have a value key.

{
  "title" : "Match anything",
  "description" : "This is a schema that matches anything.",
  "default" : "Default value",
  "examples" : [
    "Anything",
    4035
  ]
}

@P0lip
Copy link
Member Author

P0lip commented Aug 14, 2020

@philsturgeon I added oas2-valid-example rule.

@philsturgeon
Copy link
Contributor

philsturgeon commented Aug 18, 2020

Excellent! I'm going to try this out on https://github.com/philsturgeon/examples-example and see how far we get. I'd suggest we use these as a basis for test harness.

Once we've got the ones which are meant to fail to actually fail, we can try them with $ref variants too.

@philsturgeon
Copy link
Contributor

philsturgeon commented Aug 18, 2020

Amazing work here, running this branch on from https://github.com/philsturgeon/examples-examples (the oas3.yaml) I am seeing these errors:

 145:30  error  oas3-valid-content-schema-example    `example` property type should be boolean
 149:30  error  oas3-valid-content-schema-example    `example` property should match format `date-time`
 153:30  error  oas3-valid-content-schema-example    `example` property should match format `url`
 176:20  error  oas3-valid-oas-parameter-example     `example` property should be equal to one of the allowed values: foo, bar
 192:22  error  oas3-valid-parameter-schema-example  `example` property should be equal to one of the allowed values: foo, bar
 204:22  error  oas3-valid-oas-parameter-example     `value` property type should be string
 206:21  error  oas3-valid-oas-parameter-example     `value` property type should be string
 269:25  error  oas3-valid-oas-content-example       `value` property should have required property `id`
 289:25  error  oas3-valid-oas-content-example       `value` property should have required property `user`

The first 5 are in v5.5 but the last 4 are only in this branch. The errors are not wildly useful as they are, but they are catching onto the fact that there are problems in the examples.

204:22 error oas3-valid-oas-parameter-example value property type should be string
206:21 error oas3-valid-oas-parameter-example value property type should be string

        - name: multiple-examples
          description: Valid to its schema
          in: query
          schema: 
            type: string
            enum: [foo, bar]
          examples: 
            the-good:
              value: foo
            the-bad:
              value: 123                       # line 204: it's noticed this isn't a string!
            the-ugly:
              value: [an, array]            # line 206: it's noticed this isnt a string either

It's also noticed some response examples using the examples keywords:

        "500":
          description: Fail, because the example is nonsense
          content:
            application/json:
              schema:
                properties:
                  id:
                    type: integer
                  name:
                    type: string
                  completed:
                    type: boolean
                  completed_at:
                    type: string
                    format: date-time
                required:
                  - id
                  - name
                  - completed
              examples:
                response:
                  value:
                    completed: Not a boolean
                    completed_at: Not a date

So that's cool.

It's still missing this:

      "500":
        description: Fail, because the example is nonsense
        content:
          application/json:
            schema:
              properties:
                id:
                  type: integer
                name:
                  type: string
                completed:
                  type: boolean
                completed_at:
                  type: string
                  format: date-time
              required:
                - id
                - name
                - completed
              # notice how im in the schema object, not next to it
              example:
                id: 1
                completed: Not a boolean
                completed_at: Not a date

I'll have a look at OAS2 too.

@philsturgeon
Copy link
Contributor

philsturgeon commented Aug 18, 2020

For oas2.yaml in that repo I'm seeing these two errors:

 176:30    error  oas2-valid-response-example  `application/json` property should have required property `name`
 193:30    error  oas2-valid-response-example  `application/json` property should have required property `user`

That's two more than the latest release!

Similarly to oas3 feedback, if a response example is an object that sits inside a schema, it doesnt seem to get picked up. This example should fail, but doesn't:

        '500':
          description: 'Fail, because the example is nonsense'
          schema:
            properties: 
              id:
                type: integer
              name:
                type: string
              completed: 
                type: boolean
              completed_at:
                type: string
                format: date-time
            required:
              - id
              - name
              - completed
            # notice how im in the schema object, not next to it
            example:
              id: 1
              completed: Not a boolean
              completed_at: 'Not a date'

OAS2 "property examples" should also fail, but currently do not. These do work in OAS3.

        '501':
          description: 'Fail, because the property example is nonsense'
          schema:
            properties: 
              some-bool:
                type: boolean
                example: not a bool
              some-date:
                type: string
                format: date-time
                example: not a date
              some-url:
                type: string
                format: url
                example: not a URL

In OAS2, parameter examples don't seem to be failing as I suppose we aren't looking for x-example (the only way to make an example for a parameter in OAS2).

  /param-examples:
    get:
      operationId: parameter-example
      description: when parameter examples are inline (no $ref involved)
      tags:
        - parameter-examples
      parameters:
        - name: single-example-good
          description: Valid to its schema
          in: query
          type: string
          enum: [foo, bar]
          x-example: foo
        
        - name: single-example-bad
          description: Not valid to its schema
          in: query
          type: string
          enum: [foo, bar]
          x-example: not "foo" or "bar" so this is bad

@P0lip P0lip requested a review from philsturgeon Aug 20, 2020
@P0lip
Copy link
Member Author

P0lip commented Aug 20, 2020

@philsturgeon I think this should be ready for another round.

@P0lip P0lip force-pushed the fix/support-examples-validation branch from a9fcc27 to 9063a7f Compare Aug 20, 2020
Copy link
Contributor

@philsturgeon philsturgeon left a comment

Amazing work, you nailed every single example in these documents.

I gave this a go on the GitHub OpenAPI in sample-specs, and I'm seeing this output:

image

They've actually got some errors in their doc, and I'll talk to them about that, but why is this first one double reporting for a single error? Is this a $ref issue?

Screen Shot 2020-08-21 at 5 12 07 PM

@P0lip
Copy link
Member Author

P0lip commented Aug 23, 2020

@philsturgeon ah, seems like the same rule targets the same portion of the spec.
I'll tweak them.

@P0lip P0lip changed the title fix(oas): oas3-valid-...-example rules check examples as well fix(oas): improve examples validation Aug 24, 2020
@P0lip
Copy link
Member Author

P0lip commented Aug 24, 2020

@philsturgeon how do you feel about merging some of the rules?
Since given might be an array now, we do not need to have plenty of rules such as oas2-valid-response-schema-example or oas2-valid-parameter-example. What do you think?
As this is a potential breaking change, we might postpone it until 6.0

@P0lip P0lip requested a review from philsturgeon Aug 24, 2020
Signed-off-by: Jakub Rożek <jakub@stoplight.io>
@P0lip P0lip force-pushed the fix/support-examples-validation branch from 4609bf5 to 6d1fda3 Compare Aug 24, 2020
@philsturgeon
Copy link
Contributor

philsturgeon commented Aug 24, 2020

Yeah I thought we could probably get away with some rule merges so long as we can get the right function options into place. I let seems like different targets will need different function options, but if you can get the tests to pass with the changed rules then... go ahead!

Maybe we can do the minimal change now and merge the rules as a follow up in 6.0, like you said.

@P0lip
Copy link
Member Author

P0lip commented Aug 24, 2020

Maybe we can do the minimal change now and merge the rules as a follow up in 6.0, like you said.

I'd need to drop a rule or two and merge them into some existing one.
I'll create a follow-up PR, so that 5.5 can be released without any breaking changes included.

@P0lip P0lip merged commit 66114ef into develop Aug 24, 2020
9 checks passed
@P0lip P0lip deleted the fix/support-examples-validation branch Aug 24, 2020
P0lip added a commit that referenced this pull request Aug 25, 2020
* fix(oas): oas3-valid-...-example rules check examples as well

* feat: specify fields

* feat: oas2-valid-response-example rule

* fix: tweak oas3-*-schema-example rules

* fix: cover more oas2 examples

* fix: merge rule

Signed-off-by: Jakub Rożek <jakub@stoplight.io>
@P0lip P0lip mentioned this pull request Aug 26, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oas3-valid-...-example: Only checks example, but not examples
3 participants