fix: improve schema validation

[P0lip]

Fixes #403

Checklist

• Tests added / updated
• Docs added / updated

Does this PR introduce a breaking change?

• Yes
• No

Besides addressing 403, it also improves the general readability and consistency of error messages generated by schema function.

[marbemac]

Prove it - let's see that before and after screenshot for whatever cases were improved :P.

[P0lip]

Given

openapi: 3.0.1
info:
  title: Example $ref error
  version: 1.0.0
paths:
  /user:
    get:
      operationId: getUser
      responses:
        "200":
          description: An Example
          content:
            application/json:
              schema:
                type: object
                properties:
                  user_id: 12345

5.3.0

PR

Given

openapi: 3.0.1
info:
  title: Example $ref error
  version: 1.0.0
paths:
  /user:
    get:
      operationId: getUser
      responses:
        "200":
          description: An Example
          content:
            application/json:
              schema:
                type: object
                properties:

5.3.0

PR

Given

openapi: 3.0.1
info:
  title: Example $ref error
  version: 1.0.0
paths:
  /user:
    get:
      operationId: getUser
      responses:
        "200":
          description: An Example
          parameters:

5.3.0

PR

Given

openapi: 3.0.1
info:
  title: Example $ref error
  version: 1.0.0
paths:
  /user:
    get:
      operationId: getUser
      parameters:
        - type: string
          name: module_id
          in: path
          required: true
          schema:
            type: string
          description: Module ID
      responses:

5.3.0

PR

Etc, etc.

[nulltoken]

@P0lip As this is about to become a somewhat cornerstone of schema validation, how about covering it with targeted unit tests to better express what we expect from this function?

[philsturgeon]

Agreed. Also more comments on why would help future maintainers, our future selves (and my current self) understand whats going on better.

[philsturgeon]

@P0lip this is amazing! It's the biggest issue we have with Spectral and it looks like you've nailed it.

I know it might feel annoying and redundant, but can you make a test harness for each of those examples you put in? As it's such a complicated and important thing, I want to smother it with coverage so it doesn't rear its ugly head again.

[P0lip]

@nulltoken @philsturgeon do you have any additional comments or suggestions?

[nulltoken]

@nulltoken @philsturgeon do you have any additional comments or suggestions?

To be honest, I've read the code of prepareResults at least three times, I still have no idea what it exactly does. Would it be very time consuming to export it and cover it with tests putting under the lights the input AJV-like error objects and the expected results?

By taking a look at the results of the integration tests, I can see that it indeed works, however, I'd really like to get a better understanding at how it works and solves the initial issue.

If we ever need to extend/patch this function, I'd feel more at ease with a set of tight unit tests that precisely express the behavioral contract of the function.

[P0lip]

To be honest, I've read the code of prepareResults at least three times, I still have no idea what it exactly does. Would it be very time consuming to export it and cover it with tests putting under the lights the input AJV-like error objects and the expected results?

Yep, I'll prepare errors. It'll certainly make it easier for everyone to reason about that function when you show what kind of errors it filters out and how it does it.

[P0lip]

@nulltoken added some. LMK if it's more clear now. In general all it does is removal of "evil" errors.

[P0lip]

@nulltoken done!

[huksley]

Tried to use latest develop branch with this fix applied, have a different error from my issue I have
maasglobal/maas-tsp-api#70 (comment)

/Users/user/src/maas-tsp-api/schemas/core/components/travel-mode.json
 1:1  error  oas3-schema  Property `$id` is not expected to be here.

I see no problem with this JSON schema file, see https://github.com/maasglobal/maas-schemas/blob/develop/maas-schemas/schemas/core/components/travel-mode.json

[P0lip]

Is $id considered a valid property in OAS-flavor of JSON Schema?
I'm not sure and cannot seem to find anything about that in docs.
I've checked the JSON Schema model we use for validation and it has no trace of $id, therefore the report is likely to be correct.
The question is whether $id is actually allowed or not - if it is, we'll need to adjust our JSON Schema model.

@philsturgeon any clue?