mirror of
https://github.com/comfyanonymous/ComfyUI.git
synced 2026-06-22 15:59:45 +08:00
fix(assets): reject unregistered model_type: on model-asset edit
Make the edit path symmetric with upload: model_type: is an operational placement tag on a model asset, so an unregistered folder_name is an invalid placement target and is rejected (400 UNKNOWN_MODEL_TYPE), not stored as a label. A real edit-type action always targets a registered folder from discovery, so only junk manual adds are affected. Narrowly update the system-looking-labels test to drop the now-operational bare model_type: case (other reserved-ish prefixes still store as labels) and add a focused unknown-model_type:-on-model-asset rejection test.
This commit is contained in:
parent
bf898cc552
commit
5e267faf02
@ -540,16 +540,17 @@ def relocate_model_asset_for_model_type_tags(
|
|||||||
if target_folder in current_folders:
|
if target_folder in current_folders:
|
||||||
return False
|
return False
|
||||||
|
|
||||||
# An unregistered folder_name cannot correspond to any real on-disk
|
# On a model asset, model_type: is an operational placement tag (it decides
|
||||||
# location, so there is nothing to relocate. Keep Core's established
|
# where the file lives), not a free-form label — exactly as it is for a
|
||||||
# permissive model_type: labeling (spec-drift §3: Core is local/trusted and
|
# new-byte upload (resolve_destination_from_tags). An edit IS a placement,
|
||||||
# does not reject model_type: labels) — store it as a plain label, don't
|
# so an unregistered folder_name is an invalid placement target and is
|
||||||
# reject. A genuine edit-type action always targets a registered folder_name
|
# rejected on both paths. A genuine edit-type action always targets a
|
||||||
# from the discovery vocabulary, so this only affects manual label adds.
|
# registered folder_name from the discovery vocabulary, so this only fires
|
||||||
|
# on junk manual model_type: adds.
|
||||||
try:
|
try:
|
||||||
new_base = get_model_base_for_folder(target_folder)
|
new_base = get_model_base_for_folder(target_folder)
|
||||||
except ValueError:
|
except ValueError as e:
|
||||||
return False
|
raise ModelMoveError("UNKNOWN_MODEL_TYPE", str(e), status=400)
|
||||||
|
|
||||||
rel = compute_relative_filename(old_path)
|
rel = compute_relative_filename(old_path)
|
||||||
if not rel:
|
if not rel:
|
||||||
|
|||||||
@ -252,25 +252,24 @@ class TestNoMoveCases:
|
|||||||
assert src.exists()
|
assert src.exists()
|
||||||
|
|
||||||
|
|
||||||
class TestUnknownFolderIsLabelOnly:
|
class TestRejects:
|
||||||
def test_unknown_folder_is_stored_as_label_not_rejected(
|
def test_unknown_folder_on_model_asset_rejected_400(
|
||||||
self, mock_create_session, session: Session, model_dirs
|
self, mock_create_session, session: Session, model_dirs
|
||||||
):
|
):
|
||||||
# Core stays permissive about model_type: LABELS (spec-drift §3): an
|
# On a model asset, model_type: is an operational placement tag, so an
|
||||||
# unregistered folder_name can't map to a real location, so it's a plain
|
# unregistered folder is an invalid placement target -> reject (symmetric
|
||||||
# label, not a move and not a reject.
|
# with the new-byte upload path). The file is not moved and no tag is
|
||||||
|
# added (the POST is atomic).
|
||||||
src = model_dirs["checkpoints"] / "m.safetensors"
|
src = model_dirs["checkpoints"] / "m.safetensors"
|
||||||
ref = _make_fs_ref(session, src, ["models", "model_type:checkpoints"])
|
ref = _make_fs_ref(session, src, ["models", "model_type:checkpoints"])
|
||||||
|
|
||||||
result = apply_tags(reference_id=ref.id, tags=["model_type:bogus"])
|
with pytest.raises(ModelMoveError) as ei:
|
||||||
|
apply_tags(reference_id=ref.id, tags=["model_type:bogus"])
|
||||||
|
assert ei.value.status == 400
|
||||||
|
assert ei.value.code == "UNKNOWN_MODEL_TYPE"
|
||||||
assert src.exists() # not moved
|
assert src.exists() # not moved
|
||||||
assert "model_type:bogus" in result.total_tags
|
assert _tags_after(session, ref.id) == {"models", "model_type:checkpoints"}
|
||||||
# The real path-derived type is untouched.
|
|
||||||
assert "model_type:checkpoints" in result.total_tags
|
|
||||||
|
|
||||||
|
|
||||||
class TestRejects:
|
|
||||||
def test_collision_rejected_409_and_not_clobbered(
|
def test_collision_rejected_409_and_not_clobbered(
|
||||||
self, mock_create_session, session: Session, model_dirs
|
self, mock_create_session, session: Session, model_dirs
|
||||||
):
|
):
|
||||||
|
|||||||
@ -127,12 +127,16 @@ def test_add_system_looking_tags_allowed_as_labels(
|
|||||||
):
|
):
|
||||||
aid = seeded_asset["id"]
|
aid = seeded_asset["id"]
|
||||||
|
|
||||||
|
# NB: a bare `model_type:` is intentionally NOT in this list. On a model
|
||||||
|
# asset model_type: is an operational placement tag (BE-1641), not a
|
||||||
|
# free-form label, so an unregistered one is rejected (covered by
|
||||||
|
# test_edit_type_unknown_folder_rejected). Every other system-looking
|
||||||
|
# prefix below is still stored verbatim as a label.
|
||||||
response = http.post(
|
response = http.post(
|
||||||
f"{api_base}/api/assets/{aid}/tags",
|
f"{api_base}/api/assets/{aid}/tags",
|
||||||
json={
|
json={
|
||||||
"tags": [
|
"tags": [
|
||||||
"models",
|
"models",
|
||||||
"model_type:manual",
|
|
||||||
"model:true",
|
"model:true",
|
||||||
"models:foo",
|
"models:foo",
|
||||||
"input:true",
|
"input:true",
|
||||||
@ -148,7 +152,6 @@ def test_add_system_looking_tags_allowed_as_labels(
|
|||||||
|
|
||||||
assert response.status_code == 200, body
|
assert response.status_code == 200, body
|
||||||
assert "models" in body["total_tags"]
|
assert "models" in body["total_tags"]
|
||||||
assert "model_type:manual" in body["total_tags"]
|
|
||||||
assert "model:true" in body["total_tags"]
|
assert "model:true" in body["total_tags"]
|
||||||
assert "models:foo" in body["total_tags"]
|
assert "models:foo" in body["total_tags"]
|
||||||
assert "input:true" in body["total_tags"]
|
assert "input:true" in body["total_tags"]
|
||||||
@ -324,14 +327,15 @@ def test_edit_type_moves_file_and_reregisters(
|
|||||||
http.delete(f"{api_base}/api/assets/{aid}", timeout=30)
|
http.delete(f"{api_base}/api/assets/{aid}", timeout=30)
|
||||||
|
|
||||||
|
|
||||||
def test_edit_type_unknown_folder_is_label_only(
|
def test_edit_type_unknown_folder_rejected(
|
||||||
http: requests.Session,
|
http: requests.Session,
|
||||||
api_base: str,
|
api_base: str,
|
||||||
):
|
):
|
||||||
"""An unregistered model_type: target stays a plain label (no move/reject).
|
"""An unregistered model_type: target on a model asset is rejected (400).
|
||||||
|
|
||||||
Core stays permissive about model_type: labels; only a registered folder
|
model_type: is an operational placement tag on a model asset, so an
|
||||||
triggers the edit-type move. The file must not move.
|
unregistered folder is an invalid placement target — symmetric with the
|
||||||
|
new-byte upload path. The prior type is left intact (atomic).
|
||||||
"""
|
"""
|
||||||
name = f"edit_bad_{uuid.uuid4().hex[:8]}.safetensors"
|
name = f"edit_bad_{uuid.uuid4().hex[:8]}.safetensors"
|
||||||
asset = _upload_model(http, api_base, name, ["models", "model_type:checkpoints", "unit-tests"])
|
asset = _upload_model(http, api_base, name, ["models", "model_type:checkpoints", "unit-tests"])
|
||||||
@ -342,9 +346,11 @@ def test_edit_type_unknown_folder_is_label_only(
|
|||||||
json={"tags": ["model_type:does_not_exist"]},
|
json={"tags": ["model_type:does_not_exist"]},
|
||||||
timeout=120,
|
timeout=120,
|
||||||
)
|
)
|
||||||
body = r.json()
|
assert r.status_code == 400, r.json()
|
||||||
assert r.status_code == 200, body
|
|
||||||
assert "model_type:does_not_exist" in body["total_tags"]
|
rg = http.get(f"{api_base}/api/assets/{aid}", timeout=120)
|
||||||
assert "model_type:checkpoints" in body["total_tags"]
|
tags = set(rg.json()["tags"])
|
||||||
|
assert "model_type:does_not_exist" not in tags
|
||||||
|
assert "model_type:checkpoints" in tags
|
||||||
|
|
||||||
http.delete(f"{api_base}/api/assets/{aid}", timeout=30)
|
http.delete(f"{api_base}/api/assets/{aid}", timeout=30)
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user