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

Path mangling when resolving references #335

Closed
nulltoken opened this issue Jul 7, 2019 · 6 comments
Closed

Path mangling when resolving references #335

nulltoken opened this issue Jul 7, 2019 · 6 comments
Labels
t/bug Something isn't working

Comments

@nulltoken
Copy link
Contributor

nulltoken commented Jul 7, 2019

Describe the bug
Following #171 and the wonderful work in #245 (Thanks @marbemac !), here's another reference resolving relating issue.

To Reproduce

From a Windows platform, create the following tree structure

C:\test\repo
        |_ package.json # referencing "@stoplight/spectral": "4.0.0-beta.6"
        |_ repro
             |_ another_library.yaml
             |_ nested
                  |_ a_library.yaml
                  |_ three.yaml

With the following content

repro/another_library.yaml

swagger: '2.0'

info:
  title: Repro three
  description: External definitions
  version: "1.0.0"
  contact:
    name: toto
host: not-example.com
schemes:
  - https

paths: { }

definitions:
  other_email:
    type: string
    format: email

repro/nested/a_library.yaml

swagger: '2.0'

info:
  title: Repro three
  description: External definitions
  version: "1.0.0"
  contact:
    name: toto
host: not-example.com
schemes:
  - https

paths: { }

definitions:
  email:
    type: string
    format: email

repro/nested/three.yaml

swagger: '2.0'

info:
  title: Repro three
  description: Endpoint definition
  version: "1.0.0"
  contact:
    name: toto
host: not-example.com
schemes:
  - https
basePath: /repro/nested/three

paths:
  /test:
    post:
      summary: Gets nothing
      operationId: "17"
      tags: [ one ]
      description: Cf. summary
      parameters:
        - name: body
          description: Content
          in: body
          schema:
            $ref: '#/definitions/TheDefinition'
          required: true
        - name: test
          in: query
          schema:
            $ref: '#/definitions/AnotherDefinition'
          required: true
      responses:
        204:
          description: No content

definitions:
  TheDefinition:
    type: object
    properties:
      email_one:
        description: One email address.
        example: "one@above.com"
        allOf:
        -  $ref: '../another_library.yaml#/definitions/other_email'
      email_two:
        description: Another mail address.
        example: "one@local.com"
        allOf:
        -  $ref: '../another_library.yaml#/definitions/other_email'

  AnotherDefinition:
    type: string
    example: "one@two.com"
    allOf:
    - $ref: './a_library.yaml#/definitions/email'

Then run the following commands

nulltoken@somewhere MINGW64 /c/test/repo (em/spectral)
$ yarn spectral lint -q  ./repro/another_library.yaml
yarn run v1.15.2
$ C:\test\repo\node_modules\.bin\spectral lint -q ./repro/another_library.yaml
Done in 1.45s.

nulltoken@somewhere MINGW64 /c/test/repo (em/spectral)
$ yarn spectral lint -q  ./repro/nested/a_library.yaml
yarn run v1.15.2
$ C:\test\repo\node_modules\.bin\spectral lint -q ./repro/nested/a_library.yaml
Done in 1.41s.

nulltoken@somewhere MINGW64 /c/test/repo (em/spectral)
$ yarn spectral lint -q  ./repro/nested/three.yaml
yarn run v1.15.2
$ C:\test\repo\node_modules\.bin\spectral lint -q ./repro/nested/three.yaml
Encountered error when running rule 'oas2-schema' on node at path '$':
Error: Couldn't find property ~1test of /paths/~1test/post/parameters/1

repro/nested/three.yaml
 25:18  warning  valid-example  "email_one" property can't resolve reference ../another_library.yaml#/definitions/other_email from id #
 25:18  warning  valid-example  "email_two" property can't resolve reference ../another_library.yaml#/definitions/other_email from id #
 30:18  warning  valid-example  "schema" property can't resolve reference ./a_library.yaml#/definitions/email from id #
 41:17  warning  valid-example  "email_one" property can't resolve reference ../another_library.yaml#/definitions/other_email from id #
 45:18    error  invalid-ref    ENOENT: no such file or directory, open 'C:\repro\another_library.yaml'
 46:17  warning  valid-example  "email_two" property can't resolve reference ../another_library.yaml#/definitions/other_email from id #
 52:21  warning  valid-example  "AnotherDefinition" property can't resolve reference ./a_library.yaml#/definitions/email from id #
 56:13    error  invalid-ref    ENOENT: no such file or directory, open 'C:\repro\nested\a_library.yaml'

✖ 8 problems (2 errors, 6 warnings, 0 infos)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

nulltoken@somewhere MINGW64 /c/test/repo (em/spectral)
$

Expected behavior

  1. This log message seems a bit weird:

    Error: Couldn't find property ~1test of /paths/~1test/post/parameters/1
    
  2. The error messages seems to put under the light some kind of path mangling

    ENOENT: no such file or directory, open 'C:\repro\another_library.yaml'
    

    Which, given the directory structure, should rather be 'C:\test\repo\repro\another_library.yaml'

Environment (remove any that are not applicable):

  • Library version: @stoplight/spectral 4.0.0-beta.6
  • OS: Windows 7
@nulltoken nulltoken added the t/bug Something isn't working label Jul 7, 2019
@P0lip
Copy link
Contributor

P0lip commented Jul 7, 2019

Hey @nulltoken!
Thanks for reaching out again and providing valuable feedback.
There seem to be at least two separate issues:

  • schema validation function does not handle JSON pointer escape notation
  • path mangling

I spent a little investigating the first bug and I'm afraid it's caused by better-ajv-errors, a dependency we introduced in 4.x to improve the quality of our AJV validation messages.
There is a TODO comment left in the code indicating JSON pointer escape notation is not supported just yet.
There is a PR open atlassian/better-ajv-errors#33 addressing the issue, but it seems to be kind of stale. I'll try to ping everyone involved and see whether there is something we could help with it.

Regarding mangling - haven't verified that yet, but will try to do that as soon as possible and get back to you with more information on that.

@P0lip
Copy link
Contributor

P0lip commented Jul 9, 2019

@nulltoken
Error: Couldn't find property ~1test of /paths/~1test/post/parameters/1
Fixed in #336, the fix is included in the newest beta, so you can give it a try.

Unfortunately, I haven't had time to look into path mangling issue yet - been busy with the 4.x release lately, but will try to follow up on that as soon as possible.

@marbemac
Copy link
Contributor

marbemac commented Jul 9, 2019

I believe that the path issues are something in ajv - ideally ajv does not need to do any resolving, since we already resolve via our own json-ref-resolver.

Separately, @nulltoken I'm not sure portions of three.yaml are valid?

This:

email_two:
  description: Another mail address.
  example: "one@local.com"
  allOf:
  -  $ref: '../another_library.yaml#/definitions/other_email'

Is basically the equivalent of this:

email_two:
  description: Another mail address.
  example: "one@local.com"
  $ref: '../another_library.yaml#/definitions/other_email'

Which afaik is invalid, since draft 4 $ref's cannot have sibling properties. When this is dereferenced, it will end up as (clobbering the sibling description and example properties):

email_two:
  type: string
  format: email

But I might be mistaken - lmk :).

@nulltoken
Copy link
Contributor Author

@nulltoken
Error: Couldn't find property ~1test of /paths/~1test/post/parameters/1
Fixed in #336, the fix is included in the newest beta, so you can give it a try.

@P0lip Works like a charm! Thanks a lot.

Which afaik is invalid, since draft 4 $ref's cannot have sibling properties. When this is dereferenced, it will >end up as (clobbering the sibling description and example properties):

@marbemac You're completely correct (and I'm a complete idiot ;-) ). Btw, I've been bitten in the past more than I like to remember about this. Would there be a way for spectral to actually catch those patterns and well... warn about them?

@philsturgeon
Copy link
Contributor

@nulltoken Let's absolutely make a rule for spotting $ref siblings. You are not the only person getting clobbered by this, I think we've been bitten a few times here ourselves!

@philsturgeon
Copy link
Contributor

Ref siblings is over here #473

Path mangling has been solved elsewhere. I think we're good here.

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

No branches or pull requests

4 participants