[18.0][IMP] endpoint_route_handler: detect route conflicts

endpoint/tests/test_endpoint.py

@@ -2,6 +2,7 @@
 # @author: Simone Orsi <simone.orsi@camptocamp.com>
 # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl).
 
+import importlib.util
 import json
 import textwrap
 from datetime import date
@@ -12,6 +13,7 @@
 
 from odoo import exceptions
 from odoo.http import Response
+from odoo.tools import file_path
 from odoo.tools.misc import mute_logger
 
 from odoo.addons.endpoint.controllers.main import EndpointController
@@ -26,8 +28,55 @@ def _setup_records(cls):
         cls.endpoint = cls.env.ref("endpoint.endpoint_demo_1")
         return res
 
+    def test_endpoint_route_consumer_models(self):
+        """Indirect inheritors are consumers (via `endpoint.mixin`), mixins are not."""
+        models = self.env[
+            "endpoint.route.handler"
+        ]._get_endpoint_route_consumer_models()
+        self.assertIn("endpoint.endpoint", models)
+        self.assertNotIn("endpoint.mixin", models)
+
+    # Migration test: can be trashed when migrating to the next version
+    def test_migration_register_again(self):
+        """Routes in sync get the new hash, the others keep the old one."""
+        in_sync = self.endpoint
+        out_of_sync = self.env["endpoint.endpoint"].create(
+            {
+                "name": "Out of sync",
+                "route": "/demo/out_of_sync",
+                "request_method": "GET",
+                "exec_mode": "code",
+                "code_snippet": 'result = {"response": Response("ok")}',
+            }
+        )
+        (in_sync + out_of_sync)._register_controllers()
+        in_sync.registry_sync = True
+        keys = (
+            in_sync._endpoint_registry_unique_key(),
+            out_of_sync._endpoint_registry_unique_key(),
+        )
+        # Simulate hashes computed w/ the old randomized `hash()`
+        self.env.cr.execute(
+            "UPDATE endpoint_route SET endpoint_hash = 'old-' || id WHERE key IN %s",
+            (keys,),
+        )
+        migration = self._load_migration("18.0.1.3.0", "end-migrate.py")
+        migration.migrate(self.env.cr, "18.0.1.2.0")
+        reg = in_sync._endpoint_registry
+        self.assertEqual(reg._get_rule(keys[0]).endpoint_hash, in_sync.endpoint_hash)
+        self.assertTrue(reg._get_rule(keys[1]).endpoint_hash.startswith("old-"))
+
+    def _load_migration(self, version, filename):
+        """Import a migration script of `endpoint_route_handler` (not a package)."""
+        path = file_path(f"endpoint_route_handler/migrations/{version}/{filename}")
+        spec = importlib.util.spec_from_file_location("erh_migration", path)
+        module = importlib.util.module_from_spec(spec)
+        spec.loader.exec_module(module)
+        return module
+
     @mute_logger("odoo.sql_db")
     def test_endpoint_unique(self):
+        """Two endpoints can't share the same route."""
         with self.assertRaises(psycopg2.IntegrityError):
             self.env["endpoint.endpoint"].create(
                 {
@@ -38,6 +87,7 @@ def test_endpoint_unique(self):
             )
 
     def test_endpoint_validation(self):
+        """Invalid endpoints are refused: no code, or POST/PUT w/o content type."""
         with self.assertRaisesRegex(
             exceptions.UserError, r"you must provide a piece of code"
         ):
@@ -69,18 +119,21 @@ def test_endpoint_validation(self):
             self.endpoint.request_method = "POST"
 
     def test_endpoint_find(self):
+        """An endpoint is found by its route."""
         self.assertEqual(
             self.env["endpoint.endpoint"]._find_endpoint("/demo/one"), self.endpoint
         )
 
     def test_endpoint_code_eval_full_response(self):
+        """Code can return a full response: 200 OK w/ the body."""
         with self._get_mocked_request() as req:
             result = self.endpoint._handle_request(req)
         resp = result["response"]
         self.assertEqual(resp.status, "200 OK")
         self.assertEqual(resp.data, b"ok")
 
     def test_endpoint_code_eval_free_vals(self):
+        """Code can set the payload and headers of the response."""
         self.endpoint.write(
             {
                 "code_snippet": textwrap.dedent(
@@ -99,6 +152,7 @@ def test_endpoint_code_eval_free_vals(self):
         self.assertEqual(json.loads(payload), {"a": 1, "b": 2})
 
     def test_endpoint_log(self):
+        """Logging from the code doesn't break the request."""
         self.endpoint.write(
             {
                 "code_snippet": textwrap.dedent(
@@ -118,6 +172,7 @@ def test_endpoint_log(self):
 
     @mute_logger("endpoint.endpoint", "odoo.modules.registry")
     def test_endpoint_validate_request(self):
+        """Requests w/ the wrong content type or method are rejected (415, 405)."""
         endpoint = self.endpoint.copy(
             {
                 "route": "/wrong",
@@ -137,6 +192,7 @@ def test_endpoint_validate_request(self):
 
     @mute_logger("odoo.modules.registry")
     def test_routing(self):
+        """Routing info follows the fields of the endpoint, prefix included."""
         route, info, __ = self.endpoint._get_routing_info()
         self.assertEqual(route, "/demo/one")
         self.assertEqual(
@@ -189,6 +245,7 @@ def test_routing(self):
 
     @mute_logger("odoo.modules.registry")
     def test_unlink(self):
+        """Deleting an endpoint removes its routing rule."""
         endpoint = self.endpoint.copy(
             {
                 "route": "/delete/this",
@@ -207,6 +264,7 @@ def test_unlink(self):
 
     @mute_logger("odoo.modules.registry")
     def test_archive(self):
+        """Archiving an endpoint removes its routing rule."""
         endpoint = self.endpoint.copy(
             {
                 "route": "/enable-disable/this",
@@ -226,6 +284,7 @@ def test_archive(self):
         self.assertEqual(reg._get_rule(key), None)
 
     def test_registry_sync(self):
+        """A new endpoint is registered only once `registry_sync` is set."""
         endpoint = self.env["endpoint.endpoint"].create(
             {
                 "name": "New",
@@ -249,6 +308,7 @@ def test_registry_sync(self):
             )
 
     def test_duplicate(self):
+        """Copying an endpoint changes its route: no clash w/ the original."""
         endpoint = self.endpoint.copy()
         self.assertTrue(endpoint.route.endswith("/COPY_FIXME"))
 

endpoint_route_handler/__manifest__.py

@@ -4,7 +4,7 @@
 {
     "name": "Endpoint route handler",
     "summary": """Provide mixin and tool to generate custom endpoints on the fly.""",
-    "version": "18.0.1.2.1",
+    "version": "18.0.1.3.0",
     "license": "LGPL-3",
     "development_status": "Beta",
     "author": "Camptocamp,Odoo Community Association (OCA)",

endpoint_route_handler/migrations/18.0.1.3.0/end-migrate.py

@@ -0,0 +1,32 @@
+# Copyright 2026 Camptocamp SA
+# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl).
+
+import logging
+
+from odoo import SUPERUSER_ID, api
+
+_logger = logging.getLogger(__name__)
+
+
+def migrate(cr, version):
+    """Register again all routes in sync: their hash is now stable across processes.
+
+    Rows are dropped and created again to enforce the new hash.
+    Run at the end of the update: consumer models (eg: `endpoint.endpoint`)
+    belong to modules loaded after this one.
+    """
+    if not version:
+        return
+    env = api.Environment(cr, SUPERUSER_ID, {})
+    # No cache: the registry might have been partially loaded when cached
+    model_names = env["endpoint.route.handler"]._find_endpoint_route_consumer_models()
+    for model_name in model_names:
+        records = (
+            env[model_name]
+            .with_context(active_test=False)
+            .search([("registry_sync", "=", True)])
+        )
+        records._unregister_controllers(clear_cache=False)
+        records.filtered("active")._register_controllers(clear_cache=False)
+        _logger.info("%s: registered %s routes again", model_name, len(records))
+    env.registry.clear_cache("routing")

endpoint_route_handler/models/endpoint_route_handler.py

@@ -6,6 +6,8 @@
 
 from odoo import _, api, exceptions, fields, models
 
+from ..utils import stable_hash
+
 ENDPOINT_ROUTE_CONSUMER_MODELS = {
     # by db
 }
@@ -89,21 +91,32 @@ def _check_route_unique_across_models(self):
             )
 
     def _get_endpoint_route_consumer_models(self):
+        """Return the concrete models using `endpoint.route.handler`.
+
+        Indirect inheritance included (eg: `endpoint.endpoint` via `endpoint.mixin`).
+        """
         global ENDPOINT_ROUTE_CONSUMER_MODELS
         if ENDPOINT_ROUTE_CONSUMER_MODELS.get(self.env.cr.dbname):
             return ENDPOINT_ROUTE_CONSUMER_MODELS.get(self.env.cr.dbname)
-        models = []
-        route_model = "endpoint.route.handler"
-        for model in self.env.values():
-            if (
-                model._name != route_model
-                and not model._abstract
-                and route_model in model._inherit
-            ):
-                models.append(model._name)
+        models = self._find_endpoint_route_consumer_models()
         ENDPOINT_ROUTE_CONSUMER_MODELS[self.env.cr.dbname] = models
         return models
 
+    def _find_endpoint_route_consumer_models(self):
+        """Look up the persistent models using `endpoint.route.handler` (no cache).
+
+        Transient models are tools (eg: `endpoint.route.handler.tool`): their
+        records are vacuumed by Odoo, so their routes must not be synced.
+        """
+        route_model = "endpoint.route.handler"
+        return sorted(
+            name
+            for name in self.env.registry.descendants([route_model], "_inherit")
+            if name != route_model
+            and not self.env[name]._abstract
+            and not self.env[name]._transient
+        )
+
     @property
     def _logger(self):
         return logging.getLogger(self._name)
@@ -144,7 +157,7 @@ def _compute_endpoint_hash(self):
         ]
         for rec, vals in zip(self, values, strict=True):
             vals.pop("id", None)
-            rec.endpoint_hash = hash(tuple(vals.values()))
+            rec.endpoint_hash = stable_hash(vals)
 
     def _routing_impacting_fields(self):
         return ("route", "auth_type", "request_method")

endpoint_route_handler/models/ir_http.py

@@ -3,13 +3,13 @@
 # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl).
 
 import logging
-from itertools import chain
 
 import werkzeug
 
 from odoo import http, models, tools
 
 from ..registry import EndpointRegistry
+from ..utils import RouteConflictTracker, controller_owner, rule_owner
 
 _logger = logging.getLogger(__name__)
 
@@ -21,19 +21,44 @@ def _endpoint_route_registry(self, cr=None):
         return EndpointRegistry.registry_for(cr or self.env.cr)
 
     def _generate_routing_rules(self, modules, converters):
-        # Override to inject custom endpoint rules.
-        return chain(
-            super()._generate_routing_rules(modules, converters),
-            self._endpoint_routing_rules(),
-        )
+        """Inject custom endpoint rules, skipping the conflicting ones.
+
+        Standard controllers come first and are only tracked.
+        Each custom rule is checked against everything tracked so far
+        (see `RouteConflictTracker`): the 1st rule wins, as in werkzeug,
+        conflicting ones are logged and skipped.
+
+        Cost: one dict lookup per rule (index by URL), negligible compared
+        to the build of the routing map (~3 ms for ~800 rules).
+        """
+        tracker = RouteConflictTracker()
+        for url, endpoint in super()._generate_routing_rules(modules, converters):
+            # Standard controllers are not checked against each other
+            tracker.add(url, endpoint, controller_owner(endpoint))
+            yield url, endpoint
+        for endpoint_rule, url, endpoint in self._endpoint_routing_rules():
+            owner = rule_owner(endpoint_rule)
+            conflict = tracker.conflict(url, endpoint)
+            if conflict:
+                _logger.error(
+                    "Route %s %s of %s conflicts w/ %s: skipped",
+                    url,
+                    tracker.methods_label(endpoint),
+                    owner,
+                    conflict,
+                )
+                continue
+            tracker.add(url, endpoint, owner)
+            yield url, endpoint
 
     def _endpoint_routing_rules(self):
-        """Yield custom endpoint rules"""
+        """Yield `(endpoint rule, url, endpoint)` for all the custom endpoint rules."""
         e_registry = self._endpoint_route_registry()
         for endpoint_rule in e_registry.get_rules():
             _logger.debug("LOADING %s", endpoint_rule)
             if not endpoint_rule.is_generator:
-                yield from endpoint_rule.iter_routing_rules(self.env)
+                for url, endpoint in endpoint_rule.iter_routing_rules(self.env):
+                    yield endpoint_rule, url, endpoint
                 continue
             # A broken generator must not break the whole routing map.
             # Collect its rules first, so that nothing is yielded on failure.
@@ -42,7 +67,8 @@ def _endpoint_routing_rules(self):
             except Exception:
                 _logger.exception("Routes generation failed for %s", endpoint_rule)
                 continue
-            yield from rules
+            for url, endpoint in rules:
+                yield endpoint_rule, url, endpoint
 
     @tools.ormcache("key", "cls._endpoint_route_last_version()", cache="routing")
     def routing_map(cls, key=None):

endpoint_route_handler/readme/USAGE.md

@@ -108,3 +108,12 @@ the rest of the routing map is not affected.
 The routes of the rule are refreshed when the rule changes
 (the routing map version is bumped)
 or when the code changes (as for any controller: restart the server).
+
+## Route conflicts
+
+When the routing map is built, each custom route (stored or generated)
+is checked against the routes already loaded (standard controllers
+and other custom routes). A route w/ the same URL and overlapping HTTP methods
+(no methods means any method) is skipped and an error is logged w/ both owners:
+the first route wins, as werkzeug would do, but the conflict is not silent anymore.
+The same URL w/ different methods (eg: GET and POST) is fine.