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

Fails to resolve file-based $refs with definitions #8

Closed
kegsay opened this issue May 29, 2015 · 22 comments
Closed

Fails to resolve file-based $refs with definitions #8

kegsay opened this issue May 29, 2015 · 22 comments

Comments

@kegsay
Copy link

kegsay commented May 29, 2015

If you reference a definition in another file, the parser fails to load it because it doesn't strip before the #.

# foo.yaml
definitions:
  Error:
    properties:
      error:
        type: string
    required: ["error"]
# main.yaml
[...]
  schema:
    $ref: './foo.yaml#/definitions/Error'

Fails with:

// metadata
{
  baseDir: "/home/swagger/",
  files: [
    "/home/swagger/main.yaml",
    "/home/swagger/foo.yaml#/definitions/Error"
  ],
  urls: [],
  '$refs': {}
}
// error
Error: Error opening file "/home/swagger/foo.yaml#/definitions/Error"

As an aside, I believe that when $ref references files, it is not compulsory to put a starting . (this threw me off for a while as I was trying foo.yaml#/definitions/Error instead of ./foo.yaml#/definitions/Error. It was only when debugging resolve.js did I notice:

:14 // Matches anything that starts with "http://" or contains a period (".")
@JamesMessinger
Copy link
Member

Hi, thanks for opening this issue. Swagger Parser doesn't currently support deep-linking into files using hash fragments, but I can definitely add that. Expect it soon.

Also, when referencing an external file, you don't need to start the file name with a period (.). The RegExp pattern in resolve.js matches any path that contains a period anywhere in the path (such as a file extension). So you can use foo.yaml rather than ./foo.yaml.

@kegsay
Copy link
Author

kegsay commented May 29, 2015

Thanks, that would be great if you could add it! I've been playing around with trying to get it to work and can get the $refs in the metadata looking about right by splitting here:

// resolve.js:76
resolve$Ref(value.$ref.split("#/")[0], $refPath, state, callback);

Produces refs with:

'$refs': {
  "./core.yaml": { definitions: [Object] },
  "/home/core.yaml": { definitions: [Object] }
}

and this doesn't seem to produce any errors. However, modifying core.yaml and making it invalid Swagger still seems to pass the validator, so I'm guessing it isn't really reading it.

The RegExp pattern in resolve.js matches any path that contains a period anywhere in the path (such as a file extension). So you can use foo.yaml rather than ./foo.yaml.

That's interesting, in some of my earlier tests I didn't have any file extension on the referenced file which I guess is why it didn't like it.

@kegsay
Copy link
Author

kegsay commented May 29, 2015

As a workaround for now, I can get it to work by splitting each definition into a separate file e.g.

# error.yaml
type: object
properties:
  error:
    type: string
required: ["error"]

@jkordas
Copy link

jkordas commented Jun 30, 2015

I found this in swagger documentation:
https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#relative-files-with-embedded-schema-example.
It looks like swagger should support references with '#' sign.
How long does it take to implement it? When can we excpect this feature?

@JamesMessinger
Copy link
Member

I actually started working on this last night. (sorry for taking so long, but I've been totally swamped at work for the past few weeks)

This fix is part of a larger refactoring that will fix a few related bugs as well, so it's going to take a few days to finish. I'm hoping to finish it by next week. As a side-bonus, it should also improve performance - especially on Internet Explorer.

@mrkev
Copy link

mrkev commented Jul 10, 2015

Hey @BigstickCarpet just wondering how that linking is going and if there's anything I could do to help! I was stumped by this error when running gulp-swagger (which relies on this module) and finally spotted this bug, glad to hear it's being worked on!

I understand if you're busy at work. I have to do some swagger definitions for my own job so if you'd like to set up a feature branch and give me some guidance I could tackle this issue on Monday.

@JamesMessinger
Copy link
Member

@mrkev @jkordas @kegsay - I'm making good progress on this, but it's a major refactoring, since Swagger Parser doesn't properly adhere to the JSON Reference and JSON Pointer specs. To be fair, this is because the early drafts of the Swagger 2.0 spec also didn't comply fully with JSON Reference and JSON Schema, but the current version of the Swagger 2.0 spec is compliant with both, so now I'm making Swagger Parser compliant as well.

This refactoring consists of two parts:

Part 1

Split Swagger Parser into two different projects, one for JSON Schema logic and one for Swagger logic. The new project — which deals with all the JSON Schema logic — is called JSON Schema $Ref Parser. I just released v1.0.0-alpha today. It's still a bit rough around the edges, but it's stable enough for me to begin part 2 of the refactoring.

Part 2

Refactor Swagger Parser to use JSON Schema $Ref Parser for all of its JSON Schema logic. This will make Swagger Parser's code much cleaner, easier to understand, and easier to maintain. It will also make it fully-compliant with the JSON Reference and JSON Pointer specs. I just started this task today, and I should have Swagger Parser v3.0.0-alpha ready in a few days.

@glen-84
Copy link

glen-84 commented Jul 16, 2015

👍

@JamesMessinger
Copy link
Member

I've created a v3.0.0 branch for this refactoring. It's still in progress, but here's some documentation to get you started. You can install the new version by explicitly specifying the alpha tag during install:

npm install swagger-parser@alpha

The API is totally different from v2.5. Here are the key changes:

  1. Separate methods
    The old API had a single parse() method that actually did waaaaay more than just parse stuff. Depending on the options you passed it, it would also resolve $ref pointers, dereference them, and perform JSON Schema validation. In the new API, there are now separate methods for parsing, resolving, dereferencing, and validating.
  2. Support for ES6 Promises
    The old API used callbacks. The new API still supports callbacks, but also supports ES6 Promises
  3. New JSON Schema validator
    I've switched from tv4 to Z-Schema, which is faster and performs more accurate validation. It also has lots more options, some of which I plan to expose as Swagger Parser options.
  4. Removed support for shorthand $refs
    Previous versions of the Swagger 2.0 spec allowed for a shorthand $ref syntax. For example $ref: customer was a shorthand for $ref: "#/definitions/customer". This shorthand syntax is not valid JSON Reference syntax and is no longer part of the Swagger 2.0 spec, so I've removed support for it from Swagger Server.

@glen-84
Copy link

glen-84 commented Jul 16, 2015

@BigstickCarpet I need the dereferencing, so I guess I'll need to wait a bit longer. 😃

@JamesMessinger
Copy link
Member

@glen-84 - SwaggerParser.dereference() is already working. I just haven't had a chance to document it yet. Same goes for SwaggerParser.validate(). The API for those methods is exactly the same as for SwaggerParser.parse(), so refer to the documentation for that method.

DISCLAIMER: When I say it's "already working", I mean that I manually tested it on a few sample files, and the output looked as expected. I'm still in the process of refactoring all of my unit tests from the old API to the new API, so it's possible that I'll discover bugs in the code once the unit tests are working.

@glen-84
Copy link

glen-84 commented Jul 16, 2015

[SyntaxError: Unable to resolve $ref pointer "\Programming\Projects\x\Web%20API%20docs\swagger.yaml"]

http://pastie.org/private/0ozh5wm8li9qrrifl9cbua (swagger.yaml)
http://pastie.org/private/6coezsiq2d9bmawl4yygg (definitions/models/videos/video.yaml)

@JamesMessinger
Copy link
Member

@glen-84 - What operating system are you on? Based on the error message, it looks like Windows. I haven't tested the new code on Windows yet, but I can do that tonight.

@glen-84
Copy link

glen-84 commented Jul 16, 2015

Windows 7.

@glen-84
Copy link

glen-84 commented Jul 17, 2015

It seems to work on Linux, so it must be a Windows issue. Did you find anything last night?

@JamesMessinger
Copy link
Member

@glen-84 - Yeah, I found the issue. The code is mistakenly parsing Windows paths as URLs, so "c:\foo\bar" gets interpreted as "c://foo/bar". I'll fix it tonight and release a new version

@glen-84
Copy link

glen-84 commented Jul 17, 2015

Awesome, thank you! 😃

@JamesMessinger
Copy link
Member

Alright, it works on Windows now. Update to v3.0.0-alpha.3

@glen-84
Copy link

glen-84 commented Jul 18, 2015

❤️

BTW, I'm using relative references now, but in my old version, I was using remote refs, which don't seem to work:

[SyntaxError: Error at D:\Programming\Projects\x\Web API docs\swagger.yaml#/paths//videos/{id}/get/responses/200/schema/$ref
Error: { [Error: ENOENT, open 'D:\Programming\Projects\x\Web API docs\http:\localhost:3000\definitions\models\videos\video.yaml']
  errno: -4058,
  code: 'ENOENT',
  path: 'D:\\Programming\\Projects\\x\\Web API docs\\http:\\localhost:3000\\definitions\\models\\videos\\video.yaml' } 'Error opening file "%s"' 'D:\\Programming\\Projects\\x\\Web API docs\\http:\\localhost:3000\\definitions\\models\\videos\\video.yaml']

I just thought I'd mention it.

@JamesMessinger
Copy link
Member

@glen-84 - Thanks for catching that. Dumb oversight on my part. It's fixed now (upgrade to v3.0.0-alpha.4)

@glen-84
Copy link

glen-84 commented Jul 18, 2015

👍

@JamesMessinger
Copy link
Member

FYI - Swagger 3.0.0 is now officially released, and it includes this fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants