Repository: girder/girder
Issue: Plugin routing gets reset on syntax error.

 - Begin developing your own plugin
 - Set the system-wide plugin routing setting so that your plugin serves its contents from "/" and so that girder-core is served from "/girder".
 - While working on your server-side code, "accidentally" introduce a syntax error.

At this point, Girder auto-restarts, sees that your plugin is broken, and responds with a number of actions that seem to include resetting the plugin's routing.

Therefore, when you fix your code and go back, your plugin is serving on "/PLUGIN_NAME" instead of where you left it.


I propose that this behavior be regarded as a bug as the less surprising behavior, imo, would be to leave the routing alone and having your plugin work again once you fix your syntax error.  Correction of such a bug would greatly improve the plugin developer experience.

Changed files:
girder/api/v1/system.py, girder/utility/server.py, tests/cases/routetable_test.py

diff --git a/girder/api/v1/system.py b/girder/api/v1/system.py
index 54c6b6f2e..245f728fa 100644
--- a/girder/api/v1/system.py
+++ b/girder/api/v1/system.py
@@ -27,7 +27,8 @@ import os
 import logging
 
 from girder.api import access
-from girder.constants import SettingKey, TokenScope, ACCESS_FLAGS, VERSION
+from girder.constants import GIRDER_ROUTE_ID, GIRDER_STATIC_ROUTE_ID, \\
+    SettingKey, TokenScope, ACCESS_FLAGS, VERSION
 from girder.models.model_base import GirderException
 from girder.utility import install, plugin_utilities, system
 from girder.utility.progress import ProgressContext
@@ -156,7 +157,26 @@ class System(Resource):
         .errorResponse('You are not a system administrator.', 403)
     )
     def enablePlugins(self, plugins, params):
-        return self.model('setting').set(SettingKey.PLUGINS_ENABLED, plugins)
+        # Determine what plugins have been disabled and remove their associated routes.
+        setting = self.model('setting')
+        routeTable = setting.get(SettingKey.ROUTE_TABLE)
+        oldPlugins = setting.get(SettingKey.PLUGINS_ENABLED)
+        reservedRoutes = {GIRDER_ROUTE_ID, GIRDER_STATIC_ROUTE_ID}
+
+        routeTableChanged = False
+        removedRoutes = (
+            set(oldPlugins) - set(plugins) - reservedRoutes)
+
+        for route in removedRoutes:
+            if route in routeTable:
+                del routeTable[route]
+                routeTableChanged = True
+
+        if routeTableChanged:
+            setting.set(SettingKey.ROUTE_TABLE, routeTable)
+
+        # Route cleanup is done; update list of enabled plugins.
+        return setting.set(SettingKey.PLUGINS_ENABLED, plugins)
 
     @access.admin
     @autoDescribeRoute(
diff --git a/girder/utility/server.py b/girder/utility/server.py
index 06e519382..135f5df58 100644
--- a/girder/utility/server.py
+++ b/girder/utility/server.py
@@ -156,11 +156,8 @@ def loadRouteTable(reconcileRoutes=False):
     Retrieves the route table from Girder and reconciles the state of it with the current
     application state.
 
-    Reconciliation deals with 2 scenarios:
-
-    1. A plugin is no longer active (by being disabled or removed) and the route for the
-       plugin needs to be removed.
-    2. A webroot was added (a new plugin was enabled) and a default route needs to be added.
+    Reconciliation ensures that every enabled plugin has a route by assigning default routes for
+    plugins that have none, such as newly-enabled plugins.
 
     :returns: The non empty routes (as a dict of name -> route) to be mounted by CherryPy
               during Girder's setup phase.
@@ -168,17 +165,10 @@ def loadRouteTable(reconcileRoutes=False):
     pluginWebroots = plugin_utilities.getPluginWebroots()
     setting = model_importer.ModelImporter().model('setting')
     routeTable = setting.get(constants.SettingKey.ROUTE_TABLE)
-    reservedRoutes = (constants.GIRDER_ROUTE_ID, constants.GIRDER_STATIC_ROUTE_ID)
 
     def reconcileRouteTable(routeTable):
         hasChanged = False
 
-        # GIRDER_ROUTE_ID is a special route, which can't be removed
-        for name in routeTable.keys():
-            if name not in reservedRoutes and name not in pluginWebroots:
-                del routeTable[name]
-                hasChanged = True
-
         for name in pluginWebroots.keys():
             if name not in routeTable:
                 routeTable[name] = os.path.join('/', name)
diff --git a/tests/cases/routetable_test.py b/tests/cases/routetable_test.py
index 86d79ee25..8b7ac72b6 100644
--- a/tests/cases/routetable_test.py
+++ b/tests/cases/routetable_test.py
@@ -97,6 +97,40 @@ class RouteTableTestCase(base.TestCase):
         self.assertStatusOk(resp)
         self.assertTrue('g-global-info-apiroot' in self.getBody(resp))
 
+        # has_webroot is mounted on /has_weboort
+        resp = self.request('/system/setting', params={
+            'key': SettingKey.ROUTE_TABLE
+        }, user=self.admin)
+        self.assertStatusOk(resp)
+        self.assertTrue('has_webroot' in resp.json)
+        self.assertEqual(resp.json['has_webroot'], '/has_webroot')
+
+        # has_webroot is set to be mounted on /has_webroot even after removing it from the list of
+        # enabled plugins.
+        base.enabledPlugins.remove('has_webroot')
+        resp = self.request('/system/setting', params={
+            'key': SettingKey.ROUTE_TABLE
+        }, user=self.admin)
+        self.assertStatusOk(resp)
+        self.assertTrue('has_webroot' in resp.json)
+        self.assertEqual(resp.json['has_webroot'], '/has_webroot')
+        base.enabledPlugins.append('has_webroot')
+
+        # Only when has_webroot has been explicitly removed by the user is its route table entry
+        # cleared.
+        resp = self.request('/system/plugins', params={
+            'plugins': json.dumps([
+                plugin for plugin in base.enabledPlugins if plugin != 'has_webroot'])
+        }, method='PUT', user=self.admin)
+        self.assertStatusOk(resp)
+
+        # now, confirm that the plugin's route table entry has actually been removed
+        resp = self.request('/system/setting', params={
+            'key': SettingKey.ROUTE_TABLE
+        }, user=self.admin)
+        self.assertStatusOk(resp)
+        self.assertTrue('has_webroot' not in resp.json)
+
         # Setting the static route to http should be allowed
         resp = self.request('/system/setting', params={
             'key': SettingKey.ROUTE_TABLE,