Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/assets/api/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
update_asset_metadata,
upload_from_temp_path,
)
from app.assets.services.path_utils import compute_paths_for_response
from app.assets.services.tagging import list_tag_histogram

ROUTES = web.RouteTableDef()
Expand Down Expand Up @@ -160,9 +161,16 @@ def _build_asset_response(result: schemas.AssetDetailResult | schemas.UploadResu
preview_url = None
else:
preview_url = _build_preview_url_from_view(result.tags, result.ref.user_metadata)
if result.ref.file_path:
paths = compute_paths_for_response(result.ref.file_path)
file_path, display_name = paths if paths else (None, None)
else:
file_path, display_name = None, None
return schemas_out.Asset(
id=result.ref.id,
name=result.ref.name,
file_path=file_path,
display_name=display_name,
asset_hash=result.asset.hash if result.asset else None,
size=int(result.asset.size_bytes) if result.asset else None,
mime_type=result.asset.mime_type if result.asset else None,
Expand Down
4 changes: 3 additions & 1 deletion app/assets/api/schemas_out.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ class Asset(BaseModel):
``id`` here is the AssetReference id, not the content-addressed Asset id."""

id: str
name: str
name: str = Field(..., deprecated=True)
file_path: str | None = None
display_name: str | None = None
asset_hash: str | None = None
size: int | None = None
mime_type: str | None = None
Expand Down
110 changes: 93 additions & 17 deletions app/assets/services/path_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

_NON_MODEL_FOLDER_NAMES = frozenset({"custom_nodes"})

RootCategory = Literal["input", "output", "temp", "models"]


def get_comfy_models_folders() -> list[tuple[str, list[str]]]:
"""Build list of (folder_name, base_paths[]) for all model locations.
Expand Down Expand Up @@ -65,35 +67,109 @@ def validate_path_within_base(candidate: str, base: str) -> None:
raise ValueError("destination escapes base directory")


def compute_paths_for_response(
file_path: str,
) -> tuple[str, str | None] | None:
"""Compute (file_path, display_name) for an Asset response.

`file_path` is a logical locator under the asset namespace: `<root>/<rel>`
for input/output/temp assets and `<root>/<bucket>/<rel>` for model assets.
`display_name` is the path below that root or model bucket, suitable for UI
labels. Returns None when the absolute path is not under a known asset root.
"""
try:
root, bucket, rel = get_asset_root_bucket_and_filepath(file_path)
except ValueError:
return None

display_name = rel or None
if bucket is None:
response_file_path = f"{root}/{rel}" if rel else root
else:
response_file_path = f"{root}/{bucket}/{rel}" if rel else f"{root}/{bucket}"
return response_file_path, display_name


def compute_display_name(file_path: str) -> str | None:
"""Return the asset's `display_name`, or None for unknown paths."""
result = compute_paths_for_response(file_path)
return result[1] if result else None


def compute_file_path(file_path: str) -> str | None:
"""Return the asset's logical `file_path`, or None for unknown paths."""
result = compute_paths_for_response(file_path)
return result[0] if result else None


def compute_relative_filename(file_path: str) -> str | None:
"""
Return the model's path relative to the last well-known folder (the model category),
using forward slashes, eg:
Return the path relative to the asset root or model category, using forward slashes, eg:
/.../models/checkpoints/flux/123/flux.safetensors -> "flux/123/flux.safetensors"
/.../models/text_encoders/clip_g.safetensors -> "clip_g.safetensors"
/.../input/sub/image.png -> "sub/image.png"

For non-model paths, returns None.
For unknown paths, returns None.
"""
try:
root_category, rel_path = get_asset_category_and_relative_path(file_path)
except ValueError:
return None
return compute_display_name(file_path)

p = Path(rel_path)
parts = [seg for seg in p.parts if seg not in (".", "..", p.anchor)]
if not parts:
return None

if root_category == "models":
# parts[0] is the category ("checkpoints", "vae", etc) – drop it
inside = parts[1:] if len(parts) > 1 else [parts[0]]
return "/".join(inside)
return "/".join(parts) # input/output: keep all parts
def get_asset_root_bucket_and_filepath(
file_path: str,
) -> tuple[RootCategory, str | None, str]:
"""Decompose an absolute path into (root, bucket, path-under-bucket).

`bucket` is only set for model assets. The returned relative path always
uses `/` separators and is empty when the path is exactly the matched root.
Comment on lines +122 to +123
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function relies on a flawed assumption that all models will be in the models directory. As an example, the built-in SaveCheckpoint, SaveLora, etc. models actually save to the outputs directory but are still loadable as models.
If possible, I would really like to avoid special-casing models as that's usually going to fail for edge cases. Could you explain a bit more about why we actually need the bucket as a first-class concept?


Raises:
ValueError: path does not belong to any known root.
"""
fp_abs = os.path.abspath(file_path)

def _check_is_within(child: str, parent: str) -> bool:
return Path(child).is_relative_to(parent)

def _compute_relative(child: str, parent: str) -> str:
# Normalize relative path, stripping any leading ".." components
# by anchoring to root (os.sep) then computing relpath back from it.
rel = os.path.relpath(
os.path.join(os.sep, os.path.relpath(child, parent)), os.sep
)
return "" if rel == "." else rel.replace(os.sep, "/")

for root_tag, getter in (
("input", folder_paths.get_input_directory),
("output", folder_paths.get_output_directory),
("temp", folder_paths.get_temp_directory),
):
base = os.path.abspath(getter())
if _check_is_within(fp_abs, base):
return root_tag, None, _compute_relative(fp_abs, base)

# models: check deepest matching base to avoid ambiguity.
best: tuple[int, str, str] | None = None
for bucket, bases in get_comfy_models_folders():
for b in bases:
base_abs = os.path.abspath(b)
if not _check_is_within(fp_abs, base_abs):
continue
cand = (len(base_abs), bucket, _compute_relative(fp_abs, base_abs))
if best is None or cand[0] > best[0]:
best = cand

if best is not None:
_, bucket, rel_inside = best
return "models", bucket, rel_inside

raise ValueError(
f"Path is not within input, output, temp, or configured model bases: {file_path}"
)


def get_asset_category_and_relative_path(
file_path: str,
) -> tuple[Literal["input", "output", "temp", "models"], str]:
) -> tuple[RootCategory, str]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this isn't actually new to this PR, but I'm a little concerned about the assumption that models will be exactly one of these. We should make sure this works for cases like the built-in SaveCheckpoint or SaveLora nodes where the model is located under the outputs directory, but should also be treated as a model.

"""Determine which root category a file path belongs to.

Categories:
Expand Down
13 changes: 12 additions & 1 deletion openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6328,7 +6328,18 @@ components:
description: Unique identifier for the asset
name:
type: string
deprecated: true
description: Name of the asset file
file_path:
type: string
nullable: true
x-runtime: [cloud, local]
description: "Logical asset locator under the namespace root. Not a unique reference key; use `id` for identity."
display_name:
type: string
nullable: true
x-runtime: [cloud, local]
description: "Human-facing display label for the asset. Not a unique reference key; use `id` for identity."
hash:
type: string
nullable: true
Expand Down Expand Up @@ -8109,4 +8120,4 @@ components:
items:
$ref: "#/components/schemas/TaskEntry"
pagination:
$ref: "#/components/schemas/PaginationInfo"
$ref: "#/components/schemas/PaginationInfo"
30 changes: 29 additions & 1 deletion tests-unit/assets_test/services/test_path_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@

import pytest

from app.assets.services.path_utils import get_asset_category_and_relative_path
from app.assets.services.path_utils import (
compute_display_name,
compute_file_path,
get_asset_category_and_relative_path,
)


@pytest.fixture
Expand Down Expand Up @@ -79,3 +83,27 @@ def test_model_file(self, fake_dirs):
def test_unknown_path_raises(self, fake_dirs):
with pytest.raises(ValueError, match="not within"):
get_asset_category_and_relative_path("/some/random/path.png")


class TestResponsePaths:
def test_input_file_path_and_display_name_include_subfolder(self, fake_dirs):
sub = fake_dirs["input"] / "some" / "folder"
sub.mkdir(parents=True)
f = sub / "image.png"
f.touch()

assert compute_file_path(str(f)) == "input/some/folder/image.png"
assert compute_display_name(str(f)) == "some/folder/image.png"

def test_model_file_path_includes_bucket_display_name_drops_it(self, fake_dirs):
sub = fake_dirs["models"] / "flux"
sub.mkdir()
f = sub / "model.safetensors"
f.touch()

assert compute_file_path(str(f)) == "models/checkpoints/flux/model.safetensors"
assert compute_display_name(str(f)) == "flux/model.safetensors"

def test_unknown_path_returns_none(self, fake_dirs):
assert compute_file_path("/some/random/path.png") is None
assert compute_display_name("/some/random/path.png") is None
58 changes: 58 additions & 0 deletions tests-unit/assets_test/test_uploads.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import requests
import pytest

from helpers import get_asset_filename


def test_upload_ok_duplicate_reference(http: requests.Session, api_base: str, make_asset_bytes):
name = "dup_a.safetensors"
Expand Down Expand Up @@ -63,6 +65,14 @@ def test_upload_fastpath_from_existing_hash_no_file(http: requests.Session, api_
assert r2.status_code == 200, b2 # fast path returns 200 with created_new == False
assert b2["created_new"] is False
assert b2["asset_hash"] == h
assert b2.get("file_path") is None
assert b2.get("display_name") is None

rg = http.get(f"{api_base}/api/assets/{b2['id']}", timeout=120)
detail = rg.json()
assert rg.status_code == 200, detail
assert detail.get("file_path") is None
assert detail.get("display_name") is None


def test_upload_fastpath_with_known_hash_and_file(
Expand Down Expand Up @@ -107,6 +117,54 @@ def test_upload_multiple_tags_fields_are_merged(http: requests.Session, api_base
assert {"models", "checkpoints", "unit-tests", "alpha"}.issubset(tags)


@pytest.mark.parametrize(
("tags", "extension", "expected_prefix", "expected_display_prefix"),
[
(["input", "unit-tests"], ".png", "input", ""),
(["models", "checkpoints", "unit-tests"], ".safetensors", "models/checkpoints", ""),
],
)
def test_upload_response_includes_file_path_and_display_name(
tags: list[str],
extension: str,
expected_prefix: str,
expected_display_prefix: str,
http: requests.Session,
api_base: str,
asset_factory,
make_asset_bytes,
):
scope = f"response-paths-{uuid.uuid4().hex[:6]}"
scoped_tags = [*tags, scope]
name = f"asset_response_path{extension}"

created = asset_factory(name, scoped_tags, {}, make_asset_bytes(name, 1024))
stored_filename = get_asset_filename(created["asset_hash"], extension)
expected_suffix = f"unit-tests/{scope}/{stored_filename}"
expected_file_path = f"{expected_prefix}/{expected_suffix}"
expected_display_name = f"{expected_display_prefix}{expected_suffix}"

assert created["file_path"] == expected_file_path
assert created["display_name"] == expected_display_name

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["file_path"] == expected_file_path
assert detail["display_name"] == expected_display_name

list_r = http.get(
api_base + "/api/assets",
params={"include_tags": f"unit-tests,{scope}", "limit": "50"},
timeout=120,
)
listed = list_r.json()
assert list_r.status_code == 200, listed
match = next(a for a in listed["assets"] if a["id"] == created["id"])
assert match["file_path"] == expected_file_path
assert match["display_name"] == expected_display_name


@pytest.mark.parametrize("root", ["input", "output"])
def test_concurrent_upload_identical_bytes_different_names(
root: str,
Expand Down
Loading