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 new REST-based Webapi and interactive webapi Docs with Swagger-UI #276

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

bendikro
Copy link
Member

@bendikro bendikro commented Apr 1, 2020

The docstring_parser library uses type annotation syntax not supported in the python version on travis.

@bendikro bendikro force-pushed the Issue/develop/3308/new-webapi-swagger-ui branch 2 times, most recently from e829120 to e25f7e6 Compare April 12, 2020 19:08
Copy link
Member

@cas-- cas-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swagger is a good choice, I would prefer not storing the source code in codebase. Can we install with npm and copy what is needed in setup/install?

Is webapidoc is bit long-winded? Would dropping the web prefix apidoc make it too generic and confused with deluge api? wapidoc, restdoc? Not sure here if there is much other option, any thoughts?

Also the endpoints seem very long too '/webapidocs/web/deregister_event_listener', at least drop the 'webapidocs' part? Wonder if we customize, such as:

/torrent/files
/torrent/info
/events/register
/events/deregister

Which leads to another question, does the api need a version too?

setup.py Outdated Show resolved Hide resolved
deluge/httpdownloader.py Outdated Show resolved Hide resolved
@cas--
Copy link
Member

cas-- commented May 10, 2020

The openapi docs are served by the app but perhaps we also want them in docs with https://github.com/sphinx-contrib/openapi

@bendikro
Copy link
Member Author

Swagger is a good choice, I would prefer not storing the source code in codebase. Can we install with npm and copy what is needed in setup/install?

Certainly. I just included the source for simplicity. How (where) would you suggest doing this?

Is webapidoc is bit long-winded? Would dropping the web prefix apidoc make it too generic and confused with deluge api? wapidoc, restdoc? Not sure here if there is much other option, any thoughts?

Can change to api to make it shorter.

Also the endpoints seem very long too '/webapidocs/web/deregister_event_listener', at least drop the 'webapidocs' part? Wonder if we customize, such as:

/torrent/files
/torrent/info
/events/register
/events/deregister

webapidocs is the base prefix, so after changing that to api, the next part, web reflects the path of the function in the source. This matches the values given in the get_methods function for the old json api endpoint.

curl -X GET "http://0.0.0.0:8112/api/web/get_methods?id=1&pretty=pretty" -H "accept: application/json"
{
  "result": [
    "web.add_host",
    "web.add_torrents",
    "web.connect",
    "web.connected",
....
...

@bendikro bendikro force-pushed the Issue/develop/3308/new-webapi-swagger-ui branch 3 times, most recently from 4902225 to 5dd38ef Compare May 11, 2020 18:50
DEFAULT_PREFS = {
'enabled': False,
'ssl': False,
'webapidoc_enabled': False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Do we need a config to enable/disable docs?
  • If so should we enable it by default? Users will never know about it and like me annoyed when they need to set a config ;)
  • And rename this setting to just docs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. You think it should always be available? You'll have to log in to do anything of importance anyways, so maybe we can drop the setting.

@@ -75,6 +86,7 @@ def start_server(self):

self.server.port = self.config['port']
self.server.https = self.config['ssl']
self.server.setup_webapidoc(enabled=self.config['webapidoc_enabled'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't this setup should be part of the plugin code, the server itself should check config and do the setup

@@ -748,6 +831,14 @@ def start_normal(self):
ip = self.socket.getHost().host
ip = '[%s]' % ip if is_ipv6(ip) else ip
log.info('Serving at http://%s:%s%s', ip, self.port, self.base)
if self.webapidoc_enabled:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what value this log entry adds

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows where the docs are served. Isn't that useful in itself?

@@ -765,6 +856,14 @@ def start_ssl(self):
ip = self.socket.getHost().host
ip = '[%s]' % ip if is_ipv6(ip) else ip
log.info('Serving at https://%s:%s%s', ip, self.port, self.base)
if self.webapidoc_enabled:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what value this log entry adds

@@ -740,6 +819,10 @@ def start(self):

component.get('Web').enable()

# TODO: Check if correct
# Plugins must be enabled here to register exported json functions
self.plugins.enable_plugins()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were they not exported before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem I had was that the plugins list in the config was empty unless enabled here.

self.plugins.disable_plugins()
# TODO: Check if this is correct
# Disabling plugins removes them from enabled_plugins list in web.conf???
# self.plugins.disable_plugins()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, could we put these plugin issues in a separate commit?

@@ -724,6 +798,11 @@ def win_handler(ctrl_type):

SetConsoleCtrlHandler(win_handler)

def setup_webapidoc(self, enabled=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate method naming, what is the purpose of this one?

@@ -536,6 +585,28 @@ def __init__(self):
theme = CONFIG_DEFAULTS.get('theme')
self.__stylesheets.insert(1, 'themes/css/xtheme-%s.css' % theme)

def setup_webapidoc(self, enabled=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that enabling the docs should be read from config not passed as arg, thoughts?


self.putChild(self.webapidoc_path.encode(), self.webapidoc_resource)
self.js.add_script_folder(path, rpath('js', path))
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to handle users switching the config from enable to disable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. Would be easier to just have it always enabled

@@ -536,6 +536,8 @@ def run(self):

setup_requires = ['setuptools', 'wheel']
install_requires = [
"apispec; python_version >= '3'",
Copy link
Member

@cas-- cas-- May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thought that I'm not sure we need to pin this to Python 3? Only apispec v3.0.0 is Python 3 only, are you using v3 specific methods: https://apispec.readthedocs.io/en/latest/changelog.html#id5

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'm not sure as I haven't tested on python 2.

@bendikro bendikro force-pushed the Issue/develop/3308/new-webapi-swagger-ui branch from f2b4d00 to 81ae652 Compare September 15, 2020 12:14
bendikro and others added 4 commits September 15, 2020 14:34
…r-UI

The new webapi served on /api creates separate endpoints for each
exported python function, e.g. api/web/login

Webapi functions are exported with

webapi = WebapiNamespace("Webapi")

@webapi.post
def exported_post_func(self, arg):
   ...

@webapi.get
def exported_get_func(self):
   ...

The interactive webapi documenation is generated with swagger-ui and served
on http://0.0.0.0:8112/webapidoc

The function doc of the exported functions is parsed with GoogleStyleDocPlugin
that relies on 'docstring_parser' which only supported on python >= 3.6.
With python < 3.6, these doc strings are not included in the api docs.
There might be a bug causing issues with plugin functions not being
registered with the json api.

This should eb investigated
@bendikro bendikro force-pushed the Issue/develop/3308/new-webapi-swagger-ui branch from 81ae652 to 7a53887 Compare September 15, 2020 12:46
@deluge-torrent deluge-torrent deleted a comment from Erarshad Sep 22, 2021
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

Successfully merging this pull request may close these issues.

2 participants