pyopenapi / pyswagger

An OpenAPI (fka Swagger) client & converter in python, which is type-safe, dynamic, spec-compliant.
MIT License
385 stars 89 forks source link

External $ref in paths doesn't follow a relative URI #53

Closed pwfff closed 8 years ago

pwfff commented 8 years ago

Example (another_file.yaml is hosted alongside the parent yaml file, and works with swagger-ui):

paths:
  $ref: 'another_file.yaml'
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/core.py", line 359, in create
    app.prepare(strict=strict)
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/core.py", line 324, in prepare
    self._prepare_obj(strict=strict)
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/core.py", line 267, in _prepare_obj
    s.scan(root=self.__root, route=[Resolve()])
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/scan.py", line 125, in scan
    handle_cls(cls)
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/scan.py", line 118, in handle_cls
    ret = ff(the_self, path, obj, self.app)
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/scanner/v2_0/resolve.py", line 78, in _path_item
    _merge(obj, app, '#/paths', PathItemContext)
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/scanner/v2_0/resolve.py", line 44, in _merge
    _resolve(cur, app, prefix, ctx)
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/scanner/v2_0/resolve.py", line 26, in _resolve
    ro = app.resolve(normalize_jr(r, prefix), parser)
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/core.py", line 400, in resolve
    obj = self.root.resolve(utils.jp_split(jp)[1:]) # heading element is #, mapping to self.root
  File "---/.virtualenvs/uad/lib/python2.7/site-packages/pyswagger/spec/base.py", line 276, in resolve
    obj = obj[t]
KeyError: 'another_file.yaml'
mission-liao commented 8 years ago

I just verified the spec didn't have this part of description. (See Paths Object)

The only field could exist under "paths" is a map from relative urls to Path Item Object. And the $ref can be placed under Path-Item Object, but not Paths Object.

And it seems that relative issues raised againt the SPEC team, but nothing is confirmed to change yet.

Therefore, even swagger-ui supports this, I might choose not to support this behavior. The spec-compliant way to do this is to place the '$ref' under Path-Item Object. Thanks for reporting issues, very appreciated.

note: a bug has been found that relative reference in this case doesn't work well. I'll fix this part later.

pwfff commented 8 years ago

Oops, I made my example wrong. I did indeed have it under the Path-Item Object. Thank you for the fix!

pwfff commented 8 years ago

I'm having problems with $ref in other places, but I might be doing it all wrong. I'm currently trying to work out a good test case for what I'm doing. Once I have something easily reproducible I'll post back here.

mission-liao commented 8 years ago

No problem, raise any question if needed. :)

pwfff commented 8 years ago

This should be a good example:

==> internal.yaml <==
swagger: '2.0'
info:
  title: Acme Internal API
  description: Docs for working with secret stuff
  version: 0.0.1
host: api.acme.com
basePath: /v1
schemes:
  - https
produces:
  - application/json
paths:
  '/login':
    $ref: 'login.yaml'
  '/secret_stuff':
    $ref: 'secret_stuff.yaml'

==> login.yaml <==
post:
  operationId: post_login
  summary: Log a user in
  description: Insert username and password, receive token
  parameters:
    - name: login_form
      in: body
      required: true
      schema:
        $ref: 'definitions/Login.yaml'
  responses:
    '200':
      description: Login result containing a session token
      schema:
        $ref: 'definitions/LoginResult.yaml'
    '403':
      description: Returned when credentials are incorrect

==> public.yaml <==
swagger: '2.0'
info:
  title: Acme Public API
  description: Docs for working with normal stuff
  version: 0.0.1
host: api.acme.com
basePath: /v1
schemes:
  - https
produces:
  - application/json
paths:
  '/login':
    $ref: 'login.yaml'
  '/stuff':
    $ref: 'stuff.yaml'

==> secret_stuff.yaml <==
post:
  operationId: post_secret_stuff
  security:
    - session_token: []
  description: does some secret stuff
  parameters:
    - name: secret_stuff_form
      in: body
      required: true
      schema:
        $ref: 'definitions/Stuff.yaml'
  responses:
    '200':
      description: Results of doing the stuff
      schema:
        $ref: 'definitions/StuffResults.yaml'
    '403':
      description: Returned when token is incorrect

==> stuff.yaml <==
post:
  operationId: post_stuff
  security:
    - session_token: []
  description: does some stuff
  parameters:
    - name: stuff_form
      in: body
      required: true
      schema:
        $ref: 'definitions/Stuff.yaml'
  responses:
    '200':
      description: Results of doing the stuff
      schema:
        $ref: 'definitions/StuffResults.yaml'
    '403':
      description: Returned when token is incorrect

==> definitions/Login.yaml <==
description: test
type: object
properties:
  id:
    type: integer

==> definitions/LoginResults.yaml <==
description: test
type: object
properties:
  id:
    type: integer

==> definitions/Stuff.yaml <==
description: test
type: object
properties:
  id:
    type: integer

==> definitions/StuffResults.yaml <==
description: test
type: object
properties:
  id:
    type: integer

And the error I get:

/Users/---/pyswagger/pyswagger/core.pyc in create(kls, url, strict)
    357         """
    358         app = kls.load(url)
--> 359         app.prepare(strict=strict)
    360
    361         return app

/Users/---/pyswagger/pyswagger/core.pyc in prepare(self, strict)
    322         """
    323
--> 324         self._prepare_obj(strict=strict)
    325
    326         # reducer for Operation

/Users/---/pyswagger/pyswagger/core.pyc in _prepare_obj(self, strict)
    265                 self.__schemes = [six.moves.urlparse(self.__url).schemes]
    266
--> 267         s.scan(root=self.__root, route=[Resolve()])
    268         s.scan(root=self.__root, route=[PatchObject()])
    269         s.scan(root=self.__root, route=[Aggregate()])

/Users/---/pyswagger/pyswagger/scan.pyc in scan(self, route, root, nexter, leaves)
    123                     if cls is BaseObj:
    124                         break
--> 125                     handle_cls(cls)
    126

/Users/---/pyswagger/pyswagger/scan.pyc in handle_cls(cls)
    116                     if f:
    117                         for ff in f:
--> 118                             ret = ff(the_self, path, obj, self.app)
    119                             if res:
    120                                 res(the_self, ret)

/Users/---/pyswagger/pyswagger/scanner/v2_0/resolve.pyc in _path_item(self, _, obj, app)
     76         # we need to merge properties of others if missing
     77         # in current object.
---> 78         _merge(obj, app, '#/paths', PathItemContext)
     79

/Users/---/pyswagger/pyswagger/scanner/v2_0/resolve.pyc in _merge(obj, app, prefix, ctx)
     42     to_resolve = []
     43     while not is_resolved(cur):
---> 44         _resolve(cur, app, prefix, ctx)
     45
     46         to_resolve.append(cur)

/Users/---/pyswagger/pyswagger/scanner/v2_0/resolve.pyc in _resolve(obj, app, prefix, parser)
     24
     25     r = getattr(obj, '$ref')
---> 26     ro = app.resolve(normalize_jr(r, prefix, app.url), parser)
     27
     28     if not ro:

/Users/---/pyswagger/pyswagger/core.pyc in resolve(self, jref, parser)
    387                 # This loaded SwaggerApp would be kept in app_cache.
    388                 app = SwaggerApp.load(url, parser=parser, app_cache=self.__app_cache, url_load_hook=self.__url_load_hook)
--> 389                 app.prepare()
    390
    391                 # nothing but only keeping a strong reference of

/Users/---/pyswagger/pyswagger/core.pyc in prepare(self, strict)
    322         """
    323
--> 324         self._prepare_obj(strict=strict)
    325
    326         # reducer for Operation

/Users/---/pyswagger/pyswagger/core.pyc in _prepare_obj(self, strict)
    265                 self.__schemes = [six.moves.urlparse(self.__url).schemes]
    266
--> 267         s.scan(root=self.__root, route=[Resolve()])
    268         s.scan(root=self.__root, route=[PatchObject()])
    269         s.scan(root=self.__root, route=[Aggregate()])

/Users/---/pyswagger/pyswagger/scan.pyc in scan(self, route, root, nexter, leaves)
    123                     if cls is BaseObj:
    124                         break
--> 125                     handle_cls(cls)
    126

/Users/---/pyswagger/pyswagger/scan.pyc in handle_cls(cls)
    116                     if f:
    117                         for ff in f:
--> 118                             ret = ff(the_self, path, obj, self.app)
    119                             if res:
    120                                 res(the_self, ret)

/Users/---/pyswagger/pyswagger/scanner/v2_0/resolve.pyc in _schema(self, _, obj, app)
     60     @Disp.register([Schema])
     61     def _schema(self, _, obj, app):
---> 62         _resolve(obj, app, '#/definitions', SchemaContext)
     63
     64     @Disp.register([Parameter])

/Users/---/pyswagger/pyswagger/scanner/v2_0/resolve.pyc in _resolve(obj, app, prefix, parser)
     24
     25     r = getattr(obj, '$ref')
---> 26     ro = app.resolve(normalize_jr(r, prefix, app.url), parser)
     27
     28     if not ro:

/Users/---/pyswagger/pyswagger/core.pyc in resolve(self, jref, parser)
    393                 self.__strong_refs.append(app)
    394
--> 395             return self.__app_cache[url].resolve(jp)
    396
    397         if not jp.startswith('#'):

/Users/---/pyswagger/pyswagger/core.pyc in resolve(self, jref, parser)
    398             raise ValueError('Invalid Path, root element should be \'#\', but [{0}]'.format(jref))
    399
--> 400         obj = self.root.resolve(utils.jp_split(jp)[1:]) # heading element is #, mapping to self.root
    401
    402         if obj == None:

/Users/---/pyswagger/pyswagger/spec/base.pyc in resolve(self, ts)
    270
    271             if issubclass(obj.__class__, BaseObj):
--> 272                 obj = getattr(obj, t)
    273             elif isinstance(obj, list):
    274                 obj = obj[int(t)]

AttributeError: 'PathItem' object has no attribute 'definitions'
mission-liao commented 8 years ago

You are correct, the fix I proposed only applied to PathItem Object, and according to spec, all Reference Object should support relative files reference.

I'll fix it and sorry for bothering you again. :(

pwfff commented 8 years ago

No problem! It's a pleasure working with you!

mission-liao commented 8 years ago

note: In older version of 2.0 spec, this schema is valid:

{
  "definitions":{
    "User": {},
    "AuthorizedUser":{
       "$ref": "User"
    }
  }
}

The AuthorizedUser would automatically reference to "#definitions/User". It's not clear for Swagger 2.0 to support this behaviour or not. If I fix relative file reference rooted with Schema Object, this behaviour is no longer supported.

pwfff commented 8 years ago

The second example here doesn't seem to work: https://github.com/OAI/OpenAPI-Specification/blob/master/guidelines/REUSE.md#guidelines-for-referencing

It looks like it's expecting whatever file it loads there to be of the Schema type, though the schema could be nested anywhere in the yaml/json that gets loaded. Also, to get that far I had to do the following:

diff --git a/pyswagger/utils.py b/pyswagger/utils.py
index 3269f57..160bcba 100644
--- a/pyswagger/utils.py
+++ b/pyswagger/utils.py
@@ -328,7 +328,8 @@ def normalize_jr(jr, url=None):
         if p.scheme == '' and url:
             p = six.moves.urllib.parse.urlparse(url)
             # it's the path of relative file
-            path = six.moves.urllib.parse.urlunparse(p[:2]+(os.path.join(os.path.dirname(p.path), jr),)+p[3:])
+            basepath = os.path.dirname(p.path)
+            path = six.moves.urllib.parse.urlunparse(p[:2]+(os.path.join(basepath, path),)+p[3:])
     else:
         path = url

I'm testing it now by just having each definition in its own file and it seems to be working.

mission-liao commented 8 years ago

At the time when Swagger 2.0 just came out, the only 'external referencible' object is Path Item. And I didn't follow the later discussion. I'm a python guy so I prefer explicit to implicit. I would definitely complained about this kind of design if I joined those discussion.

It's still possible to make this work, let me investigate it for a while. Big thanks for your efforts on trying these scenario!


It's actually much more complicated than I expect, refer to this discussion

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

My head gets hurt now.......


Asking question here, await replies.


Discussion kept going...


It seems clear now, will proceed to redesign the resolving procedure.


The implementation of pyswagger about "external $ref" is not correct according to REUSE Guide. In short, pyswagger requires the root object externally referenced be either PathItem, Schema, Parameter, Response, or Swagger object, but the spec suggests that the root object can be any valid structure, as long as the object pointed by $ref is either PathItem, Schema, Parameter, Response, or Swagger.

mission-liao commented 8 years ago

@pwfff A PR is merged to master branch and a new build would be provided next Tuesday, any further issue found later would also be included.

It's getting harder to provide a fix for pyswagger :(

mission-liao commented 8 years ago

Those fixes should be included in 0.8.17, reopen this issue if you find anything broken. :)