Repository: kytos/kytos

Issue:
Error if two NApps have methods with the same name decorated with @rest
If two NApps declare REST endpoints in methods with the same name, Flask raises an exception. One way to solve this problem is to use Flask [blueprints](https://flask.palletsprojects.com/en/1.1.x/blueprints).

Discussion:

Patch:
diff --git a/Makefile b/Makefile
index 42b498c..da50505 100644
--- a/Makefile
+++ b/Makefile
@@ -1,12 +1,12 @@
 build: clean prepare
-	python3 setup.py sdist bdist_wheel
+	python3.6 setup.py sdist bdist_wheel
 	ls -l dist/
 
 clean:
 	rm -rf build/ dist/ *.egg-info/ kytos/web-ui-*
 
 prepare:
-	pip3 install --upgrade pip setuptools wheel twine
+	pip3.6 install --upgrade pip setuptools wheel twine
 
 testupload: build
 	twine upload -r pypitest dist/*
diff --git a/kytos/core/api_server.py b/kytos/core/api_server.py
index b14d80d..f10c3b0 100644
--- a/kytos/core/api_server.py
+++ b/kytos/core/api_server.py
@@ -12,7 +12,7 @@ from http import HTTPStatus
 from urllib.error import HTTPError, URLError
 from urllib.request import urlopen, urlretrieve
 
-from flask import Flask, jsonify, request, send_file
+from flask import Blueprint, Flask, jsonify, request, send_file
 from flask_cors import CORS
 from flask_socketio import SocketIO, join_room, leave_room
 from werkzeug.exceptions import HTTPException
@@ -98,7 +98,8 @@ class APIServer:
                       stacklevel=2)
         if url.startswith('/'):
             url = url[1:]
-        self._start_endpoint(f'/kytos/{url}', function, methods=methods)
+        self._start_endpoint(self.app, f'/kytos/{url}', function,
+                             methods=methods)
 
     def start_api(self):
         """Start this APIServer instance API.
@@ -123,7 +124,8 @@ class APIServer:
 
         Not used by NApps, but controller.
         """
-        self._start_endpoint(self._CORE_PREFIX + rule, function, **options)
+        self._start_endpoint(self.app, self._CORE_PREFIX + rule, function,
+                             **options)
 
     def _register_web_ui(self):
         """Register routes to the admin-ui homepage."""
@@ -291,15 +293,27 @@ class APIServer:
     def register_napp_endpoints(self, napp):
         """Add all NApp REST endpoints with @rest decorator.
 
+        We are using Flask Blueprints to register these endpoints. Blueprints
+        are essentially the Flask equivalent of Python modules and are used to
+        keep related logic and assets grouped and separated from one another.
+
         URLs will be prefixed with ``/api/{username}/{napp_name}/``.
 
         Args:
             napp (Napp): Napp instance to register new endpoints.
         """
+        # Create a Flask Blueprint for a specific NApp
+        napp_blueprint = Blueprint(napp.napp_id, __name__)
+
+        # Start all endpoints for this NApp
         for function in self._get_decorated_functions(napp):
             for rule, options in function.route_params:
                 absolute_rule = self.get_absolute_rule(rule, napp)
-                self._start_endpoint(absolute_rule, function, **options)
+                self._start_endpoint(napp_blueprint, absolute_rule, function,
+                                     **options)
+
+        # Register this Flask Blueprint in the Flask App
+        self.app.register_blueprint(napp_blueprint)
 
     @staticmethod
     def _get_decorated_functions(napp):
@@ -323,14 +337,14 @@ class APIServer:
 
     # END decorator methods
 
-    def _start_endpoint(self, rule, function, **options):
+    def _start_endpoint(self, app, rule, function, **options):
         """Start ``function``'s endpoint.
 
         Forward parameters to ``Flask.add_url_rule`` mimicking Flask
         ``@route`` decorator.
         """
         endpoint = options.pop('endpoint', None)
-        self.app.add_url_rule(rule, endpoint, function, **options)
+        app.add_url_rule(rule, endpoint, function, **options)
         self.log.info('Started %s - %s', rule,
                       ', '.join(options.get('methods', self.DEFAULT_METHODS)))
 
@@ -357,6 +371,9 @@ class APIServer:
             self.app.url_map._rules.pop(index)
             # pylint: enable=protected-access
 
+        # Remove the Flask Blueprint of this NApp from the Flask App
+        self.app.blueprints.pop(napp.napp_id)
+
         self.log.info('The Rest endpoints from %s were disabled.', prefix)
 
     def register_core_napp_services(self):
diff --git a/requirements/dev.txt b/requirements/dev.txt
index 0efbe77..5feeef4 100644
--- a/requirements/dev.txt
+++ b/requirements/dev.txt
@@ -2,69 +2,50 @@
 # This file is autogenerated by pip-compile
 # To update, run:
 #
-#    pip-compile --output-file=requirements/dev.txt requirements/dev.in requirements/run.txt
+#    pip-compile --output-file=requirements/dev.txt requirements/dev.in
 #
 -e git+git://github.com/kytos/sphinx-theme.git#egg=kytos-sphinx-theme  # via -r requirements/dev.in
--e git+git://github.com/kytos/python-openflow.git#egg=python-openflow  # via -r requirements/dev.in, -r requirements/run.txt
+-e git+git://github.com/kytos/python-openflow.git#egg=python-openflow  # via -r requirements/dev.in
 alabaster==0.7.12         # via sphinx
 appdirs==1.4.3            # via virtualenv
-appnope==0.1.0            # via -r requirements/run.txt
 argh==0.26.2              # via sphinx-autobuild
 astroid==2.3.3            # via pylint
 attrs==19.3.0             # via pytest
 babel==2.8.0              # via sphinx
-backcall==0.1.0           # via -r requirements/run.txt, ipython
 certifi==2019.11.28       # via requests
 chardet==3.0.4            # via requests
-click==7.1.1              # via -r requirements/run.txt, flask, pip-tools
+click==7.1.1              # via pip-tools
 coverage==5.0.4           # via -r requirements/dev.in
-decorator==4.4.2          # via -r requirements/run.txt, ipython, traitlets
 distlib==0.3.0            # via virtualenv
 docopt==0.6.2             # via yala
-docutils==0.16            # via -r requirements/run.txt, python-daemon, sphinx
+docutils==0.16            # via sphinx
 filelock==3.0.12          # via tox, virtualenv
-flask-cors==3.0.8         # via -r requirements/run.txt
-flask-socketio==4.2.1     # via -r requirements/run.txt
-flask==1.1.2              # via -r requirements/run.txt, flask-cors, flask-socketio
 idna==2.9                 # via requests
 imagesize==1.2.0          # via sphinx
-ipython-genutils==0.2.0   # via -r requirements/run.txt, traitlets
-ipython==7.13.0           # via -r requirements/run.txt
+importlib-metadata==1.6.0  # via importlib-resources, pluggy, pytest, tox, virtualenv
+importlib-resources==1.4.0  # via virtualenv
 isort==4.3.21             # via pylint, yala
-itsdangerous==1.1.0       # via -r requirements/run.txt, flask
-janus==0.4.0              # via -r requirements/run.txt
-jedi==0.16.0              # via -r requirements/run.txt, ipython
-jinja2==2.11.1            # via -r requirements/run.txt, flask, sphinx
+jinja2==2.11.1            # via sphinx
 lazy-object-proxy==1.4.3  # via astroid
 livereload==2.6.1         # via sphinx-autobuild
-lockfile==0.12.2          # via -r requirements/run.txt, python-daemon
-markupsafe==1.1.1         # via -r requirements/run.txt, jinja2
+markupsafe==1.1.1         # via jinja2
 mccabe==0.6.1             # via pylint
 more-itertools==8.2.0     # via pytest
 packaging==20.3           # via pytest, sphinx, tox
-parso==0.6.2              # via -r requirements/run.txt, jedi
-pathtools==0.1.2          # via -r requirements/run.txt, sphinx-autobuild, watchdog
-pexpect==4.8.0            # via -r requirements/run.txt, ipython
-pickleshare==0.7.5        # via -r requirements/run.txt, ipython
+pathtools==0.1.2          # via sphinx-autobuild, watchdog
 pip-tools==4.5.1          # via -r requirements/dev.in
 pluggy==0.13.1            # via pytest, tox
 port_for==0.3.1           # via sphinx-autobuild
-prompt-toolkit==3.0.5     # via -r requirements/run.txt, ipython
-ptyprocess==0.6.0         # via -r requirements/run.txt, pexpect
 py==1.8.1                 # via pytest, tox
 pycodestyle==2.5.0        # via yala
-pygments==2.6.1           # via -r requirements/run.txt, ipython, sphinx
-pyjwt==1.7.1              # via -r requirements/run.txt
+pygments==2.6.1           # via sphinx
 pylint==2.4.4             # via yala
 pyparsing==2.4.6          # via packaging
 pytest==5.4.1             # via -r requirements/dev.in
-python-daemon==2.2.4      # via -r requirements/run.txt
-python-engineio==3.12.1   # via -r requirements/run.txt, python-socketio
-python-socketio==4.5.1    # via -r requirements/run.txt, flask-socketio
 pytz==2019.3              # via babel
 pyyaml==5.3.1             # via sphinx-autobuild
 requests==2.23.0          # via sphinx
-six==1.15.0               # via -r requirements/run.txt, astroid, flask-cors, livereload, packaging, pip-tools, python-engineio, python-socketio, tox, traitlets, virtualenv
+six==1.15.0               # via astroid, livereload, packaging, pip-tools, tox, virtualenv
 snowballstemmer==2.0.0    # via sphinx
 sphinx-autobuild==0.7.1   # via kytos-sphinx-theme
 sphinx==2.0.1             # via kytos-sphinx-theme
@@ -77,14 +58,14 @@ sphinxcontrib-serializinghtml==1.1.4  # via sphinx
 toml==0.10.0              # via tox
 tornado==6.0.4            # via livereload, sphinx-autobuild
 tox==3.14.6               # via -r requirements/dev.in
-traitlets==4.3.3          # via -r requirements/run.txt, ipython
+typed-ast==1.4.1          # via astroid
 urllib3==1.25.8           # via requests
 virtualenv==20.0.15       # via tox
-watchdog==0.10.2          # via -r requirements/run.txt, sphinx-autobuild
-wcwidth==0.1.9            # via -r requirements/run.txt, prompt-toolkit, pytest
-werkzeug==1.0.1           # via -r requirements/run.txt, flask
+watchdog==0.10.2          # via sphinx-autobuild
+wcwidth==0.1.9            # via pytest
 wrapt==1.11.2             # via astroid
 yala==2.2.0               # via -r requirements/dev.in
+zipp==3.1.0               # via importlib-metadata, importlib-resources
 
 # The following packages are considered to be unsafe in a requirements file:
 # setuptools


Test patch:
diff --git a/tests/unit/test_core/test_api_server.py b/tests/unit/test_core/test_api_server.py
index 7775586..1d98e71 100644
--- a/tests/unit/test_core/test_api_server.py
+++ b/tests/unit/test_core/test_api_server.py
@@ -393,13 +393,15 @@ class RESTNApp:  # pylint: disable=too-few-public-methods
     def __init__(self):
         self.username = 'test'
         self.name = 'MyNApp'
+        self.napp_id = 'test/MyNApp'
 
 
 class TestAPIDecorator(unittest.TestCase):
     """@rest should have the same effect as ``Flask.route``."""
 
     @classmethod
-    def test_flask_call(cls):
+    @patch('kytos.core.api_server.Blueprint')
+    def test_flask_call(cls, mock_blueprint):
         """@rest params should be forwarded to Flask."""
         rule = 'rule'
         # Use sentinels to be sure they are not changed.
@@ -412,9 +414,12 @@ class TestAPIDecorator(unittest.TestCase):
             def my_endpoint(self):
                 """Do nothing."""
 
+        blueprint = Mock()
+        mock_blueprint.return_value = blueprint
+
         napp = MyNApp()
-        server = cls._mock_api_server(napp)
-        server.app.add_url_rule.assert_called_once_with(
+        cls._mock_api_server(napp)
+        blueprint.add_url_rule.assert_called_once_with(
             '/api/test/MyNApp/' + rule, None, napp.my_endpoint, **options)
 
     @classmethod
@@ -426,6 +431,7 @@ class TestAPIDecorator(unittest.TestCase):
             def __init__(self):
                 self.username = 'test'
                 self.name = 'MyNApp'
+                self.napp_id = 'test/MyNApp'
 
         napp = MyNApp()
         server = cls._mock_api_server(napp)
@@ -447,9 +453,11 @@ class TestAPIDecorator(unittest.TestCase):
         # pylint: disable=protected-access
         server.app.url_map._rules.pop.assert_called_once_with(0)
         # pylint: enable=protected-access
+        server.app.blueprints.pop.assert_called_once_with(napp.napp_id)
 
     @classmethod
-    def test_rule_with_slash(cls):
+    @patch('kytos.core.api_server.Blueprint')
+    def test_rule_with_slash(cls, mock_blueprint):
         """There should be no double slashes in a rule."""
         class MyNApp(RESTNApp):  # pylint: disable=too-few-public-methods
             """API decorator example usage."""
@@ -458,10 +466,13 @@ class TestAPIDecorator(unittest.TestCase):
             def my_endpoint(self):
                 """Do nothing."""
 
-        cls._assert_rule_is_added(MyNApp)
+        blueprint = Mock()
+        mock_blueprint.return_value = blueprint
+        cls._assert_rule_is_added(MyNApp, blueprint)
 
     @classmethod
-    def test_rule_from_classmethod(cls):
+    @patch('kytos.core.api_server.Blueprint')
+    def test_rule_from_classmethod(cls, mock_blueprint):
         """Use class methods as endpoints as well."""
         class MyNApp(RESTNApp):  # pylint: disable=too-few-public-methods
             """API decorator example usage."""
@@ -471,10 +482,13 @@ class TestAPIDecorator(unittest.TestCase):
             def my_endpoint(cls):
                 """Do nothing."""
 
-        cls._assert_rule_is_added(MyNApp)
+        blueprint = Mock()
+        mock_blueprint.return_value = blueprint
+        cls._assert_rule_is_added(MyNApp, blueprint)
 
     @classmethod
-    def test_rule_from_staticmethod(cls):
+    @patch('kytos.core.api_server.Blueprint')
+    def test_rule_from_staticmethod(cls, mock_blueprint):
         """Use static methods as endpoints as well."""
         class MyNApp(RESTNApp):  # pylint: disable=too-few-public-methods
             """API decorator example usage."""
@@ -484,14 +498,16 @@ class TestAPIDecorator(unittest.TestCase):
             def my_endpoint():
                 """Do nothing."""
 
-        cls._assert_rule_is_added(MyNApp)
+        blueprint = Mock()
+        mock_blueprint.return_value = blueprint
+        cls._assert_rule_is_added(MyNApp, blueprint)
 
     @classmethod
-    def _assert_rule_is_added(cls, napp_class):
+    def _assert_rule_is_added(cls, napp_class, blueprint):
         """Assert Flask's add_url_rule was called with the right parameters."""
         napp = napp_class()
-        server = cls._mock_api_server(napp)
-        server.app.add_url_rule.assert_called_once_with(
+        cls._mock_api_server(napp)
+        blueprint.add_url_rule.assert_called_once_with(
             '/api/test/MyNApp/rule', None, napp.my_endpoint)
 
     @staticmethod
diff --git a/tests/unit/test_core/test_auth.py b/tests/unit/test_core/test_auth.py
index b84d772..3f6f467 100644
--- a/tests/unit/test_core/test_auth.py
+++ b/tests/unit/test_core/test_auth.py
@@ -56,7 +56,6 @@ class TestAuth(TestCase):
     @staticmethod
     def get_auth_test_client(auth):
         """Return a flask api test client."""
-        auth.controller.api_server.register_napp_endpoints(auth)
         return auth.controller.api_server.app.test_client()
 
     @patch('kytos.core.auth.Auth._create_superuser')
diff --git a/tests/unit/test_core/test_controller.py b/tests/unit/test_core/test_controller.py
index d365f21..20524f7 100644
--- a/tests/unit/test_core/test_controller.py
+++ b/tests/unit/test_core/test_controller.py
@@ -60,7 +60,8 @@ class TestController(TestCase):
         # Restore original state
         logging.root.handlers = handlers_bak
 
-    def test_unload_napp_listener(self):
+    @patch('kytos.core.api_server.APIServer.remove_napp_endpoints')
+    def test_unload_napp_listener(self, _):
         """Call NApp shutdown listener on unload."""
         username, napp_name = 'test', 'napp'
         listener = self._add_napp(username, napp_name)
@@ -69,7 +70,8 @@ class TestController(TestCase):
         self.controller.unload_napp(username, napp_name)
         listener.assert_called()
 
-    def test_unload_napp_other_listener(self):
+    @patch('kytos.core.api_server.APIServer.remove_napp_endpoints')
+    def test_unload_napp_other_listener(self, _):
         """Should not call other NApps' shutdown listener on unload."""
         username, napp_name = 'test', 'napp1'
         self._add_napp(username, napp_name)