Swagger rebased updated #145

NotSpecial · 2016-12-10T16:04:42Z

Fancy new swagger interface for /docs added by hermann

hermannsblum · 2016-12-10T16:10:15Z

Seems like there are still some rebase problems, travis fails...

coveralls · 2016-12-10T16:22:06Z

Coverage increased (+0.04%) to 87.077% when pulling be4c594 on swagger_rebased_updated into dffbf98 on master.

cburchert

Are we sure we have to include the whole swagger code in the API?

The design looks weird as well. Can we make the resource description smaller, in a new line or bold the resource name? Currently the resource name and the description are in one block, I don't like that.

We loose a lot of documentation like max and min values and additional validators. Can we at least give something like the resource definition or a link to it?

Otherwise see the comments I left. I think some things should be changed.

cburchert · 2016-12-10T17:21:56Z

amivapi/auth/sessions.py

+        #    'POST': "Login and aquire a login token. Post the fields "
+        #    "'username' and 'password', the response will contain the "
+        #    "token. 'username' can be either nethz, mail, or user_id",
+        #    'GET': "Check token(s)."}


Is this documentation lost now? If yes, can we add it somewhere else?
If no, can we remove this comment?

cburchert · 2016-12-10T17:32:24Z

amivapi/docs.py

+    add_documentation({'definitions': {'User': {'properties': {
+        'password': {'default': ''},
+        'nethz': {'default': ''}
+    }}}})


What is the purpose of this? We generate empty documentation fields?

Swagger in the currrent version does not support

Default: null

And crashes. They decided to fix this only in later versions, so for now we have to replace all null fields with something else

cburchert · 2016-12-10T17:35:45Z

amivapi/docs.py

+            'name': 'Authorization',
+            'in': 'header',
+            'description': 'enter a token you got with POST to /sessions'
+        }


This is badly worded, as it suggests API keys and tokens are the same. Looking at the created website, I propose:

name: Authorization header
remove in
description: 'enter either a session token, you retrived using POST to /sessions, or an API key defined in the server config.'

This is swagger code. Swagger supports 3 types of authorization schemes, and they call token-based authorization 'apiKey'

'name' is the key of the header field, this cant be changes therefore.

But if course we can adapt the description field

cburchert · 2016-12-10T17:36:04Z

amivapi/events/model.py

+        '2014-12-20T11:50:06Z',
+        # 'methods': {
+        #     'GET': 'You are always allowed, even without session, '
+        #     'to view AMIV-Events'


Again: do we loose this? remove comment?

cburchert · 2016-12-10T17:36:48Z

amivapi/events/model.py

+        'events.',
+        # 'methods': {
+        #     'PATCH': 'Only additional_fields can be changed'
+        # }


Same thing again

cburchert · 2016-12-10T17:42:27Z

amivapi/settings.py

+    'termsOfService': 'todo',
+    'contact': {
+        'name': 'AMIV an der ETH',
+        'url': 'http://amiv.ch/'


https please

cburchert · 2016-12-10T17:43:30Z

amivapi/settings.py

+    'title': 'AMIVAPI',
+    'version': '0.1-dev',
+    'description': "The REST API behind most of AMIV's web services.",
+    'termsOfService': 'todo',


Add a # TODO comment, so this shows up in grep

cburchert · 2016-12-10T17:45:06Z

amivapi/settings.py

+        'url': 'http://amiv.ch/'
+    },
+    'license': {
+        'name': 'GNU',


This just puts "GNU" without context to the website. We need to write what it is.
Change to "GNU Affero General Public License"?

cburchert · 2016-12-10T17:46:50Z

amivapi/settings.py

+                       'add_permitted_methods_after_fetch_item',
+                       'add_permitted_methods_after_fetch_resource',
+                       'add_permitted_methods_for_home',
+                       'add_permitted_methods_after_update']


This does not seem to work. add_permitted_methods_after_insert is shown under POST to joboffers.
Any ideas?

cburchert · 2016-12-10T17:48:43Z

amivapi/settings.py

+                       'add_permitted_methods_after_fetch_resource',
+                       'add_permitted_methods_for_home',
+                       'add_permitted_methods_after_update']
+X_DOMAINS = ['http://127.0.0.1:5000',  # The domain where Swagger UI is running


This IP should not be hardcoded, this breaks the documentation in many setups. The address should be available via flask. (maybe SERVER_NAME config value?)

cburchert · 2016-12-11T02:50:51Z

I fixed all the comments I made apart from the one about the code in docs.py line 50. It is not clear to me, what that code is for.

Still design does not look good and we are missing the validator information.

cburchert · 2016-12-11T02:54:19Z

Looking at other swagger based documentation, I think we should move the description of resources to implementation_notes and just have a short string for the description.

hermannsblum · 2016-12-11T13:00:24Z

I fixed the authorization again following my comments above

hermannsblum · 2016-12-11T13:08:12Z

I think the documentation with realtion to special HTTP methods is OK now, even though it should in the end only be displayed for the actual method you look at right now.

Also related to the more detailed description and "summary": How do you suggest to structure the json? We could think of something like this:

    'events': {
        'summary': 'Anything happening in the AMIV'.
        'description': 'An Event is basically everything happening in the '
        'AMIV. All time fields have the format YYYY-MM-DDThh:mmZ, e.g. '
        '2014-12-20T11:50:06Z\n\n'
        'method_documentation' {
            'GET': 'This is public even without a session'
        }

We would then adapt the mapping into the swagger fields, this is done by eve-swagger and therefore easily adaptable as we already use our own fork

cburchert · 2016-12-12T13:08:37Z

I like that approach.

hermannsblum · 2016-12-12T22:29:44Z

For the documentation of validation rules: This is a known swagger issue right now.

It looks like they lost this with the implementation of the example objects as they had it before. Apparently you can see some stuff by hovering, but I do not consider this a solution.

As we can expect this issue to be worked on, I suggest an easy solution for the moment printing out the schema into the implementation notes via our version of eve-swagger.

hermannsblum · 2016-12-12T23:18:22Z

Doing some more research 😃

I think I actually prefer this much more over swagger: http://rebilly.github.io/ReDoc/
It uses the same documentation resource (swagger-like JSON description is basis of OpenAPI), but describes actually all the stuff we want.
However, it does not yet have an interactive console to try out requests like swagger does...

What are your opinions?

cburchert · 2017-02-28T13:28:00Z

I have at least two issues:

import error: bootstrap.py imports docs, but the file is called documentation.py
I don't like the structure. There are three things in the amivapi package, which all belong together (swagger_ui folder, documentation.py and ReDoc folder). The swagger_ui folder looks like it is API code, while actually it is a library. I would recommend we either have a folder "lib" or "external" or whatever and put it in there, or we move it outside of amivapi altogether and somehow reference it.

An easy solution to 2 might be to have:

amivapi/documentation/lib/swagger_ui/
amivapi/documentation/ReDoc/
amivapi/documentation/documentation.py

hermannsblum · 2017-03-06T14:20:50Z

You fixed the import error by yourself, thanks and sorry for that 🤙

Concerning the structure: This was actually only there as I never really got a feedback whether we should use ReDoc or SwaggerUI. As nobody seems to care too much, I simply decided for ReDoc, which we can include as external JS and therefore we only need the file amivapi/documentation.py which now inlcudes the 14 lines HTML template.

coveralls · 2017-03-06T19:42:50Z

Coverage increased (+0.01%) to 87.242% when pulling b971e94 on swagger_rebased_updated into 44de894 on master.

hermannsblum and others added 13 commits December 10, 2016 15:10

add swagger as endpoint documentation

6c87cd0

remove eve-docs

0879025

remove eve-docs also from requirements

d7597f7

refractor endpoint descriptions to be parsable by eve-swagger

d2bcc66

add swagger ui resources

ec54ab5

add authentication description for swagger client

621c055

fix critical bug where delete hooks get overwritten

6c1ebcd

add description to explain how to authorize in swagger

694a70f

hide auth hooks in swagger

b09a99a

hide all item methods at swagger start

f816860

Added validator to docs to stop errors about unknown field

515231c

Updated requirements

bcfc0ac

add js library of swagger_ui

5761591

hermannsblum requested a review from cburchert December 10, 2016 16:06

This was referenced Dec 10, 2016

Switch to Eve-Swagger #140

Closed

Authentication example wrong in docs #137

Closed

NotSpecial added 2 commits December 10, 2016 17:13

Updated a description in events

c4ffedb

Removed flask bootstrap, not needed anymore

be4c594

hermannsblum mentioned this pull request Dec 10, 2016

Provide API Schema in jsonschema format #91

Closed

cburchert suggested changes Dec 10, 2016

View reviewed changes

cburchert added 5 commits December 11, 2016 03:42

Updated some documentation

63ef7c3

Added VERSION constant

6343af9

Cosmetic improvements to swagger

168d85f

Removed redundant settings

a94f0d5

Fixed missing comma in dictionary

f445a06

fix swagger authorization again

e7137ca

hermannsblum added 3 commits December 16, 2016 11:19

switch from swagger-ui to redoc documentation rendering

21de841

rename documentation blueprint file to avoid mixup with studydocs

d50af80

correct documentation format for user model

7e31224

cburchert and others added 4 commits February 28, 2017 14:30

Removed comment with old documentation

5eed252

Fixed wrong import

7389e80

remove swagger-ui and move redoc template into string

89fb92a

update comments as we now use ReDoc instead of SwaggerUI

88b3fe3

hermannsblum added 2 commits March 6, 2017 17:38

Merge branch 'master' into swagger_rebased_updated

c9967d3

fix flake8 issues

248dcc5

cburchert approved these changes Mar 6, 2017

View reviewed changes

Merge branch 'master' into swagger_rebased_updated

b971e94

cburchert merged commit b971e94 into master Mar 6, 2017

cburchert deleted the swagger_rebased_updated branch March 6, 2017 19:45

hermannsblum mentioned this pull request Mar 6, 2017

JSON schema vs Cerberus schema #158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swagger rebased updated #145

Swagger rebased updated #145

NotSpecial commented Dec 10, 2016

hermannsblum commented Dec 10, 2016

coveralls commented Dec 10, 2016 •

edited

Loading

cburchert left a comment

cburchert Dec 10, 2016

cburchert Dec 10, 2016

hermannsblum Dec 11, 2016

cburchert Dec 10, 2016

hermannsblum Dec 11, 2016

cburchert Dec 10, 2016

cburchert Dec 10, 2016

cburchert Dec 10, 2016

cburchert Dec 10, 2016

cburchert Dec 10, 2016

cburchert Dec 10, 2016

cburchert Dec 10, 2016

cburchert commented Dec 11, 2016

cburchert commented Dec 11, 2016

hermannsblum commented Dec 11, 2016

hermannsblum commented Dec 11, 2016

cburchert commented Dec 12, 2016

hermannsblum commented Dec 12, 2016

hermannsblum commented Dec 12, 2016

cburchert commented Feb 28, 2017 •

edited

Loading

hermannsblum commented Mar 6, 2017

coveralls commented Mar 6, 2017 •

edited

Loading

Swagger rebased updated #145

Swagger rebased updated #145

Conversation

NotSpecial commented Dec 10, 2016

hermannsblum commented Dec 10, 2016

coveralls commented Dec 10, 2016 • edited Loading

cburchert 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburchert commented Dec 11, 2016

cburchert commented Dec 11, 2016

hermannsblum commented Dec 11, 2016

hermannsblum commented Dec 11, 2016

cburchert commented Dec 12, 2016

hermannsblum commented Dec 12, 2016

hermannsblum commented Dec 12, 2016

cburchert commented Feb 28, 2017 • edited Loading

hermannsblum commented Mar 6, 2017

coveralls commented Mar 6, 2017 • edited Loading

coveralls commented Dec 10, 2016 •

edited

Loading

cburchert commented Feb 28, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading