redocly-cli icon indicating copy to clipboard operation
redocly-cli copied to clipboard

Rule no-invalid-media-type-examples not properly evaluated for types using allOf

Open austpaul opened this issue 2 years ago • 8 comments

Describe the bug Rule no-invalid-media-type-examples is not triggered for unevaluated properties for examples of allOf types.

To Reproduce Steps to reproduce the behavior:

  1. Given this redocly.yaml configuration
lint:
  extends:
    - recommended
  1. And this (zipped) OpenAPI file: bikes.yaml.zip
  2. Run this command with these arguments... redocly lint bikes.yaml
  3. See linting output Only one warning is produced:
Example value must conform to the schema: must NOT have unevaluated properties 'unevaluatedProperty2'.

Expected behavior Two warnings should be included in the linting output:

Example value must conform to the schema: must NOT have unevaluated properties 'unevaluatedProperty1'.
Example value must conform to the schema: must NOT have unevaluated properties 'unevaluatedProperty2'.

Redocly Version(s) 1.0.0-beta.112

Node.js Version(s) v18.2.0

austpaul avatar Nov 29 '22 13:11 austpaul

Looks like ajv doesn't run the properties code for schemas that don't have any property keywords.

We can fix it in our fork.

Meanwhile you can workaround this issue by adding empty properties into the Wheel schema:

    Wheel:
      properties: {}
      allOf:
        -  $ref: "#/components/schemas/Tire"
        -  $ref: "#/components/schemas/Rim"

RomanHotsiy avatar Nov 29 '22 15:11 RomanHotsiy

@RomanHotsiy are you saying Ajv doesn't evaluate allOf unless type: object and properties keywords are included? I think this is related to our prior discussion of using Ajv 8+ and passing the options. In order to maintain full JSON Schema spec compliance, you should pass strict: false without any other options related to strict mode. I think you include strictSchemas: true which is probably where this is failing because he expects every instance to declare the type with that option setting. Ignoring those schemas is not JSON Schema spec compliant behavior.

jeremyfiel avatar Jan 25 '23 21:01 jeremyfiel

are you saying Ajv doesn't evaluate allOf unless type: object and properties keywords are included?

@jeremyfiel no, I'm saying AJV doesn't run the code for determining unevaluatedProperties if the schema doesn't have any property-related keywords (properties, additionalProperties):

Wheel:
      allOf:
        -  $ref: "#/components/schemas/Tire"
        -  $ref: "#/components/schemas/Rim"

We force unevaluatedProperties in AJV but the corresponding code is not run for the Wheel schema. We can fix it.

We don't use any strict mode settings, and we don't use strictSchemas: true, see here: https://github.com/Redocly/redocly-cli/blob/main/packages/core/src/rules/ajv.ts#L17

RomanHotsiy avatar Jan 26 '23 09:01 RomanHotsiy

I'm also running into this issue. My schema looks something like this though: 

Thing:
  type: object
  properties:
    type:
      type: string
    id:
      type: string
    attributes:
      type: object
      allOf:
        - properties:
            tags:
              type: array
        - '$ref': 'shared.yml#/DefaultFields'

When I put additional, unexpected properties inside attributes in an example, the no-invalid-media-type-examples rule does not return an error.

However, the workaround of doing:

Thing:
  type: object
  properties:
    type:
      type: string
    id:
      type: string
    attributes:
      unevaluatedProperties: false # added this -- properties: {} also works
      type: object
      allOf:
        - properties:
            tags:
              type: array
        - '$ref': 'shared.yml#/DefaultFields'

Gets no-invalid-media-type-examples to return something like this: Example value must conform to the schema: `attributes` property must NOT have unevaluated properties `something-extra`.

samanthawritescode avatar Jan 10 '24 23:01 samanthawritescode

I'm getting the error to display when I add additional properties without using properties: {}.

Which version of the CLI are you using? I'm on 1.5.0


1. openapi: '3.1.0'
2. info:
3.   title: Title
4.   version: '1.0.0'
5. servers: 
6.   - url: 'https://api.redocly.com'
7. security: [] 
8. paths:
9.   /api/v1/things/{thing-id}:
10.     get:
11.       summary: a summary
12.       description: delete with requestBody
13.       parameters:
14.         - name: thing-id
15.           in: path
16.           required: true
17.           schema:
18.             type: string
19.       responses:
20.         '200':
21.           description: OK
22.           content:
23.             application/json:
24.               schema:
25.                 type: object
26.                 required: 
27.                   - attributes
28.                 properties:
29.                   attributes:
30.                     allOf:
31.                       - type: object
32.                         properties:
33.                           tags:
34.                             type: array
35.                             items:
36.                               type: string
37.                       - $ref: '#/components/schemas/anotherSchema'
38.               examples:
39.                 empty_schema:
40.                   value: {}
41.                 attr_w_tags:
42.                   value:
43.                     attributes:
44.                       tags:
45.                         - thing1
46.                         - thing2
47.                         - thing3
48.                 attr_w_other_fields:
49.                   value:
50.                     attributes: {}
51.                     things: {}
52.                     otherThings: []
53.                     booleanThing: true
54.                     anotherThing: false
55. components: 
56.   schemas:
57.     anotherSchema:
58.       type: object

rule-error.yaml:
  40:26  warning  no-invalid-media-type-examples  Example value must conform to the schema: must have required property 'attributes'.
  51:21  warning  no-invalid-media-type-examples  Example value must conform to the schema: must NOT have unevaluated properties `things`.
  52:21  warning  no-invalid-media-type-examples  Example value must conform to the schema: must NOT have unevaluated properties `otherThings`.
  53:21  warning  no-invalid-media-type-examples  Example value must conform to the schema: must NOT have unevaluated properties `booleanThing`.
  54:21  warning  no-invalid-media-type-examples  Example value must conform to the schema: must NOT have unevaluated properties `anotherThing`.

jeremyfiel avatar Jan 11 '24 01:01 jeremyfiel

Hey, thanks for the quick response. I should've been clearer about what example I'm trying to get to fail. Your attr_w_other_fields isn't quite right. This reproduces what I'm seeing:

openapi: '3.1.0'
info:
  title: Title
  version: '1.0.0'
servers:
  - url: 'https://api.redocly.com'
security: []
paths:
  /api/v1/things/{thing-id}:
    get:
      summary: a summary
      description: delete with requestBody
      parameters:
        - name: thing-id
          in: path
          required: true
          schema:
            type: string
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                type: object
                properties:
                  attributes:
                    allOf:
                      - type: object
                        properties:
                          tags:
                            type: array
                            items:
                              type: string
                      - $ref: '#/components/schemas/anotherSchema'
              examples:
                attr_w_tags:
                  value:
                    attributes:
                      tags:
                        - thing1
                        - thing2
                        - thing3
                attr_w_other_fields:
                  value:
                    attributes:
                      tags: []
                      things: {}
                      otherThings: []
                      booleanThing: true
                      anotherThing: false
components:
  schemas:
    anotherSchema:
      type: object

I expect to see

Example value must conform to the schema: `attributes` property must NOT have unevaluated properties `things`.
Example value must conform to the schema: `attributes` property must NOT have unevaluated properties `otherThings`.
Example value must conform to the schema: `attributes` property must NOT have unevaluated properties `booleanThing`.
Example value must conform to the schema: `attributes` property must NOT have unevaluated properties `anotherThing`.

but I don't. Including properties: {} on the same level as the allOf, does cause these errors to manifest.

And FWIW I was on CLI version 1.3 but I updated to 1.6 and still see this issue.

samanthawritescode avatar Jan 11 '24 18:01 samanthawritescode

So a couple things here..

The allOf by itself, without any properties or additionalProperties keyword used is basically an open schema at the root. So you shouldn't have an error if you provide additional properties in the attributes schema.

It would be the equivalent of using addtiionalProperties: true or properties: {}

schema:
                type: object
                properties:
                  attributes:
                    additionalProperties: true
                    allOf:
                      - type: object
                        properties:
                          tags:
                            type: array
                            items:
                              type: string
                      - $ref: '#/components/schemas/anotherSchema'
schema:
                type: object
                properties:
                  attributes:
                    properties: {}
                    allOf:
                      - type: object
                        properties:
                          tags:
                            type: array
                            items:
                              type: string
                      - $ref: '#/components/schemas/anotherSchema'

BUT there is one caveat mentioned by Roman which is they force unevaluatedProperties/additionalProperties: false on their example evaluation because they feel a lot of users don't understand some of these JSON Schema intricacies. That means they have introduced some default behavior that is unexpected by advanced JSON Schema users. Again, this is an opinionated linting tool to help the majority so it's understandable why they've made that decision.

By adding properties: {}, they are triggering some Ajv validation code to run against that schema, and then applying their own forced additional properties logic against the schema, which is outputting the errors. Again, using properties: {} by itself, should not produce any errors from JSON Schema perspective.

The work around of providing properties: {} still remains the fix IF you want to see errors in that location. This is non-conformant behavior for JSON Schema.

If you want to disable the behavior of properties: {} incorrectly outputting an error, modify your config file:

rules:
  no-invalid-media-type-examples:
    disallowAdditionalProperties: false

https://github.com/Redocly/redocly-cli/issues/552#issuecomment-1072948715

related PR: https://github.com/Redocly/ajv/pull/12

jeremyfiel avatar Jan 11 '24 19:01 jeremyfiel

Thank you for your thorough response! I am indeed new to JSON Schema and OpenAPI, so I guess I appreciate this unexpected behavior 😅 This makes sense now. I will continue to keep the properties: {} logic because it's the best way to ensure my schema and examples are kept accurate.

samanthawritescode avatar Mar 13 '24 21:03 samanthawritescode