Skip to content

Commit

Permalink
feat(endpoint): use legacy endpoints along new ones
Browse files Browse the repository at this point in the history
This entails a dynamically created class voodoo for the change not to poison too much of the existing code.

We should add test alternatives for those currently changed that they work properly with the LEGACY_ENDPOINTS turned off.
  • Loading branch information
vit-zikmund committed Mar 8, 2024
1 parent cf62ee5 commit f7e07ac
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 11 deletions.
15 changes: 14 additions & 1 deletion giftless/config.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Configuration handling helper functions and default configuration."""
import os
import warnings
from pathlib import Path
from typing import Any

Expand Down Expand Up @@ -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",
Expand All @@ -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


Expand Down
12 changes: 9 additions & 3 deletions giftless/transfer/basic_streaming.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
26 changes: 23 additions & 3 deletions giftless/view.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -31,10 +31,30 @@ class BaseView(FlaskView):
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(
Expand Down
4 changes: 2 additions & 2 deletions tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def test_upload_request_with_x_forwarded_middleware(
json = cast(dict[str, Any], response.json)
upload_action = json["objects"][0]["actions"]["upload"]
href = upload_action["href"]
base_uri = "myorg/myrepo.git/info/lfs"
base_uri = "myorg/myrepo"
assert href == f"http://localhost/{base_uri}/objects/storage/12345678"

response = test_client.post(
Expand All @@ -71,5 +71,5 @@ def test_upload_request_with_x_forwarded_middleware(
href = upload_action["href"]
assert (
href
== "https://mycompany.xyz:1234/lfs/myorg/myrepo.git/info/lfs/objects/storage/12345678"
== f"https://mycompany.xyz:1234/lfs/{base_uri}/objects/storage/12345678"
)
4 changes: 2 additions & 2 deletions tests/transfer/test_basic_external_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def test_upload_action_new_file() -> None:
"expires_in": 900,
},
"verify": {
"href": "http://giftless.local/myorg/myrepo.git/info/lfs/objects/storage/verify",
"href": "http://giftless.local/myorg/myrepo/objects/storage/verify",
"header": {},
"expires_in": 43200,
},
Expand Down Expand Up @@ -73,7 +73,7 @@ def test_upload_action_extras_are_passed() -> None:
"expires_in": 900,
},
"verify": {
"href": "http://giftless.local/myorg/myrepo.git/info/lfs/objects/storage/verify",
"href": "http://giftless.local/myorg/myrepo/objects/storage/verify",
"header": {},
"expires_in": 43200,
},
Expand Down

0 comments on commit f7e07ac

Please sign in to comment.