Repository: sanic-org/sanic
Pull request: Allow blueprints and groups to be infinitely reusable
The main purpose of this PR is to allow blueprints and blueprint groups to be more composable and reusable when applied to multiple groups. See issue #2113 for further discussion.

Resolves #2113 

---

This PR does _not_ really introduce anything new. The difference is only in the time at which we are applying arguments to the Blueprints. Previously, when you did: `Blueprint.group()`, it would immediately update the `url_prefix`, `version`, `strict_slashes`, and (the newly added) `version_prefix`.

However, doing this would cause multiple applications of the Blueprint to stack up `url_prefix` and the other properties to behave in unexpected patterns.

By moving the application of these variables until the time of registry (and to apply them recursively for nested groups), we allow for Blueprints to exists **without modification**. Because they are no longer being modified, you can easily reuse them.

Problem statement:
Reuse of blueprint groups causes duplicate route errors
**Describe the bug**

When adding a blueprint to multiple groups, the router seems to club `url_prefix`es together in an unexpected way, resulting in duplicate route errors.

```
Traceback (most recent call last):
  File "/home/jraymond/Development/sanic-org/bugreport/bugreport/app.py", line 26, in <module>
    app.blueprint(new)
  File "/home/jraymond/Development/sanic-org/sanic/sanic/app.py", line 407, in blueprint
    self.blueprint(item, **options)
  File "/home/jraymond/Development/sanic-org/sanic/sanic/app.py", line 423, in blueprint
    blueprint.register(self, options)
  File "/home/jraymond/Development/sanic-org/sanic/sanic/blueprints.py", line 227, in register
    route = app._apply_route(apply_route)
  File "/home/jraymond/Development/sanic-org/sanic/sanic/app.py", line 337, in _apply_route
    routes = self.router.add(**params)
  File "/home/jraymond/Development/sanic-org/sanic/sanic/router.py", line 128, in add
    route = super().add(**params)  # type: ignore
  File "/home/jraymond/.local/lib/python3.9/site-packages/sanic_routing/router.py", line 170, in add
    route.add_handler(path, handler, method, requirements, overwrite)
  File "/home/jraymond/.local/lib/python3.9/site-packages/sanic_routing/route.py", line 114, in add_handler
    raise RouteExists(
sanic_routing.exceptions.RouteExists: Route already registered: api/new/v1/api/v1/hello [GET]
```

**Code snippet**
Minimal reproduction:
```python
from sanic import Blueprint, Sanic
from sanic.response import text


bp1 = Blueprint("bp1", url_prefix="/hello")


@bp1.get("/")
async def hello(_):
    return text("hello")


bp2 = Blueprint("bp2", url_prefix="/goodbye")


@bp2.get("/")
async def goodbye(_):
    return text("goodbye")


legacy = Blueprint.group([bp1, bp2], url_prefix="/api/v1")
new = Blueprint.group([bp1, bp2], url_prefix="/api/new/v1")

app = Sanic("bugreport")
app.blueprint(legacy)
app.blueprint(new)

if __name__ == "__main__":
    app.run(host="0.0.0.0", port=1234, debug=True)
```

**Expected behavior**
Blueprint groups should be independent references to handlers:
```
GET: /api/thing/v1/hello => "hello"
GET: /api/v1/hello => "hello"
```

**Environment (please complete the following information):**
 - OS: Linux (Arch)
 - Python: 3.9
 - Version 21.3.2

Hints from issue discussion:
Problem appears [to be here](https://github.com/sanic-org/sanic/blob/e21521f45c0b58bac619a9111fd47426e208bf08/sanic/blueprint_group.py#L176). Not sure if there's an obvious fix, but the API for blueprint groups doesn't indicate that the op is destructive. Can try to come up with sth if nobody beats me to the punch
Thanks @jdraymon for the great bug report.
The 21.3 release included a new router and backend server, along with those changes came some some necessary modifications to the way url_prefixes work. Can you confirm that this code works correctly on Sanic v20.12?

I guess there is simply no test in the Sanic test suite that tests adding a blueprint to multiple bp groups.
@ashleysommer Running with version 20.12.3 produces a similar stack trace:
```
Traceback (most recent call last):
  File "/home/jraymond/Development/sanic-org/bugreport/bugreport/app.py", line 26, in <module>
    app.blueprint(new)
  File "/home/jraymond/.cache/pypoetry/virtualenvs/bugreport-qBp1ZuVR-py3.9/lib/python3.9/site-packages/sanic/app.py", line 707, in blueprint
    self.blueprint(item, **options)
  File "/home/jraymond/.cache/pypoetry/virtualenvs/bugreport-qBp1ZuVR-py3.9/lib/python3.9/site-packages/sanic/app.py", line 717, in blueprint
    blueprint.register(self, options)
  File "/home/jraymond/.cache/pypoetry/virtualenvs/bugreport-qBp1ZuVR-py3.9/lib/python3.9/site-packages/sanic/blueprints.py", line 119, in register
    _routes, _ = app.route(
  File "/home/jraymond/.cache/pypoetry/virtualenvs/bugreport-qBp1ZuVR-py3.9/lib/python3.9/site-packages/sanic/app.py", line 211, in response
    self.router.add(
  File "/home/jraymond/.cache/pypoetry/virtualenvs/bugreport-qBp1ZuVR-py3.9/lib/python3.9/site-packages/sanic/router.py", line 158, in add
    routes.append(self._add(uri, methods, handler, host, name))
  File "/home/jraymond/.cache/pypoetry/virtualenvs/bugreport-qBp1ZuVR-py3.9/lib/python3.9/site-packages/sanic/router.py", line 317, in _add
    route = merge_route(route, methods, handler)
  File "/home/jraymond/.cache/pypoetry/virtualenvs/bugreport-qBp1ZuVR-py3.9/lib/python3.9/site-packages/sanic/router.py", line 267, in merge_route
    raise RouteExists(
sanic.router.RouteExists: Route already registered: /api/new/v1/api/v1/hello/ [GET]
```
Walked the tree back a goodly long ways and it seems this has always been the behavior, looks like I'm just the first one to try and do this :slightly_smiling_face: 
@jdraymon This is a good report, and I can confirm that this is a problem with the way in which we are applying the URLs. TBH, to solve this properly will require a bit of reworking of the groups. I do agree with @sjsadowski applying the `LTS` label here that we should fix this and then apply it backwards.

I propose that the `BlueprintGroup.append` solution instead of making the change to the blueprint, registers it either on the group instance with a reference (less appealing), or on some property on the blueprint (`blueprint.alternatives`). Then, when the blueprint is added to the router, we simply register the handlers once for each alternative definition that was supplied.

The other problem that the current methodology has (not reported in this issue) is that having blueprints with differing `version` or `strict_slashes` would result in weird and unexpected behaviors as they would override one another.
See PR #2150 

Here is the original snippet, albeit slightly modified to add in another layer of grouping and to test version args.

```python
from sanic import Blueprint, Sanic
from sanic.response import text

bp1 = Blueprint("bp1", url_prefix="/hello")


@bp1.get("/")
async def hello(_):
    return text("hello")


bp2 = Blueprint("bp2", url_prefix="/goodbye")


@bp2.get("/")
async def goodbye(_):
    return text("goodbye")


legacy = Blueprint.group([bp1, bp2], url_prefix="/api", version=1)
new = Blueprint.group([bp1, bp2], url_prefix="/new", version=1)
v2 = Blueprint.group(legacy, new, url_prefix="/alt", version=2)

app = Sanic("bugreport")
app.blueprint(legacy)
app.blueprint(new)
app.blueprint(v2)


@app.before_server_start
def display(app, _):
    for route in app.router.routes:
        print(route.path)
```

When we start up, the routes we get are:

```
v1/api/hello
v1/api/goodbye
v1/new/hello
v1/new/goodbye
v1/alt/api/hello
v1/alt/api/goodbye
v1/alt/new/hello
v1/alt/new/goodbye
```

---

Thinking back on this more, I am not sure that I actually agree that this is a bug that we need to backport to 20.12. Really, this is a new feature. The old documentation did not claim that blueprints were infinitely composable. So, while I think this is a novel use, and a really great addition that can make for some nice composable APIs, I am not convinced we need another PR to 20.12.

---

@jdraymon 

As a sidenote, there will now be a better way to handle this:

```python
# FROM THIS
legacy = Blueprint.group([bp1, bp2], url_prefix="/api/v1")
new = Blueprint.group([bp1, bp2], url_prefix="/api/new/v1")

# TO THIS
legacy = Blueprint.group([bp1, bp2], version_prefix="/api/v", version=1)
new = Blueprint.group([bp1, bp2], version_prefix="/api/new/v", version=1)
```

See #2137

Commit message: Merge branch 'main' into reusable-blueprints

Patch to review:
diff --git a/sanic/app.py b/sanic/app.py
index 002c92dcf9..ec9027b512 100644
--- a/sanic/app.py
+++ b/sanic/app.py
@@ -420,7 +420,33 @@ def blueprint(self, blueprint, **options):
         """
         if isinstance(blueprint, (list, tuple, BlueprintGroup)):
             for item in blueprint:
-                self.blueprint(item, **options)
+                params = {**options}
+                if isinstance(blueprint, BlueprintGroup):
+                    if blueprint.url_prefix:
+                        merge_from = [
+                            options.get("url_prefix", ""),
+                            blueprint.url_prefix,
+                        ]
+                        if not isinstance(item, BlueprintGroup):
+                            merge_from.append(item.url_prefix or "")
+                        merged_prefix = "/".join(
+                            u.strip("/") for u in merge_from
+                        ).rstrip("/")
+                        params["url_prefix"] = f"/{merged_prefix}"
+
+                    for _attr in ["version", "strict_slashes"]:
+                        if getattr(item, _attr) is None:
+                            params[_attr] = getattr(
+                                blueprint, _attr
+                            ) or options.get(_attr)
+                    if item.version_prefix == "/v":
+                        if blueprint.version_prefix == "/v":
+                            params["version_prefix"] = options.get(
+                                "version_prefix"
+                            )
+                        else:
+                            params["version_prefix"] = blueprint.version_prefix
+                self.blueprint(item, **params)
             return
         if blueprint.name in self.blueprints:
             assert self.blueprints[blueprint.name] is blueprint, (
diff --git a/sanic/blueprint_group.py b/sanic/blueprint_group.py
index 7dd729a9e7..92b91ccc68 100644
--- a/sanic/blueprint_group.py
+++ b/sanic/blueprint_group.py
@@ -187,27 +187,6 @@ def __len__(self) -> int:
         """
         return len(self._blueprints)
 
-    def _sanitize_blueprint(self, bp: "sanic.Blueprint") -> "sanic.Blueprint":
-        """
-        Sanitize the Blueprint Entity to override the Version and strict slash
-        behaviors as required.
-
-        :param bp: Sanic Blueprint entity Object
-        :return: Modified Blueprint
-        """
-        if self._url_prefix:
-            merged_prefix = "/".join(
-                u.strip("/") for u in [self._url_prefix, bp.url_prefix or ""]
-            ).rstrip("/")
-            bp.url_prefix = f"/{merged_prefix}"
-        for _attr in ["version", "strict_slashes"]:
-            if getattr(bp, _attr) is None:
-                setattr(bp, _attr, getattr(self, _attr))
-        if bp.version_prefix == "/v":
-            bp.version_prefix = self._version_prefix
-
-        return bp
-
     def append(self, value: "sanic.Blueprint") -> None:
         """
         The Abstract class `MutableSequence` leverages this append method to
@@ -215,7 +194,7 @@ def append(self, value: "sanic.Blueprint") -> None:
         :param value: New `Blueprint` object.
         :return: None
         """
-        self._blueprints.append(self._sanitize_blueprint(bp=value))
+        self._blueprints.append(value)
 
     def insert(self, index: int, item: "sanic.Blueprint") -> None:
         """
@@ -226,7 +205,7 @@ def insert(self, index: int, item: "sanic.Blueprint") -> None:
         :param item: New `Blueprint` object.
         :return: None
         """
-        self._blueprints.insert(index, self._sanitize_blueprint(item))
+        self._blueprints.insert(index, item)
 
     def middleware(self, *args, **kwargs):
         """
diff --git a/sanic/blueprints.py b/sanic/blueprints.py
index eba91c39b7..1fa24e889d 100644
--- a/sanic/blueprints.py
+++ b/sanic/blueprints.py
@@ -168,8 +168,6 @@ def chain(nested) -> Iterable[Blueprint]:
             for i in nested:
                 if isinstance(i, (list, tuple)):
                     yield from chain(i)
-                elif isinstance(i, BlueprintGroup):
-                    yield from i.blueprints
                 else:
                     yield i
 
@@ -196,6 +194,7 @@ def register(self, app, options):
         self._apps.add(app)
         url_prefix = options.get("url_prefix", self.url_prefix)
         opt_version = options.get("version", None)
+        opt_strict_slashes = options.get("strict_slashes", None)
         opt_version_prefix = options.get("version_prefix", self.version_prefix)
 
         routes = []
@@ -220,18 +219,13 @@ def register(self, app, options):
                     version_prefix = prefix
                     break
 
-            version = self.version
-            for v in (future.version, opt_version, self.version):
-                if v is not None:
-                    version = v
-                    break
-
-            strict_slashes = (
-                self.strict_slashes
-                if future.strict_slashes is None
-                and self.strict_slashes is not None
-                else future.strict_slashes
+            version = self._extract_value(
+                future.version, opt_version, self.version
+            )
+            strict_slashes = self._extract_value(
+                future.strict_slashes, opt_strict_slashes, self.strict_slashes
             )
+
             name = app._generate_name(future.name)
 
             apply_route = FutureRoute(
@@ -315,3 +309,12 @@ def event(self, event: str, timeout: Optional[Union[int, float]] = None):
             return_when=asyncio.FIRST_COMPLETED,
             timeout=timeout,
         )
+
+    @staticmethod
+    def _extract_value(*values):
+        value = values[-1]
+        for v in values:
+            if v is not None:
+                value = v
+                break
+        return value
diff --git a/tests/test_blueprint_group.py b/tests/test_blueprint_group.py
index 893cc80551..77ddf44c5a 100644
--- a/tests/test_blueprint_group.py
+++ b/tests/test_blueprint_group.py
@@ -200,7 +200,7 @@ def test_bp_group_as_nested_group():
     blueprint_group_1 = Blueprint.group(
         Blueprint.group(blueprint_1, blueprint_2)
     )
-    assert len(blueprint_group_1) == 2
+    assert len(blueprint_group_1) == 1
 
 
 def test_blueprint_group_insert():
@@ -215,9 +215,29 @@ def test_blueprint_group_insert():
     group.insert(0, blueprint_1)
     group.insert(0, blueprint_2)
     group.insert(0, blueprint_3)
-    assert group.blueprints[1].strict_slashes is False
-    assert group.blueprints[2].strict_slashes is True
-    assert group.blueprints[0].url_prefix == "/test"
+
+    @blueprint_1.route("/")
+    def blueprint_1_default_route(request):
+        return text("BP1_OK")
+
+    @blueprint_2.route("/")
+    def blueprint_2_default_route(request):
+        return text("BP2_OK")
+
+    @blueprint_3.route("/")
+    def blueprint_3_default_route(request):
+        return text("BP3_OK")
+
+    app = Sanic("PropTest")
+    app.blueprint(group)
+    app.router.finalize()
+
+    routes = [(route.path, route.strict) for route in app.router.routes]
+
+    assert len(routes) == 3
+    assert ("v1/test/bp1/", True) in routes
+    assert ("v1.3/test/bp2", False) in routes
+    assert ("v1.3/test", False) in routes
 
 
 def test_bp_group_properties():
@@ -231,19 +251,25 @@ def test_bp_group_properties():
         url_prefix="/grouped",
         strict_slashes=True,
     )
+    primary = Blueprint.group(group, url_prefix="/primary")
 
-    assert group.version_prefix == "/api/v"
-    assert blueprint_1.version_prefix == "/api/v"
-    assert blueprint_2.version_prefix == "/api/v"
+    @blueprint_1.route("/")
+    def blueprint_1_default_route(request):
+        return text("BP1_OK")
+
+    @blueprint_2.route("/")
+    def blueprint_2_default_route(request):
+        return text("BP2_OK")
 
-    assert group.version == 1
-    assert blueprint_1.version == 1
-    assert blueprint_2.version == 1
+    app = Sanic("PropTest")
+    app.blueprint(group)
+    app.blueprint(primary)
+    app.router.finalize()
 
-    assert group.strict_slashes
-    assert blueprint_1.strict_slashes
-    assert blueprint_2.strict_slashes
+    routes = [route.path for route in app.router.routes]
 
-    assert group.url_prefix == "/grouped"
-    assert blueprint_1.url_prefix == "/grouped/bp1"
-    assert blueprint_2.url_prefix == "/grouped/bp2"
+    assert len(routes) == 4
+    assert "api/v1/grouped/bp1/" in routes
+    assert "api/v1/grouped/bp2/" in routes
+    assert "api/v1/primary/grouped/bp1" in routes
+    assert "api/v1/primary/grouped/bp2" in routes


Code review comments:
diff_hunk: @@ -187,35 +187,14 @@ def __len__(self) -> int:
         """
         return len(self._blueprints)
 
-    def _sanitize_blueprint(self, bp: "sanic.Blueprint") -> "sanic.Blueprint":
-        """
-        Sanitize the Blueprint Entity to override the Version and strict slash
-        behaviors as required.
-
-        :param bp: Sanic Blueprint entity Object
-        :return: Modified Blueprint
-        """
-        if self._url_prefix:
-            merged_prefix = "/".join(
-                u.strip("/") for u in [self._url_prefix, bp.url_prefix or ""]
-            ).rstrip("/")
-            bp.url_prefix = f"/{merged_prefix}"
-        for _attr in ["version", "strict_slashes"]:
-            if getattr(bp, _attr) is None:
-                setattr(bp, _attr, getattr(self, _attr))
-        if bp.version_prefix == "/v":
-            bp.version_prefix = self._version_prefix
-
-        return bp
-
     def append(self, value: "sanic.Blueprint") -> None:
path: sanic/blueprint_group.py
text: @user1:
Can't we use [PEP 563](https://www.python.org/dev/peps/pep-0563/#enabling-the-future-behavior-in-python-3-7) style annotations, since Sanic is Python 3.7+?