Merge pull request #152 from the-mama-ai/endpoints

feat: use endpoints compatible with LFS server discovery

giftless/config.py

@@ -1,5 +1,6 @@
 """Configuration handling helper functions and default configuration."""
 import os
+import warnings
 from pathlib import Path
 from typing import Any
 
@@ -29,9 +30,10 @@
 }
 
 default_config = {
-    "TRANSFER_ADAPTERS": Extensible(default_transfer_config),
     "TESTING": False,
     "DEBUG": False,
+    "LEGACY_ENDPOINTS": True,
+    "TRANSFER_ADAPTERS": Extensible(default_transfer_config),
     "AUTH_PROVIDERS": ["giftless.auth.allow_anon:read_only"],
     "PRE_AUTHORIZED_ACTION_PROVIDER": {
         "factory": "giftless.auth.jwt:factory",
@@ -55,6 +57,17 @@ def configure(app: Flask, additional_config: dict | None = None) -> Flask:
     """Configure a Flask app using Figcan managed configuration object."""
     config = _compose_config(additional_config)
     app.config.update(config)
+    if app.config["LEGACY_ENDPOINTS"]:
+        warnings.warn(
+            FutureWarning(
+                "LEGACY_ENDPOINTS (starting with '<org>/<repo>/') are enabled"
+                " as the default. They will be eventually removed in favor of"
+                " those starting with '<org-path>/<repo>.git/info/lfs/')."
+                " Switch your clients to them and set the configuration"
+                " option to False to disable this warning."
+            ),
+            stacklevel=1,
+        )
     return app
 
 

giftless/transfer/basic_streaming.py

@@ -10,7 +10,7 @@
 from typing import Any, BinaryIO, cast
 
 import marshmallow
-from flask import Flask, Response, request, url_for
+from flask import Flask, Response, current_app, request, url_for
 from flask_classful import route
 from webargs.flaskparser import parser
 
@@ -33,7 +33,7 @@ class VerifyView(BaseView):
     make the test structures a little less weird?
     """
 
-    route_base = "<organization>/<repo>/objects/storage"
+    route_base = "objects/storage"
 
     def __init__(self, storage: VerifiableStorage) -> None:
         self.storage = storage
@@ -61,7 +61,10 @@ def get_verify_url(
         cls, organization: str, repo: str, oid: str | None = None
     ) -> str:
         """Get the URL for upload / download requests for this object."""
-        op_name = f"{cls.__name__}:verify"
+        # Use the legacy endpoint when enabled
+        # see giftless.view.BaseView:register for details
+        legacy = "Legacy" if current_app.config["LEGACY_ENDPOINTS"] else ""
+        op_name = f"{legacy}{cls.__name__}:verify"
         url: str = url_for(
             op_name,
             organization=organization,
@@ -75,7 +78,7 @@ def get_verify_url(
 class ObjectsView(BaseView):
     """Provides methods for object storage management."""
 
-    route_base = "<organization>/<repo>/objects/storage"
+    route_base = "objects/storage"
 
     def __init__(self, storage: StreamingStorage) -> None:
         self.storage = storage
@@ -138,7 +141,10 @@ def get_storage_url(
         oid: str | None = None,
     ) -> str:
         """Get the URL for upload / download requests for this object."""
-        op_name = f"{cls.__name__}:{operation}"
+        # Use the legacy endpoint when enabled
+        # see giftless.view.BaseView:register for details
+        legacy = "Legacy" if current_app.config["LEGACY_ENDPOINTS"] else ""
+        op_name = f"{legacy}{cls.__name__}:{operation}"
         url: str = url_for(
             op_name,
             organization=organization,

giftless/view.py

@@ -1,5 +1,5 @@
 """Flask-Classful View Classes."""
-from typing import Any, ClassVar
+from typing import Any, ClassVar, cast
 
 from flask import Flask
 from flask_classful import FlaskView
@@ -23,13 +23,38 @@ class BaseView(FlaskView):
         "flask-classful/default": representation.output_git_lfs_json,
     }
 
+    route_prefix: ClassVar = "<path:organization>/<repo>.git/info/lfs/"
+    # [flask-classful bug/feat?] Placeholders in route_prefix not skipped for
+    # building the final rule for methods with them (FlaskView.build_rule).
+    base_args: ClassVar = ["organization", "repo"]
+
     trailing_slash = False
 
     @classmethod
-    def register(cls, *args: Any, **kwargs: Any) -> Any:
+    def register(cls, app: Flask, *args: Any, **kwargs: Any) -> Any:
         if kwargs.get("base_class") is None:
             kwargs["base_class"] = BaseView
-        return super().register(*args, **kwargs)
+        if (
+            app.config["LEGACY_ENDPOINTS"]
+            and kwargs.get("route_prefix") is None
+            and not hasattr(cls, "_legacy_")  # break the cycle
+        ):
+            # To span any transition required for the switch to the current
+            # endpoint URI, create a "Legacy" class "copy" of this view and
+            # register it too, for both the views and their endpoints to
+            # coexist.
+            legacy_view = type(
+                f"Legacy{cls.__name__}",
+                (cls,),
+                {
+                    "route_prefix": "<organization>/<repo>/",
+                    "_legacy_": True,
+                },
+            )
+            legacy_view = cast(BaseView, legacy_view)
+            legacy_view.register(app, *args, **kwargs)
+
+        return super().register(app, *args, **kwargs)
 
     @classmethod
     def _check_authorization(
@@ -64,7 +89,7 @@ def _is_authorized(
 class BatchView(BaseView):
     """Batch operations."""
 
-    route_base = "<organization>/<repo>/objects/batch"
+    route_base = "objects/batch"
 
     def post(self, organization: str, repo: str) -> dict[str, Any]:
         """Batch operations."""

pyproject.toml

@@ -126,6 +126,8 @@ filterwarnings = [
     "ignore:.*pkg_resources\\.declare_namespace:DeprecationWarning",
     # Bug in kopf
     "ignore:.*require all values to be sortable:DeprecationWarning:kopf.*",
+    # Our warnings towards users
+    "ignore::FutureWarning",
 ]
 # The python_files setting is not for test detection (pytest will pick up any
 # test files named *_test.py without this setting) but to enable special

tests/conftest.py

@@ -2,6 +2,7 @@
 import pathlib
 import shutil
 from collections.abc import Generator
+from typing import Any
 
 import flask
 import pytest
@@ -10,6 +11,7 @@
 
 from giftless.app import init_app
 from giftless.auth import allow_anon, authentication
+from tests.helpers import legacy_endpoints_id
 
 
 @pytest.fixture
@@ -22,12 +24,14 @@ def storage_path(tmp_path: pathlib.Path) -> Generator:
         shutil.rmtree(path)
 
 
-@pytest.fixture
-def app(storage_path: str) -> flask.Flask:
+@pytest.fixture(params=[False], ids=legacy_endpoints_id)
+def app(storage_path: str, request: Any) -> flask.Flask:
     """Session fixture to configure the Flask app."""
+    legacy_endpoints = request.param
     app = init_app(
         additional_config={
             "TESTING": True,
+            "LEGACY_ENDPOINTS": legacy_endpoints,
             "TRANSFER_ADAPTERS": {
                 "basic": {
                     "options": {"storage_options": {"path": storage_path}}

tests/helpers.py

@@ -3,6 +3,8 @@
 from pathlib import Path
 from typing import Any
 
+import flask
+
 
 def batch_request_payload(
     delete_keys: Sequence[str] = (), **kwargs: Any
@@ -39,3 +41,14 @@ def create_file_in_storage(
     with Path(repo_path / filename).open("wb") as f:
         for c in (b"0" for _ in range(size)):
             f.write(c)
+
+
+def legacy_endpoints_id(enabled: bool) -> str:
+    return "legacy-ep" if enabled else "current-ep"
+
+
+def expected_uri_prefix(app: flask.Flask, *args: str) -> str:
+    core_prefix = "/".join(args)
+    if not app.config.get("LEGACY_ENDPOINTS"):
+        return core_prefix + ".git/info/lfs"
+    return core_prefix

tests/test_batch_api.py

@@ -12,7 +12,7 @@ def test_upload_batch_request(test_client: FlaskClient) -> None:
     """Test basic batch API with a basic successful upload request."""
     request_payload = batch_request_payload(operation="upload")
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 200
@@ -40,7 +40,7 @@ def test_download_batch_request(
     create_file_in_storage(storage_path, "myorg", "myrepo", oid, size=8)
 
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 200
@@ -70,7 +70,7 @@ def test_download_batch_request_two_files_one_missing(
     request_payload["objects"].append({"oid": "12345679", "size": 5555})
 
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 200
@@ -104,7 +104,7 @@ def test_download_batch_request_two_files_missing(
     request_payload["objects"].append({"oid": "12345679", "size": 5555})
 
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 404
@@ -139,7 +139,7 @@ def test_download_batch_request_two_files_one_mismatch(
     )
 
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 200
@@ -177,7 +177,7 @@ def test_download_batch_request_one_file_mismatch(
     )
 
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 422
@@ -206,7 +206,7 @@ def test_download_batch_request_two_files_different_errors(
     )
 
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 422

tests/test_error_responses.py

@@ -7,7 +7,7 @@
 def test_error_response_422(test_client: FlaskClient) -> None:
     """Test an invalid payload error."""
     response = test_client.post(
-        "/myorg/myrepo/objects/batch",
+        "/myorg/myrepo.git/info/lfs/objects/batch",
         json=batch_request_payload(delete_keys=["operation"]),
     )
 
@@ -30,7 +30,7 @@ def test_error_response_403(test_client: FlaskClient) -> None:
     read-only setup.
     """
     response = test_client.post(
-        "/myorg/myrepo/objects/batch",
+        "/myorg/myrepo.git/info/lfs/objects/batch",
         json=batch_request_payload(operation="upload"),
     )
 

tests/test_middleware.py

@@ -1,17 +1,21 @@
 """Tests for using middleware and some specific middleware."""
 from typing import Any, cast
 
+import flask
 import pytest
-from flask import Flask
 from flask.testing import FlaskClient
 
 from giftless.app import init_app
 
-from .helpers import batch_request_payload
+from .helpers import (
+    batch_request_payload,
+    expected_uri_prefix,
+    legacy_endpoints_id,
+)
 
 
 @pytest.fixture
-def app(storage_path: str) -> Flask:
+def app(storage_path: str) -> flask.Flask:
     """Session fixture to configure the Flask app."""
     return init_app(
         additional_config={
@@ -36,25 +40,32 @@ def app(storage_path: str) -> Flask:
 
 
 @pytest.mark.usefixtures("_authz_full_access")
+@pytest.mark.parametrize(
+    "app", [False, True], ids=legacy_endpoints_id, indirect=True
+)
 def test_upload_request_with_x_forwarded_middleware(
+    app: flask.Flask,
     test_client: FlaskClient,
 ) -> None:
     """Test the ProxyFix middleware generates correct URLs if
     X-Forwarded headers are set.
     """
     request_payload = batch_request_payload(operation="upload")
     response = test_client.post(
-        "/myorg/myrepo/objects/batch", json=request_payload
+        "/myorg/myrepo.git/info/lfs/objects/batch", json=request_payload
     )
 
     assert response.status_code == 200
     json = cast(dict[str, Any], response.json)
     upload_action = json["objects"][0]["actions"]["upload"]
     href = upload_action["href"]
-    assert href == "http://localhost/myorg/myrepo/objects/storage/12345678"
+    exp_uri_prefix = expected_uri_prefix(app, "myorg", "myrepo")
+    assert (
+        href == f"http://localhost/{exp_uri_prefix}/objects/storage/12345678"
+    )
 
     response = test_client.post(
-        "/myorg/myrepo/objects/batch",
+        "/myorg/myrepo.git/info/lfs/objects/batch",
         json=request_payload,
         headers={
             "X-Forwarded-Host": "mycompany.xyz",
@@ -70,5 +81,5 @@ def test_upload_request_with_x_forwarded_middleware(
     href = upload_action["href"]
     assert (
         href
-        == "https://mycompany.xyz:1234/lfs/myorg/myrepo/objects/storage/12345678"
+        == f"https://mycompany.xyz:1234/lfs/{exp_uri_prefix}/objects/storage/12345678"
     )