-
Notifications
You must be signed in to change notification settings - Fork 339
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
base: develop
Are you sure you want to change the base?
Add new REST-based Webapi and interactive webapi Docs with Swagger-UI #276
Conversation
e829120
to
e25f7e6
Compare
There was a problem hiding this 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?
The openapi docs are served by the app but perhaps we also want them in docs with https://github.com/sphinx-contrib/openapi |
Certainly. I just included the source for simplicity. How (where) would you suggest doing this?
Can change to
|
4902225
to
5dd38ef
Compare
DEFAULT_PREFS = { | ||
'enabled': False, | ||
'ssl': False, | ||
'webapidoc_enabled': False, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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']) |
There was a problem hiding this comment.
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
deluge/ui/web/server.py
Outdated
@@ -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: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
deluge/ui/web/server.py
Outdated
@@ -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: |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
deluge/ui/web/server.py
Outdated
@@ -724,6 +798,11 @@ def win_handler(ctrl_type): | |||
|
|||
SetConsoleCtrlHandler(win_handler) | |||
|
|||
def setup_webapidoc(self, enabled=True): |
There was a problem hiding this comment.
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?
deluge/ui/web/server.py
Outdated
@@ -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): |
There was a problem hiding this comment.
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?
deluge/ui/web/server.py
Outdated
|
||
self.putChild(self.webapidoc_path.encode(), self.webapidoc_resource) | ||
self.js.add_script_folder(path, rpath('js', path)) | ||
else: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
f2b4d00
to
81ae652
Compare
…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
81ae652
to
7a53887
Compare
The
docstring_parser
library uses type annotation syntax not supported in the python version on travis.