From 529abc991f590c52f83fd3a82cb97beb8b6adf9f Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Fri, 3 Jul 2026 13:53:35 +1200 Subject: [PATCH] feat(assets): drop logical_path from the asset response Ratified in the loader-key sync: the namespaced locator buys nothing today (hash/ID-based locating is the long-term direction), and keeping it adds a third path-shaped field for clients to confuse. loader_path and display_name carry the loader and display concerns; the storage-root matching stays internal, still powering display_name. Response-surface only; no column or migration involved. --- app/assets/api/routes.py | 8 +++----- app/assets/api/schemas_out.py | 8 ++------ app/assets/services/path_utils.py | 19 +++++++++++-------- openapi.yaml | 6 +----- .../test_asset_response_loader_path.py | 2 -- tests-unit/assets_test/test_uploads.py | 10 ++-------- 6 files changed, 19 insertions(+), 34 deletions(-) diff --git a/app/assets/api/routes.py b/app/assets/api/routes.py index f1d9c118c..b66607265 100644 --- a/app/assets/api/routes.py +++ b/app/assets/api/routes.py @@ -40,7 +40,7 @@ from app.assets.services import ( ) from app.assets.services.cursor import InvalidCursorError from app.assets.services.path_utils import ( - compute_asset_response_paths, + compute_display_name, compute_loader_path, ) from app.assets.services.tagging import list_tag_histogram @@ -165,8 +165,7 @@ def _build_asset_response(result: schemas.AssetDetailResult | schemas.UploadResu else: preview_url = _build_preview_url_from_view(result.tags, result.ref.user_metadata) if result.ref.file_path: - paths = compute_asset_response_paths(result.ref.file_path) - logical_path, display_name = paths if paths else (None, None) + display_name = compute_display_name(result.ref.file_path) # In-root loader path (model category dropped): what model loaders consume. # Persisted at scan/ingest; fall back to computing for rows written # before the column existed. @@ -174,14 +173,13 @@ def _build_asset_response(result: schemas.AssetDetailResult | schemas.UploadResu if loader_path is None: loader_path = compute_loader_path(result.ref.file_path) else: - logical_path, display_name, loader_path = None, None, None + display_name, loader_path = None, None asset_content_hash = result.asset.hash if result.asset else None return schemas_out.Asset( id=result.ref.id, name=result.ref.name, hash=asset_content_hash, loader_path=loader_path, - logical_path=logical_path, display_name=display_name, asset_hash=asset_content_hash, size=int(result.asset.size_bytes) if result.asset else None, diff --git a/app/assets/api/schemas_out.py b/app/assets/api/schemas_out.py index 4bede72d7..77c9eae18 100644 --- a/app/assets/api/schemas_out.py +++ b/app/assets/api/schemas_out.py @@ -12,20 +12,16 @@ class Asset(BaseModel): name: str = Field( ..., deprecated=True, - description="Reference label, often caller-provided or derived from the filename. Deprecated for storage path/display semantics; use `loader_path`, `logical_path`, and `display_name` when present.", + description="Reference label, often caller-provided or derived from the filename. Deprecated for storage path/display semantics; use `loader_path` and `display_name` when present.", ) hash: str | None = None loader_path: str | None = Field( default=None, description="In-root loader path for filesystem-backed assets: the path relative to its storage root with the top-level model category dropped (e.g. `models/checkpoints/foo/bar.safetensors` -> `foo/bar.safetensors`). This is the value model loaders consume. `None` when the file is not within a recognized root or model category.", ) - logical_path: str | None = Field( - default=None, - description="Runtime storage locator for filesystem-backed assets, using Comfy storage namespaces such as `input/`, `output/`, `temp/`, or `models/` (e.g. `models/checkpoints/foo/bar.safetensors`). Not an absolute filesystem path, unique identity, or model loader path.", - ) display_name: str | None = Field( default=None, - description="Human-facing label derived from `logical_path`, usually the path below the top-level storage namespace. Not unique.", + description="Human-facing label for filesystem-backed assets: the path below the top-level storage namespace (e.g. `checkpoints/foo/bar.safetensors` under `models/`). Not unique.", ) asset_hash: str | None = None size: int | None = None diff --git a/app/assets/services/path_utils.py b/app/assets/services/path_utils.py index bb497c454..f3d4e2d60 100644 --- a/app/assets/services/path_utils.py +++ b/app/assets/services/path_utils.py @@ -84,11 +84,14 @@ def _is_relative_to(child: str, parent: str) -> bool: def compute_asset_response_paths(file_path: str) -> tuple[str, str | None] | None: - """Return public (file_path, display_name) response fields for a file path. + """Return (logical_path, display_name) for a file path. - These fields are storage locators, not model-loader namespaces. Registered - model-folder membership is represented by backend tags such as - ``model_type:``; response paths only use known storage roots. + ``logical_path`` is the internal namespaced storage locator (e.g. + ``models/checkpoints/foo/bar.safetensors``); ``display_name`` is the + human-facing label below that namespace, served on Asset responses. These + are storage locators, not model-loader namespaces. Registered model-folder + membership is represented by backend tags such as + ``model_type:``; these paths only use known storage roots. """ fp_abs = os.path.abspath(file_path) candidates: list[tuple[int, int, str, str]] = [] @@ -123,7 +126,7 @@ def compute_display_name(file_path: str) -> str | None: def compute_logical_path(file_path: str) -> str | None: - """Return the asset's namespaced storage `logical_path`, or None for unknown paths.""" + """Return the internal namespaced storage locator, or None for unknown paths.""" result = compute_asset_response_paths(file_path) return result[0] if result else None @@ -136,9 +139,9 @@ def compute_loader_path(file_path: str) -> str | None: /.../models/text_encoders/clip_g.safetensors -> "clip_g.safetensors" This is the value model loaders consume (the model category is dropped). It - backs the public Asset response `file_path` field and the internal - ``computed_filename`` metadata. The namespaced storage locator (`logical_path`) - and human-facing `display_name` come from compute_asset_response_paths(). + is persisted as ``AssetReference.loader_path`` and served as the public + Asset response `loader_path` field. The human-facing `display_name` comes + from compute_asset_response_paths(). For input/output/temp paths the full path relative to that root is returned. For paths outside any known root, returns None. diff --git a/openapi.yaml b/openapi.yaml index 68f59dd62..c37d543c7 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -15,12 +15,8 @@ components: description: 'In-root loader path for filesystem-backed assets: the path relative to its storage root with the top-level model category dropped (e.g. `models/checkpoints/foo/bar.safetensors` -> `foo/bar.safetensors`). This is the value model loaders consume. `None` when the file is not within a recognized root or model category.' nullable: true type: string - logical_path: - description: Runtime storage locator for filesystem-backed assets, using Comfy storage namespaces such as `input/`, `output/`, `temp/`, or `models/` (e.g. `models/checkpoints/foo/bar.safetensors`). Not an absolute filesystem path, unique identity, or model loader path. - nullable: true - type: string display_name: - description: Human-facing label derived from `logical_path`, usually the path below the top-level storage namespace. Not unique. + description: 'Human-facing label for filesystem-backed assets: the path below the top-level storage namespace (e.g. `checkpoints/foo/bar.safetensors` under `models/`). Not unique.' nullable: true type: string id: diff --git a/tests-unit/assets_test/services/test_asset_response_loader_path.py b/tests-unit/assets_test/services/test_asset_response_loader_path.py index fbe864152..7420f5734 100644 --- a/tests-unit/assets_test/services/test_asset_response_loader_path.py +++ b/tests-unit/assets_test/services/test_asset_response_loader_path.py @@ -69,7 +69,6 @@ def test_falls_back_to_compute_when_stored_loader_path_is_null(tmp_path: Path): resp = _build_asset_response(result) assert resp.loader_path == "bar.safetensors" - assert resp.logical_path == "models/checkpoints/bar.safetensors" assert resp.display_name == "checkpoints/bar.safetensors" @@ -80,5 +79,4 @@ def test_all_path_fields_null_without_file_path(): resp = _build_asset_response(result) assert resp.loader_path is None - assert resp.logical_path is None assert resp.display_name is None diff --git a/tests-unit/assets_test/test_uploads.py b/tests-unit/assets_test/test_uploads.py index f27172f4a..7be7b0935 100644 --- a/tests-unit/assets_test/test_uploads.py +++ b/tests-unit/assets_test/test_uploads.py @@ -233,15 +233,13 @@ def test_upload_multiple_tags_fields_are_merged(http: requests.Session, api_base ( "tags", "extension", - "expected_prefix", "expected_display_prefix", ), [ - (["input", "unit-tests"], ".png", "input", ""), + (["input", "unit-tests"], ".png", ""), ( ["models", "model_type:checkpoints", "unit-tests"], ".safetensors", - "models/checkpoints", "checkpoints/", ), ], @@ -249,7 +247,6 @@ def test_upload_multiple_tags_fields_are_merged(http: requests.Session, api_base def test_upload_response_includes_loader_path_and_display_name( tags: list[str], extension: str, - expected_prefix: str, expected_display_prefix: str, http: requests.Session, api_base: str, @@ -270,20 +267,18 @@ def test_upload_response_includes_loader_path_and_display_name( assert created_r.status_code in (200, 201), created stored_filename = get_asset_filename(created["asset_hash"], extension) expected_suffix = stored_filename - expected_logical_path = f"{expected_prefix}/{expected_suffix}" expected_display_name = f"{expected_display_prefix}{expected_suffix}" # In-root loader path: model category dropped, no subfolders here -> just the filename. expected_loader_path = expected_suffix assert created["loader_path"] == expected_loader_path - assert created["logical_path"] == expected_logical_path assert created["display_name"] == expected_display_name + assert "logical_path" not in created detail_r = http.get(f"{api_base}/api/assets/{created['id']}", timeout=120) detail = detail_r.json() assert detail_r.status_code == 200, detail assert detail["loader_path"] == expected_loader_path - assert detail["logical_path"] == expected_logical_path assert detail["display_name"] == expected_display_name list_r = http.get( @@ -295,7 +290,6 @@ def test_upload_response_includes_loader_path_and_display_name( assert list_r.status_code == 200, listed match = next(a for a in listed["assets"] if a["id"] == created["id"]) assert match["loader_path"] == expected_loader_path - assert match["logical_path"] == expected_logical_path assert match["display_name"] == expected_display_name