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

add support for relative urls in resolver #417

Closed
fehguy opened this issue May 8, 2015 · 26 comments
Closed

add support for relative urls in resolver #417

fehguy opened this issue May 8, 2015 · 26 comments

Comments

@fehguy
Copy link
Contributor

fehguy commented May 8, 2015

No description provided.

@javiervidal
Copy link

Any reason for closing it?

@webron
Copy link
Contributor

webron commented May 11, 2015

It's open?

@javiervidal
Copy link

Sorry, it's the issue in swagger-ui the one that has been closed. I need more coffee.

@webron
Copy link
Contributor

webron commented May 11, 2015

@fehguy
Copy link
Contributor Author

fehguy commented May 21, 2015

Some thoughts on this request. First, relative refs will be, by definition, relative to the schema document. That means, for the swagger document at http://www.foo.com/swagger.json, the relative ref, $ref: MyModel will resolve to http://www.foo.com/swagger.json#MyModel.

Next, if you follow the swagger schema definition, you cannot arbitrarily put models in different locations. It is specific about where you put it. You cannot put it at /foo/bar or it will fail the validation.

So... how will relative references work? We could say #/definitions/MyModel but that's not very useful.

@jharmn
Copy link

jharmn commented May 21, 2015

To clarify: I think the request is more about $ref using files relative to swagger.yaml, without the use of http.
Context: to state the obvious - in scaling larger API definitions (especially in microservice environments), there's a need to separate definitions into multiple files/folders.

  • Using http:// | https:// (per https://github.com/swagger-api/swagger-spec/blob/master/guidelines/REUSE.md) to resolve all related files is problematic, as different environments change hostnames.
  • During development, hosting the swagger.yaml on different servers instantly creates cross domain issues if the hosting server doesn't match the $refs in the swagger.yaml.
  • Additionally, it's difficult to deal with specs hosted in Github with forks (e.g. /repo changes to /jason). By using files references relative to swagger.yaml, hostnames+parent folders per environment aren't an issue.

IMO ^/ is perfect and should remain, should avoid a number of path traversal risks.
With that in mind, paths with or without or dots should resolve

Path Examples

  • domain/definitions.json#/foo
  • ./domain/definitions.json#/foo
  • ../common/definitions.json#/bar/bar

Swagger Examples:

Definitions from a peer file
        200:
          description: Successful responses
          schema:
            $ref: 'definitions.yaml#/MyResource'
Definitions from a sub-folder/file
        200:
          description: Successful responses
          schema:
            $ref: 'specific-domain/definitions.yaml#/MyResource'
Common definition in a parent folder/file
        400:
          description: Validation error
          schema:
            $ref: '../common/definitions.yaml#/ApiError'
Parameter from shared definition

Really just to highlight that this would work for other $ref scenarios outside of responses ;)

      parameters:
        - $ref: '../common/parameters.json#/apiVersionParam'

Essentially, the current resolution looks for http or #/; when neither is present, local paths should be leveraged. The scheme/host/port of the original request to swagger.yaml would be used to resolve these relative file references.

P.S. If the resolution of ., .. is problematic, it could be a Swagger reuse recommendation to use soft links or gitmodules for references to common schema. In other words, instead of ../common/definitions.json, a common sub-folder would be linked to another location (which is pretty realistic, as you'd likely want shared schema in a different repo). As such, you'd only need to resolve common/definitions.json.

@fehguy
Copy link
Contributor Author

fehguy commented May 21, 2015

Thank you. These all make sense and are doable but I believe are not legal in JSON schema. Let me dig into it more or perhaps @webron knows more about what is allowed.

@fehguy
Copy link
Contributor Author

fehguy commented May 21, 2015

OK looking at this more, here are some thoughts.

  1. The spec may be hosted at http://foo.com/bar/path/to/spec/swagger.json. If we are resolving references to this file, we would have to use #/definitions/... or #/parameters/..., etc.

  2. Supporting paths relative to the root of the spec is easy. We can simply look for ^/ in the reference and find the host serving the spec, and make it relative to that.

  3. Supporting paths relative to the spec location is also easy. We'll take the location of the spec and walk up the path as needed.

  4. Support relative paths in remote files is tricky. What does /foo/bar mean inside a remote reference?

For example:

Pet:
  properties:
    id:
      type: integer
      format: int64
    category:
      $ref: "http://foo.com/bar/models/Category"

and at http://foo.com/bar/models/Category:

Category:
  properties:
    id:
      type: integer
      format: int64
    person:
      $ref: "/models/Person"

Where does /models/Person resolve to?

The first 3 scenarios are easily supported. The fourth makes my head hurt.

@jharmn
Copy link

jharmn commented May 21, 2015

It's definitely legit in JSON Schema. I sometimes wish it wasn't...the horrors I've seen 😱

From http://spacetelescope.github.io/understanding-json-schema/structuring.html:

Note
$ref can also be a relative or absolute URI, so if you prefer to include your definitions in separate files, you can also do that. For example:

{ "$ref": "definitions.json#/address" }

@fehguy
Copy link
Contributor Author

fehguy commented May 21, 2015

Thanks--what do you think of item 4?

@jharmn
Copy link

jharmn commented May 21, 2015

Redoing your example based on my experience of how this works (150+ repos full of linked JSON Schema).

Pet:
  properties:
    id:
      type: integer
      format: int64
    category:
      $ref: "http://foo.com/bar/models/Category.json"

and at http://foo.com/bar/models/Category.json:

Category:
  properties:
    id:
      type: integer
      format: int64
    person:
      $ref: "Person.json" #Note that the folder is removed; relative, not absolute file ref

In the revised example, $ref: "Person.json" would resolve in the same folder that Category.json resides in. As stated before, IMO it would be good to be opinionated about ^/.

However, the dot resolution adds a slightly tricky twist to think about too.

Person:
  properties:
    id:
      type: integer
      format: int64
    address:
      $ref: "../common/Address.json" # This assumes that there's a peer folder to `models`, `common`

Plus there's file + anchor resolution to think about:

Person:
  properties:
    id:
      type: integer
      format: int64
    address:
      $ref: "../common/definitions.json#/Address" # Assumes there are a number of definitions in one file

@webron
Copy link
Contributor

webron commented May 21, 2015

  • domain/definitions.json#/foo
  • ./domain/definitions.json#/foo
  • ../common/definitions.json#/bar/bar

As far as I can tell, the first two are equivalent. /domain/definitions.json#/foo would be different. Isn't that the case?

@jharmn
Copy link

jharmn commented May 21, 2015

@webron definitely equivalent, some folks do the dot slash (linux-ey habits I guess), which should work in premise.
/domain/definitions.json#/foo is a questionable approach, as mentioned before: with github forking etc, you can't always guarantee what the root directory will be.

@olensmar
Copy link
Contributor

isn't the relative reference always relative to the absolute path of the containing file/resource? So in your 4th example above @fehguy it would resolve relative to the path http://foo.com/bar/models/Category - which I think is in line with what @jasonh-n-austin added!?

@jharmn
Copy link

jharmn commented May 21, 2015

@olensmar yep that's the idea. Each file resolves it's own refs, regardless where they're included from. In the swagger.yaml->Category->Person example, the $ref in Category is resolved from it's own location to find Person (as opposed to the location of swagger.yaml).

@webron
Copy link
Contributor

webron commented May 21, 2015

@jasonh-n-austin - regarding /domain/definitions.json#/foo I don't see an issue. This is more of a user error problem. If the user hosts it in an environment where it doesn't make sense, sure, it won't work. Not really a problem.

@jharmn
Copy link

jharmn commented May 21, 2015

@webron yeah I don't think it's something not worth supporting, but if there's any issue with scope, I'd make it higher on the list to kill, that's all.

fehguy added a commit that referenced this issue May 22, 2015
@yonderblue
Copy link

big 👍 to get this working

@fehguy
Copy link
Contributor Author

fehguy commented May 26, 2015

Been working on it, but there are many details to finalize the cases that @jasonh-n-austin brought up (which I think are valid cases).

@jharmn
Copy link

jharmn commented May 26, 2015

🍻 => @fehguy, only after ☕ ;)

fehguy added a commit that referenced this issue May 29, 2015
@fehguy
Copy link
Contributor Author

fehguy commented May 29, 2015

@jasonh-n-austin this has been a grind, but I've refactored resolver.js to support these scenarios. The tests are all failing, but please look at the expected results:

swagger-js/test/resolver.js

Lines 561 to 635 in 6fc1063

it('resolves relative references from a peer file', function(done) {
var api = new Resolver();
var spec = {
host: 'http://petstore.swagger.io',
basePath: '/v2',
paths: {
'/health': {
$ref: 'definitions.yaml#/MyResource'
}
}
};
// should look in http://localhost:8000/foo/bar/swagger.json#/paths/health
api.resolve(spec, 'http://localhost:8000/foo/bar/swagger.json', function (spec, unresolvedRefs) {
console.log(unresolvedRefs)
expect(unresolvedRefs.Category).toEqual({
root: 'http://localhost:8000/foo/bar/definitions.yaml',
location: '/paths/health/MyResource'
});
var health = spec.paths['/MyResource'];
test.object(health);
done();
});
});
it('resolves relative references from a sub-folder/file', function(done) {
var api = new Resolver();
var spec = {
host: 'http://petstore.swagger.io',
basePath: '/v2',
paths: {
'/health': {
$ref: 'specific-domain/definitions.yaml#/MyResource'
}
}
};
// should look in http://localhost:8000/foo/bar/swagger.json#/paths/health
api.resolve(spec, 'http://localhost:8000/foo/bar/swagger.json', function (spec, unresolvedRefs) {
console.log(unresolvedRefs)
expect(unresolvedRefs.Category).toEqual({
root: 'http://localhost:8000/specific-domain/definitions.yaml',
location: '/paths/health/MyResource'
});
var health = spec.paths['/MyResource'];
test.object(health);
done();
});
});
it('resolves relative references from a parent folder/file', function(done) {
var api = new Resolver();
var spec = {
host: 'http://petstore.swagger.io',
basePath: '/v2',
paths: {
'/health': {
$ref: '../common/definitions.yaml#/ApiError'
}
}
};
// should look in http://localhost:8000/foo/bar/swagger.json#/paths/health
api.resolve(spec, 'http://localhost:8000/foo/bar/swagger.json', function (spec, unresolvedRefs) {
console.log(unresolvedRefs)
expect(unresolvedRefs.Category).toEqual({
root: 'http://localhost:8000/foo/common/definitions.yaml',
location: '/paths/health/ApiError'
});
var health = spec.paths['/ApiError'];
test.object(health);
done();
});
});
});

Note that in each test case, we're calling the resolver with the location of the spec being read (that's required for traversing the path and will happen automatically). For the test cases, we're assuming the location of the spec is this:

http://localhost:8000/foo/bar/swagger.json

The expected results are captured by enumerating the unresolvedRefs, which are, essentially, the information used to locate the reference. For example:

{
  "root": "http://localhost:8000/specific-domain/definitions.yaml",
  "location": "/paths/health/MyResource"
}

indicates that we'd be using the object at this location:

http://localhost:8000/specific-domain/definitions.yaml#/paths/health/MyResource

Can you please confirm that this is what is expected from the relative ref examples? This whole thing gave me a huge headache, btw.

@jharmn
Copy link

jharmn commented May 29, 2015

I have no doubt this was a tough one. I took a gander at the code, and decided I should just help quantify it in the issue instead of mangling your code (I'm not strong in Node)...glad you're performing the surgery 😷

Looked through the tests, this hits all the right scenarios...peer, parent, sub. Reviewing the cases I outlined before, these tests should cover it all.

A thought: I've been fiddling with various Java, Ruby & Python-based swagger libraries in Github this week...it's going to be a long haul to get everything to support this.
Examples in the petstore (https://github.com/swagger-api/swagger-spec/tree/master/examples/v2.0) and refinements to the spec (https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#reference-object) would certainly go a long way toward socializing how it works.
I'd be happy to help chop up the pet store to create samples and write up some examples in the spec. The petstore samples might help out your testing effort in the dependent apps (swagger-ui, swagger-editor etc) as well.

@jharmn
Copy link

jharmn commented Jun 1, 2015

FYI @fehguy created a PR with some samples to start the discussion:
OAI/OpenAPI-Specification#384

@jharmn
Copy link

jharmn commented Jun 2, 2015

FYI @fehguy swagger-parser has external file support built in, could be helpful
https://github.com/BigstickCarpet/swagger-parser

@fehguy
Copy link
Contributor Author

fehguy commented Jun 3, 2015

OK I believe this is mostly working. Adding tests now, and hope to merge to develop_2.0 & close this out tonight.

@fehguy fehguy closed this as completed Jun 3, 2015
@fehguy fehguy reopened this Jun 3, 2015
@jharmn
Copy link

jharmn commented Jun 5, 2015

@fehguy FYI examples are merged in swagger-spec/master now:
https://github.com/swagger-api/swagger-spec/tree/master/examples/v2.0/json/petstore-separate

Doc updates for Reference Object next:
OAI/OpenAPI-Specification#387

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

6 participants