Static/Version analysis of mixins (#430)

In service of fixing the broken Telegram mixin:

- Packages must declare the mixins they use statically in
`USED_MIXIN_CLASSES`. This allows static analysis of the mixin routes,
allowing knowledge of the mixin routes at the version-level dir
- New test to validate that the `telegram_respond` route can be called
publicly.
- Dependencies now installed server-side for integration tests (this
matches the behavior of regular deploys, and fixes a version issue with
tiktoken/regex)

.gitignore

@@ -106,4 +106,5 @@ fabric.properties
 .coverage/
 
 # Generated website
-docs/_build
+docs/_build
+venv

src/steamship/invocable/invocable.py

@@ -6,9 +6,10 @@
 from abc import ABC
 from functools import wraps
 from http import HTTPStatus
-from typing import Any, Dict, Optional, Type, Union
+from typing import Any, Dict, List, Optional, Type, Union
 
 import toml
+from pydantic import BaseModel
 
 from steamship import SteamshipError
 from steamship.base.package_spec import MethodSpec, PackageSpec
@@ -85,6 +86,31 @@ def post(path: str, **kwargs):
     return endpoint(verb=Verb.POST, path=path, **kwargs)
 
 
+class RouteMethod(BaseModel):
+    attribute: Any
+    verb: Optional[Verb]
+    path: str
+    config: Dict[str, Any]
+
+
+def find_route_methods(on_class: Type) -> List[RouteMethod]:
+    base_fn_list = [
+        may_be_decorated
+        for base_cls in on_class.__bases__
+        for may_be_decorated in base_cls.__dict__.values()
+    ]
+    result: List[RouteMethod] = []
+    for attribute in base_fn_list + list(on_class.__dict__.values()):
+        decorator = getattr(attribute, "decorator", None)
+        if decorator:
+            if getattr(decorator, "__is_endpoint__", False):
+                path = getattr(attribute, "__path__", None)
+                verb = getattr(attribute, "__verb__", None)
+                config = getattr(attribute, "__endpoint_config__", {})
+                result.append(RouteMethod(attribute=attribute, verb=verb, path=path, config=config))
+    return result
+
+
 class Invocable(ABC):
     """A Steamship microservice.
 
@@ -156,21 +182,13 @@ def __init_subclass__(cls, **kwargs):
         # The subclass always overrides whatever the superclass set here, having cloned its data.
         cls._package_spec = _package_spec
 
-        base_fn_list = [
-            may_be_decorated
-            for base_cls in cls.__bases__
-            for may_be_decorated in base_cls.__dict__.values()
-        ]
-        for attribute in base_fn_list + list(cls.__dict__.values()):
-            decorator = getattr(attribute, "decorator", None)
-            if decorator:
-                if getattr(decorator, "__is_endpoint__", False):
-                    path = getattr(attribute, "__path__", None)
-                    verb = getattr(attribute, "__verb__", None)
-                    config = getattr(attribute, "__endpoint_config__", {})
-                    cls._register_mapping(
-                        name=attribute.__name__, verb=verb, path=path, config=config
-                    )
+        for route_method in find_route_methods(cls):
+            cls._register_mapping(
+                name=route_method.attribute.__name__,
+                verb=route_method.verb,
+                path=route_method.path,
+                config=route_method.config,
+            )
 
         # Add the HTTP GET /__dir__ method which returns a serialization of the PackageSpec.
         # Wired up to both GET and POST for convenience (POST is Steamship default; GET is browser friendly)

src/steamship/invocable/package_service.py

@@ -2,10 +2,10 @@
 
 import logging
 from functools import partial
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, List, Optional, Type
 
 from steamship import SteamshipError, Task
-from steamship.base.package_spec import MethodSpec, RouteConflictError
+from steamship.base.package_spec import MethodSpec, PackageSpec, RouteConflictError
 from steamship.invocable import Invocable
 
 # Note!
@@ -14,6 +14,7 @@
 # This the files in this package are for Package Implementors.
 # If you are using the Steamship Client, you probably are looking for either steamship.client or steamship.data
 #
+from steamship.invocable.invocable import find_route_methods
 from steamship.invocable.package_mixin import PackageMixin
 from steamship.utils.url import Verb
 
@@ -28,45 +29,80 @@ class PackageService(Invocable):
 
     """
 
+    USED_MIXIN_CLASSES: List[Type[PackageMixin]] = []
     mixins: List[PackageMixin]
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self.mixins = []
 
+    def __init_subclass__(cls, **kwargs):
+        super().__init_subclass__(**kwargs)
+
+        # The subclass takes care to extend what the superclass may have set here by copying it.
+        _package_spec = PackageSpec(name=cls.__name__, doc=cls.__doc__, methods=[])
+        if hasattr(cls, "_package_spec"):
+            _package_spec.import_parent_methods(cls._package_spec)
+
+        # The subclass always overrides whatever the superclass set here, having cloned its data.
+        cls._package_spec = _package_spec
+
+        # Now must add routes for mixins
+        for used_mixin_class in cls.USED_MIXIN_CLASSES:
+            cls.scan_mixin(cls._package_spec, used_mixin_class)
+
+    @classmethod
+    def scan_mixin(
+        cls,
+        package_spec: PackageSpec,
+        mixin_class: Type[PackageMixin],
+        mixin_instance: Optional[PackageMixin] = None,
+        permit_overwrite_of_existing_methods: bool = False,
+    ):
+        for route_method in find_route_methods(mixin_class):
+            if mixin_instance is not None:
+                func_binding = partial(route_method.attribute, self=mixin_instance)
+            else:
+                func_binding = (
+                    route_method.attribute
+                )  # This binding is not truly valid. It must be overwritten during add_mixin
+            method_spec = MethodSpec.from_class(
+                mixin_class,
+                route_method.attribute.__name__,
+                path=route_method.path,
+                verb=route_method.verb,
+                config=route_method.config,
+                func_binding=func_binding,
+            )
+            try:
+                allow_override_for_this_route = permit_overwrite_of_existing_methods
+                existing_route = package_spec.method_mappings.get(route_method.verb, {}).get(
+                    "/" + route_method.path
+                )
+                if (
+                    existing_route is not None
+                    and existing_route.class_name == mixin_class.__name__
+                    and mixin_instance is not None
+                ):
+                    allow_override_for_this_route = True
+                package_spec.add_method(
+                    method_spec,
+                    permit_overwrite_of_existing=allow_override_for_this_route,
+                )
+            except RouteConflictError as conflict_error:
+                message = f"When attempting to add mixin {mixin_class.__name__}, route {route_method.verb} {route_method.path} conflicted with already added route {route_method.verb} {route_method.path} on class {conflict_error.existing_method_spec.class_name}"
+                raise RouteConflictError(
+                    message=message,
+                    existing_method_spec=conflict_error.existing_method_spec,
+                )
+
     def add_mixin(self, mixin: PackageMixin, permit_overwrite_of_existing_methods: bool = False):
-        base_fn_list = [
-            may_be_decorated
-            for base_cls in mixin.__class__.__bases__
-            for may_be_decorated in base_cls.__dict__.values()
-        ]
-        for attribute in base_fn_list + list(mixin.__class__.__dict__.values()):
-            decorator = getattr(attribute, "decorator", None)
-            if decorator:
-                if getattr(decorator, "__is_endpoint__", False):
-                    path = getattr(attribute, "__path__", None)
-                    verb = getattr(attribute, "__verb__", None)
-                    config = getattr(attribute, "__endpoint_config__", {})
-                    func_binding = partial(attribute, self=mixin)
-                    method_spec = MethodSpec.from_class(
-                        mixin.__class__,
-                        attribute.__name__,
-                        path=path,
-                        verb=verb,
-                        config=config,
-                        func_binding=func_binding,
-                    )
-                    try:
-                        self._package_spec.add_method(
-                            method_spec,
-                            permit_overwrite_of_existing=permit_overwrite_of_existing_methods,
-                        )
-                    except RouteConflictError as conflict_error:
-                        message = f"When attempting to add mixin {mixin.__class__.__name__}, route {verb} {path} conflicted with already added route {verb} {path} on class {conflict_error.existing_method_spec.class_name}"
-                        raise RouteConflictError(
-                            message=message,
-                            existing_method_spec=conflict_error.existing_method_spec,
-                        )
+        PackageService.scan_mixin(
+            self._package_spec,
+            mixin.__class__,
+            mixin,
+            permit_overwrite_of_existing_methods=permit_overwrite_of_existing_methods,
+        )
         self.mixins.append(mixin)
 
     def instance_init(self):

tests/assets/packages/package_with_mixins.py

@@ -13,12 +13,16 @@ def __init__(self, suffix: str):
     def instance_init(self, config: Config, context: InvocationContext):
         self.instance_init_called = True
 
-    @post("test_mixin_route")
-    def test_mixin_route(self) -> InvocableResponse:
+    @post("test_mixin_route", public=True)
+    def test_mixin_route(self, text: str) -> InvocableResponse:
+        _ = text
         return InvocableResponse(data=f"mixin {self.suffix}")
 
 
 class PackageWithMixin(PackageService):
+
+    USED_MIXIN_CLASSES = [TestMixin]
+
     def __init__(self, **kwargs):
         super(PackageWithMixin, self).__init__(**kwargs)
         self.add_mixin(

tests/assets/packages/transports/test_telegram_agent.py

@@ -55,6 +55,8 @@ class TestTelegramAgent(AgentService):
     config: TestTelegramAgentConfig
     agent: Agent
 
+    USED_MIXIN_CLASSES = [TelegramTransport, SteamshipWidgetTransport]
+
     def __init__(
         self, client: Steamship, config: Dict[str, Any] = None, context: InvocationContext = None
     ):

tests/steamship_tests/agents/test_telegram.py

@@ -1,6 +1,7 @@
 from typing import Dict
 
 import pytest
+import requests
 from assets.packages.transports.mock_telegram_package import MockTelegram
 from steamship_tests import PACKAGES_PATH
 from steamship_tests.utils.deployables import deploy_package
@@ -25,6 +26,20 @@ def test_telegram(client: Steamship):
         # the bot token to not be empty and the two appended to each other to just equal the
         # invocation url.
         instance_config = {"bot_token": "/", "api_base": mock_telegram_instance.invocation_url[:-1]}
+
+        # Should be able to invoke setWebhook publicly
+        response = requests.get(
+            url=mock_telegram_instance.invocation_url + "/setWebhook",
+            params={"url": "test", "allowed_updates": "", "drop_pending_updates": True},
+        )
+        assert response.ok
+        # Test that the call to setWebhook wrote a file
+        files = File.query(client, f'kind "{MockTelegram.WEBHOOK_TAG}"').files
+        assert len(files) == 1
+        assert files[0].tags[0].name == "test"
+
+        files[0].delete()
+
         with deploy_package(
             client,
             telegram_agent_path,
@@ -37,8 +52,12 @@ def test_telegram(client: Steamship):
             assert len(files) == 1
             assert files[0].tags[0].name == telegram_instance.invocation_url + "telegram_respond"
 
-            # test sending messages
-            telegram_instance.invoke("telegram_respond", **generate_telegram_message("a test"))
+            # test sending messages (without auth)
+            response = requests.post(
+                url=telegram_instance.invocation_url + "/telegram_respond",
+                json=generate_telegram_message("a test"),
+            )
+            assert response.ok
             files = File.query(client, f'kind "{MockTelegram.TEXT_MESSAGE_TAG}"').files
             assert len(files) == 1
             assert files[0].tags[0].name == "Response to: a test".replace(

tests/steamship_tests/app/integration/test_e2e_mixins.py

@@ -10,7 +10,7 @@ def test_mixin_and_package_invocation(client: Steamship):
     demo_package_path = PACKAGES_PATH / "package_with_mixins.py"
 
     with deploy_package(client, demo_package_path) as (_, _, instance):
-        mixin_response = instance.invoke("test_mixin_route")
+        mixin_response = instance.invoke("test_mixin_route", text="test")
         assert mixin_response == "mixin yo"
 
         package_response = instance.invoke("test_package_route")

tests/steamship_tests/app/unit/test_mixins.py

@@ -6,23 +6,28 @@
 from steamship.utils.url import Verb
 
 
-def invoke(o: Invocable, path: str):
-    req = InvocableRequest(invocation=Invocation(http_verb="POST", invocation_path=path))
+def invoke(o: Invocable, path: str, **kwargs):
+    req = InvocableRequest(
+        invocation=Invocation(http_verb="POST", invocation_path=path, arguments=kwargs)
+    )
     return o(req).data
 
 
 def test_package_with_mixin_routes():
     """Tests that we can inspect the package and mixin routes"""
-    package = PackageWithMixin()
-    assert (
-        package._package_spec.method_mappings[Verb.POST]["/test_mixin_route"].func_binding
-        is not None
-    )
+    package_class = PackageWithMixin
+    mixin_route = package_class._package_spec.method_mappings[Verb.POST]["/test_mixin_route"]
+    assert mixin_route.func_binding is not None
+    assert len(mixin_route.args) == 1
+    assert mixin_route.args[0].name == "text"
+    assert mixin_route.config.get("public", False) is True
+
     assert (
-        package._package_spec.method_mappings[Verb.POST]["/test_package_route"].func_binding
+        package_class._package_spec.method_mappings[Verb.POST]["/test_package_route"].func_binding
         == "test_package_route"
     )
-    assert invoke(package, "test_mixin_route") == "mixin yo"
+    package = PackageWithMixin()
+    assert invoke(package, "test_mixin_route", text="test") == "mixin yo"
     assert invoke(package, "test_package_route") == "package"
 
     routes = [m["path"] for m in package.__steamship_dir__()["methods"]]

tests/steamship_tests/utils/deployables.py

@@ -1,9 +1,6 @@
 import contextlib
 import io
-import logging
 import os
-import subprocess  # noqa: S404
-import tempfile
 import zipfile
 from pathlib import Path
 from typing import Any, Dict, Optional
@@ -17,18 +14,6 @@
 from steamship.data.user import User
 
 
-def install_dependencies(folder: str, requirements_path: Path):
-    subprocess.run(  # noqa: S607,S603
-        ["pip", "install", "--target", folder, "-r", str(requirements_path.resolve())],
-        stdout=subprocess.PIPE,
-    )
-    # Write an empty requirements.txt to the test deployment.
-    # Note that this is INTENTIONALLY different than the behavior of a deployable
-    # deployed with the CLI, so that you can use the steamship version that you're currently editing
-    with open(Path(folder) / "requirements.txt", "w") as requirements_file:
-        requirements_file.write("")
-
-
 def zip_deployable(file_path: Path) -> bytes:
     """Prepare and zip a Steamship plugin."""
 
@@ -48,18 +33,7 @@ def zip_deployable(file_path: Path) -> bytes:
                 for file in files:
                     pypi_file = Path(root) / file
                     zip_file.write(pypi_file, pypi_file.relative_to(package_path.parent))
-
-        # Copy in package paths from pip
-        with tempfile.TemporaryDirectory() as package_dir:
-            logging.info(f"Created tempdir for pip installs: {package_dir}")
-            install_dependencies(
-                folder=package_dir, requirements_path=ROOT_PATH / "requirements.txt"
-            )
-            # Now write that whole folder
-            for root, _, files in os.walk(package_dir):
-                for file in files:
-                    pypi_file = Path(root) / file
-                    zip_file.write(pypi_file, pypi_file.relative_to(package_dir))
+        zip_file.write(ROOT_PATH / "requirements.txt", "requirements.txt")
 
         # Now we'll copy in the whole assets directory so that our test files can access things there..
         for root, _, files in os.walk(TEST_ASSETS_PATH):