Add OpenAPI/Swagger spec (fixes #631) #977

gabisurita · 2016-12-08T18:36:11Z

Fixes #631

Features:

OpenAPI documentation resource at /v1/swagger.json
Works with Swagger codegen and Bravado automated client generators.
It's compatible with Swagger UI and Apiary interactive documentation generators. You can try them on Swagger Hub or Apiary.io (although working with basicAuth there is tricky).
Tests for semantic and logic using bravado_core.

Limitations:

No support for schema free fields.
- Treated like extensions for code generators (may raise warnings on other utilities)
- No filtering
- No field selection
- No validation on collection schema
No validation on OR required clauses (ex: provide data or permissions)
No validation for fields that are required on response, but not on request
(ex: id doesn't show as a required field), doing so would result in two different
object definitions for each object, which would add unnecessary complexity to the clients IMO)
Backoff headers

Pending:

r? @glasserc @Natim @leplatrem

delijati · 2016-12-09T15:49:49Z

There are two ways of creating a valid json api with openapi (swagger) support:

1.) swagger.yaml (use yaml, we need to read it) as master + some validation [1]
2.) cornice + colander as master and create a swagger.yaml file [2]

Every way has its advantages and disadvantages.

[1] https://github.com/pipermerriam/flex or https://github.com/striglia/pyramid_swagger
[2] https://github.com/Cornices/cornice.ext.swagger

gabisurita · 2016-12-09T19:46:22Z

@delijati, thank you for the feedback!

When you say validation you're talking about validating every request/response against the spec or validating the spec itself? Unfortunately, from what I've tested, pyramid swagger request/response validation isn't suitable for all use cases (ex: extra parameters with any type on an object body). I've just performed validation during tests using bravado_core (which is pyramid swagger validation core). I'm also not sure about adding another validation layer. If you say about validating the spec itself, why not use bravado_core instead of flex or pyramid_swagger?
I'm not sure using cornice.ext.swagger is feasible, but I think it might add considerable complexity to the API code, which was something I want to avoid.

About YAML instead of JSON, I agree. It is listed as pending.

delijati · 2016-12-09T20:12:32Z

@gabisurita

Yes every request/ response against the spec. I looked bravado_core up, they seem also to
provide validation of requests and responses. (Sorry i assumed you have no validation at all)
cornice.ext.swagger is the other way (generating swagger.yaml) from colander schema. That
makes only sense if you already use or have plans to use cornice. I'm also not sure if the
generated swagger.yaml is feasible for yours needs.

For the YAML part, swagger2.0 understands json, yaml:

class YamlRendererFactory(object):
    def __init__(self, info):
        pass

    def __call__(self, value, system):
        response = system['request'].response
        response.headers['Content-Type'] = 'application/x-yaml; charset=UTF-8'
        return yaml.dump(value).encode('utf-8')

def includeme(config):
    config.add_renderer(
        'yaml', 'pyramid_swagger.api.YamlRendererFactory'
    )

gabisurita · 2016-12-09T20:58:44Z

@delijati i've used validation of requests/responses just on tests. I think using it in production doesn't make much sense (as we already validate them using colander). The idea is only to ensure that the spec is correct, not use it for validation.

delijati · 2016-12-10T00:21:39Z

As you mentioned that kinto is already using cornice i integrated cornice.ext.swagger (with a minor patch) and the resulting swagger.json looks promising.
swagger_json.txt

delijati · 2016-12-10T12:10:18Z

@gabisurita I hacked a small plugin together, check it out if you like.

$ pip install git+https://github.com/delijati/kinto_swagger.git
$ vim config/kinto.ini
kinto.includes=kinto_swagger

$ firefox http://0.0.0.0:8888/v1/swagger

gabisurita · 2016-12-10T13:10:38Z

@delijati I've tried the plugin, but got a 500 on GET /v1/swagger_json. Do you have a branch where you've been testing this? It would be easier to reproduce on my side.

Here's the traceback:

2016-12-10 11:00:06,578 ERROR [kinto.core.views.errors][waitress] "GET   /v1/swagger_json" ? (? ms) unbound method get() must be called with BatchRequest instance as first argument (got str instance instead) agent=HTTPie/0.9.4 authn_type=None errno=None exception=Traceback (most recent call last):
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/pyramid/tweens.py", line 22, in excview_tween
    response = handler(request)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/pyramid_tm/__init__.py", line 119, in tm_tween
    reraise(*exc_info)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/pyramid_tm/__init__.py", line 98, in tm_tween
    response = handler(request)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/pyramid/router.py", line 158, in handle_request
    view_name
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/pyramid/view.py", line 547, in _call_view
    response = view_callable(context, request)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/pyramid/viewderivers.py", line 442, in rendered_view
    result = view(context, request)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/pyramid/viewderivers.py", line 147, in _requestonly_view
    response = view(request)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/kinto_swagger/__init__.py", line 20, in swagger_json
    base_path=base_path, head=False)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/cornice_swagger/swagger.py", line 174, in generate_swagger_spec
    parameters, definition = schema_to_parameters(schema, service)
  File "/home/gabi/kinto/.venv/lib/python2.7/site-packages/cornice_swagger/swagger.py", line 22, in schema_to_parameters
    location_schema = schema.get(location)
TypeError: unbound method get() must be called with BatchRequest instance as first argument (got str instance instead) lang=None uid=None

delijati · 2016-12-10T13:35:03Z

@gabisurita i tried it on the master of kinto with only two plugins activated:

$ git clone https://github.com/Kinto/kinto.git
$ cd kinto
$ virtualenv env
$ env/bin/pip install -e .
$ env/bin/pip install git+https://github.com/delijati/kinto_swagger.git
$ kinto init
$ kinto migrate
$ vim config/kinto.ini
...
kinto.includes = kinto.plugins.default_bucket
                         kinto_swagger
...
$ kinto start
$ firefox http://0.0.0.0:8888/v1/swagger

Btw. the error seams to be a older version of cornice_swagger. I also fixed the path to the swagger js files in kinto_swagger.

gabisurita · 2016-12-10T14:14:33Z

Upgrading cornice_swagger did the work.
This looks great. Thank you for taking the time to do it. Skimming through the docs I've got a few questions while following this approach:

How should responses be documented?
Patch requests should have a body, but that doesn't show on the documentation
How to document query strings?
Does it allows customizing the tags? I think putting all the operations under the tag buckets may be confusing, specially when using client generators.
How to set up authentication?
Finally it doesn't seems to generate valid swagger, but I understand it's just a sketch.

delijati · 2016-12-10T15:48:16Z

1. How should responses be documented?

I think currently cornice does not support that @leplatrem ? Maybe this can be added as docstrings and extracted from there?

2. Patch requests should have a body, but that doesn't show on the documentation

Maybe the body schema is not defined right see 3.

3. How to document query strings?

That is a cornice feature and they are extracted. (cornice docs "Schema validation")

class Query(MappingSchema):
    yeah = SchemaNode(String())
    mau = SchemaNode(String())

class RequestSchema(MappingSchema):
    body = Body(description="Defines a cornice body schema")
    querystring = Query()

4. Does it allows customizing the tags? I think putting all the operations under the tag buckets may be confusing, specially when using client generators.

Currently it only does tag_name = service.path.split("/")[1] so sure this can be improved somehow.

5. How to set up authentication?

You mean this? https://github.com/swagger-api/swagger-ui#header-parameters

6. Finally it doesn't seems to generate valid swagger, but I understand it's just a sketch.

Never checked that ;)

gabisurita · 2016-12-10T20:43:53Z

@delijati, great! We may wait for @glasserc, @Natim and @leplatrem to discuss this,but for now my impressions are:

I think documenting responses with docstrings might be too much information on them. I think we should find a better option.
I'm not sure about that, it would need proper investigation.
The problem is that as far as I know the querystrings are not validated with cornice schemas, but on kinto.core.resource.
Great.
I mean securityDefinitions and /path/{path}/{method}/security fields on the swagger document.

glasserc · 2016-12-12T21:17:57Z

Sorry I wasn't able to look at this for so long!

I haven't had a chance to look at the memory issue you encountered. I'm glad you found a workaround. Maybe that's good enough, or maybe we should dig deeper?
The tests look very thorough. Testing the definitions in isolation is a great idea.
Adding a new kind of request means adding a new test to the swagger tests. Maybe it would be cool if there were a way to avoid this? For example, maybe we could have a test that just makes every request possible. Or is it at least possible to verify that all requests in the spec were exercised by the test suite, and fail if one wasn't? (This might not work for error responses, which I think we still have to write out explicitly.)
If it makes sense to use cornice_swagger, then go ahead, but if it doesn't suit our use cases, then I don't want to spend a lot of time trying to force it to fit. In our meeting today, you mentioned that our Colander schemae are actually shared, so probably not a good match for a user-facing schema. In addition, some of our API involves custom headers, which I bet aren't captured in any Colander schema. Both of these seem like good reasons why cornice_swagger wouldn't fit.
@gabisurita proposed making Swagger stuff into a plugin. @Natim didn't have any objections. @leplatrem ? We agreed that we should proceed as-is (i.e. in this repo) for now, since moving it should be easy once it exists. I think it should stay in this repo as long as we're maintaining it manually.
@gabisurita asked where the Swagger view should live (kinto or kinto.core). I think that as long as it's being maintained manually, it should live in kinto, given that the views that are documented are there too. If we come up with some way of generating it automatically, it could live in kinto.core so that projects "inheriting" from kinto.core could get Swagger support too. What do you think @Natim @leplatrem ?

Custom headers like If-Match and If-Not-Match weren't there when I looked at this earlier, but I see that they're now. In a meeting today, @gabisurita said that support for headers has just landed in Swagger 2.0.

…i/swagger-codegen#1318 (comment)

Natim

This work is impressive, just a few nits.

Bravo 👍

Natim · 2016-12-21T16:02:42Z

docs/api/1.x/openapi.rst

+client generation.
+
+.. important::
+    OpenAPI 2.0 currently doensn't some features that are present on Kinto API.


OpenAPI 2.0 currently doensn't some features that are present on Kinto API.

Do you mean:

OpenAPI 2.0 currently doesn't support some features that are present on Kinto API.

Natim · 2016-12-21T16:03:51Z

docs/api/1.x/openapi.rst

+    Also, :ref:`collection defined schemas <collection-json-schema>`,
+    are not validated, So if you're using client
+    generation, you may need to implement these features.
+


Can you add a note on how you choose to handle this limitations in the SWAGGER file?

Natim · 2016-12-21T16:05:43Z

docs/api/1.x/openapi.rst

+``swagger.yaml`` file on the package root containing an OpenAPI extension
+for the main specification. You may use it to include additional resources
+that the plugin provides or override the original definitions. You may disable
+Swagger extensions by setting ``kinto.swagger_extensions`` to ``False`` on the


I would rather have a default plugin then rather than yet another way to activate / deactivate features.

Note that if you can deactivate it we probably need a capability for it.
Any reason why you want to be able to deactivate it?

It may be unlikely, but I think it possible to have conflicts in plugins. But I think we can drop this keep it as a default, and reintroduce if we need it later. What do you think?

Natim · 2016-12-21T16:10:06Z

requirements.txt

@@ -16,6 +16,7 @@ pyramid==1.7.3
 pyramid-multiauth==0.9.0
 pyramid-tm==1.1.1
 python-dateutil==2.6.0
+PyYAML==3.12


We should use ruamel.yaml rather than PyYAML

http://yaml.readthedocs.io/en/latest/pyyaml.html

And also because we had this discussion for kinto-wizard and we made this choice there: https://github.com/Kinto/kinto-wizard/blob/master/setup.py

Natim · 2016-12-21T16:12:05Z

setup.py

@@ -26,6 +27,7 @@ def read_file(filename):
    'jsonpatch',
    'python-dateutil',
    'pyramid_multiauth >= 0.8',  # User on policy selected event.
+    'PyYAML',


ruamel.yaml

glasserc

This is certainly getting very close! I don't really understand why turning additionalProperties from {"description": "blah"} to {} fixes anything. Could you do some investigation to figure out why this causes a bug and exactly where the bug is?

glasserc · 2016-12-21T15:54:36Z

docs/api/1.x/openapi.rst

+By default all includes on the configuration file will be prompt for a
+``swagger.yaml`` file on the package root containing an OpenAPI extension
+for the main specification. You may use it to include additional resources
+that the plugin provides or override the original definitions. You may disable


An example might help here. We should at least say something like "These YAML files are merged recursively with the Kinto swagger.yaml file".

glasserc · 2016-12-21T16:08:27Z

kinto/core/views/swagger.py

-    # Include securityDefinitions that are not on default security
-    for name, prop in auth_types.items():
+    # include securityDefinitions that are not on default security options
+    for name, prop in security_defs.items():
        security_def = {name: prop.get('scopes', {}).keys()}


I think some more comments would be helpful here to readers who don't already know much about the OpenAPI spec. How about: The 'security' field lists all acceptable ways of authenticating, so no plugin should add something to 'securityDefinitions' without it being here too, and then A 'security' requirement is an object of the form {security_mechanism: ['list', 'of', 'oauth2', 'scopes']} # or an empty list if oauth2 isn't used.

glasserc · 2016-12-21T16:25:40Z

kinto/swagger.yaml

+      on the default error message.
+      4. [Collection schemas](http://kinto.readthedocs.io/en/stable/api/1.x/collections.html#collection-json-schema)
+      can be provided when defining a collection and should be a valid JSON Schema object,
+      but this is not validated by this specification.


I think we should unify this list of limitations with the ones in the documentation, and provide a link to them.

glasserc · 2016-12-21T16:28:19Z

docs/api/1.x/openapi.rst

+    OpenAPI 2.0 currently doensn't some features that are present on Kinto API.
+    Some known limitations are
+    lack of validation on or clauses (e.g. provide `data` or `permissions` in PATCH
+    operations), no support for non schema querystrings, as used on :ref:`filtering <filtering>`


, and provide request or default in /batch requests

Also, I'd like to see a comment about whatever we ended up doing with additionalProperties and what bugs that fixes in what libraries, etc.

glasserc · 2016-12-21T16:30:44Z

docs/api/1.x/openapi.rst

+GET /swagger.json
+=================
+
+Returns the OpenAPI specification for the running instance of Kinto on JSON format.


We should also make a note that this is the ONLY supported way to get the spec, since the running code does some transformations on the data, for example updating the host and listing the accepted security mechanisms.

glasserc · 2016-12-21T16:31:44Z

docs/api/1.x/openapi.rst

+    lack of validation on or clauses (e.g. provide `data` or `permissions` in PATCH
+    operations), no support for non schema querystrings, as used on :ref:`filtering <filtering>`
+    with additional fields, and no optional response headers, as required for
+    `Backoff signs`<http://kinto.readthedocs.io/en/stable/api/1.x/backoff.html>`.


Something got messed up with your RST here. I think you have an extra backtick before your link. You might also need an underscore after the closing backtick.

gabisurita · 2016-12-21T17:48:12Z

I don't really understand why turning additionalProperties from {"description": "blah"} to {} fixes anything.

In terms of logic that's super weird, but following the traceback for the error, unmarshal_model, has a special case for empty objects, which skips unmarshal_object, that explicitly checks for type. So there's a indeed a special case to handle empty objects. We may call it a bug on bravado, but at this point I don't really know where the bug is... ¯\(ツ)/¯

/home/travis/build/gabisurita/kinto/.tox/py34/lib/python3.4/site-packages/bravado_core/unmarshal.py:173: in unmarshal_model

    model_as_dict = unmarshal_object(swagger_spec, model_spec, model_value)

/home/travis/build/gabisurita/kinto/.tox/py34/lib/python3.4/site-packages/bravado_core/unmarshal.py:133: in unmarshal_object

    result[k] = unmarshal_schema_object(swagger_spec, prop_spec, v)

/home/travis/build/gabisurita/kinto/.tox/py34/lib/python3.4/site-packages/bravado_core/unmarshal.py:54: in unmarshal_schema_object

))))

E           bravado_core.exception.SwaggerMappingError: The following schema object is missing a type field: {'description': 'Any extra field that applies.'}

glasserc · 2016-12-22T01:01:03Z

OK, I'm r+ on this. @Natim @leplatrem ?

leplatrem · 2016-12-22T14:55:35Z

I'm in favor of merging it too!

There's one little detail that we discussed recently: the URL /swagger.json. We don't have this .json suffix in any of the other endpoints, and since the spec is evolving to Open API, maybe we should not insist too much on the former name swagger.

In Shavar @tarekziade used the URL /__api__ which is agnostic and consistent with the other ones like /__version__ or /__heartbeat__. mozilla-services/shavar#78

I don't want to delay the merging of this PR, but if you agree we can create an issue and think about it before we release the next Kinto version :)

Congrats!

gabisurita · 2016-12-22T15:15:00Z

There's one little detail that we discussed recently: the URL /swagger.json. We don't have this .json suffix in any of the other endpoints, and since the spec is evolving to Open API, maybe we should not insist too much on the former name swagger.

I agree. According to spec, there's still a convention in using swagger.json to refer to OpenAPI description files, but it doesn't say anything about resource paths.

glasserc · 2016-12-22T15:26:55Z

tests/core/test_views_swagger.py

@@ -0,0 +1,65 @@
+import unittest
+import mock
+import yaml


Do we have to change this to ruamel.yaml now that we've updated the dependency? Sorry if this is a stupid question, I've never used ruamel.yaml before...

Yes, we should. This should have raised on tests, but I guess there's another package that imports pyYAML.

glasserc · 2016-12-22T16:51:48Z

🎊

leplatrem · 2016-12-22T22:37:16Z

gabisurita added 5 commits December 8, 2016 16:06

swagger spec

46ba48f

serve spec at /swagger.json

84c778d

add tests

6c80d7f

add docs

4bc8b22

update requirements

e5f0184

gabisurita added the in progress label Dec 9, 2016

gabisurita added 4 commits December 10, 2016 17:10

use yaml by default instead of json

3215e92

add plugin and configuration support

ca7268b

Merge branch 'master' into 631-swagger - fix docs

e784934

dummy fix memory issues

a4cf5eb

gabisurita force-pushed the 631-swagger branch from 0069e55 to a4cf5eb Compare December 10, 2016 20:46

add headers and improve organization

3fee325

gabisurita added 4 commits December 14, 2016 12:40

add descriptions and increase code reuse

a54c4bd

add automatic resource/path validation

a649c68

remove repetitive tests

e9c38b1

allow __version__ to fail

9a1fbd7

gabisurita force-pushed the 631-swagger branch from 1b13119 to 9a1fbd7 Compare December 15, 2016 13:39

add an extra Kinto tag to improve generated code quality

290f48d

clarify default security generation and fix py3

277b44c

gabisurita force-pushed the 631-swagger branch from 27e7e62 to 26d122c Compare December 20, 2016 23:35

gabisurita added 2 commits December 20, 2016 21:35

remove hardcoded properties

26d122c

solve conflicts from release 5.1.0

7560985

gabisurita force-pushed the 631-swagger branch from ceea4c2 to 60cf185 Compare December 21, 2016 00:36

drop metaprogramming and go back to one testcase per resource approach

7f2c091

gabisurita force-pushed the 631-swagger branch from 60cf185 to 9519ccd Compare December 21, 2016 14:45

gabisurita added 3 commits December 21, 2016 12:45

define additionalAttributes as {} to match JSONSchema true swagger-ap…

9519ccd

…i/swagger-codegen#1318 (comment)

(re)update CHANGELOG

0600945

remove hardcoded https

0c8e8e3

Natim approved these changes Dec 21, 2016

View reviewed changes

glasserc reviewed Dec 21, 2016

View reviewed changes

gabisurita force-pushed the 631-swagger branch from 949a562 to 0bd3081 Compare December 21, 2016 21:18

gabisurita added 2 commits December 21, 2016 19:18

@Natim and @glasserc comments

0bd3081

add Total-Records header

f5d94e0

leplatrem approved these changes Dec 22, 2016

View reviewed changes

change path to /__api__

91f0a04

glasserc reviewed Dec 22, 2016

View reviewed changes

gabisurita force-pushed the 631-swagger branch from 8a7ac47 to 250e78d Compare December 22, 2016 15:49

fix ruamel.yaml import

250e78d

glasserc merged commit 8f18dc1 into Kinto:master Dec 22, 2016

gabisurita deleted the 631-swagger branch December 23, 2016 06:44

gabisurita mentioned this pull request Dec 27, 2016

Generate the OpenAPI specification from Python code. #1006

Closed

gabisurita mentioned this pull request Mar 8, 2017

Speedup tests #1144

Merged

Add OpenAPI/Swagger spec (fixes #631) #977

Add OpenAPI/Swagger spec (fixes #631) #977

Conversation

gabisurita commented Dec 8, 2016 • edited

delijati commented Dec 9, 2016

gabisurita commented Dec 9, 2016 • edited

delijati commented Dec 9, 2016

gabisurita commented Dec 9, 2016 • edited

delijati commented Dec 10, 2016

delijati commented Dec 10, 2016

gabisurita commented Dec 10, 2016 • edited

delijati commented Dec 10, 2016

gabisurita commented Dec 10, 2016 • edited

delijati commented Dec 10, 2016

gabisurita commented Dec 10, 2016

glasserc commented Dec 12, 2016

Natim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita commented Dec 21, 2016 • edited

glasserc commented Dec 22, 2016

leplatrem commented Dec 22, 2016

gabisurita commented Dec 22, 2016

Choose a reason for hiding this comment

gabisurita Dec 22, 2016 • edited

Choose a reason for hiding this comment

glasserc commented Dec 22, 2016

leplatrem commented Dec 22, 2016

gabisurita commented Dec 8, 2016 •

edited

gabisurita commented Dec 9, 2016 •

edited

gabisurita commented Dec 9, 2016 •

edited

gabisurita commented Dec 10, 2016 •

edited

gabisurita commented Dec 10, 2016 •

edited

gabisurita commented Dec 21, 2016 •

edited

gabisurita Dec 22, 2016 •

edited