feat(assets): add job_ids filter to GET /api/assets#13998
Open
mattmillerai wants to merge 5 commits into
Open
Conversation
Mirrors the existing cloud `job_ids` query param on the local Python server: clients can pass a comma-separated list (or repeated query params) of UUIDs to filter assets by their associated job. The `AssetReference.job_id` column already exists, so no migration is needed — this just plumbs the filter through schema → service → query. Marks the parameter as available in both runtimes by dropping the `[cloud-only]` description prefix and the `x-runtime: [cloud]` tag from the OpenAPI spec, per the OSS field-drift convention (absent runtime tag = populated by both local and cloud).
mattmillerai
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
📝 WalkthroughWalkthroughThis PR adds a validated 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…-to-get-apiassets
From cursor-reviews on the parent commit: - OpenAPI: declare job_ids as `type: array, items: string format: uuid` with `style: form, explode: true` so it matches the documented contract (and matches sibling include_tags/exclude_tags shape). Description now states both accepted shapes explicitly. - Schema: cap `job_ids` at 500 entries (max_length on the Pydantic field) so a client can't splice an unbounded list into the IN clauses. - Schema: drop `AttributeError` from the except — `raw` only contains `str` items by construction, so `uuid.UUID(<str>)` raises `ValueError` exclusively; the second clause was dead code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi.yaml`:
- Around line 1562-1568: The OpenAPI schema for the job_ids query parameter in
ListAssetsQuery lacks the server-side cap; update the array schema for parameter
"job_ids" (the ListAssetsQuery / job_ids items block) to include maxItems: 500
so generated clients/docs reflect the runtime limit—add a top-level "maxItems:
500" property under the array definition that currently has "type: array" /
"items: { type: string, format: uuid }".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82e239f0-0540-4274-9925-967852ac96da
📒 Files selected for processing (2)
app/assets/api/schemas_in.pyopenapi.yaml
CodeRabbit catch on PR #13998: the Pydantic schema caps `job_ids` at 500 entries, but the OpenAPI parameter omitted `maxItems`, so generated clients and docs wouldn't reflect the runtime limit.
Aligns with the parallel hardening from draft PR #13848 (now closed as a duplicate). The validator now: - Raises ValueError on non-string list items (was: silently dropped). - Raises ValueError on non-string / non-list top-level values like dict or int (was: silently passed through to Pydantic's downstream coercion). Adds tests-unit/assets_test/queries/test_list_assets_query.py covering the validator end-to-end: CSV canonicalization, dedup order, default empty, invalid UUID, non-string list item, non-string non-list value, and the max_length=500 boundary.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests-unit/assets_test/queries/test_list_assets_query.py (1)
10-60: ⚡ Quick winConsider adding a test for mixed CSV-in-list input.
The validator supports CSV within list items (e.g.,
["uuid1,uuid2", "uuid3"]), which corresponds to the HTTP query string?job_ids=uuid1,uuid2&job_ids=uuid3. This behavior is implemented in line 102 of the validator but isn't explicitly tested.🧪 Suggested test to add
def test_mixed_csv_in_list(self): a = "11111111-1111-1111-1111-111111111111" b = "22222222-2222-2222-2222-222222222222" c = "33333333-3333-3333-3333-333333333333" # Simulates ?job_ids=uuid1,uuid2&job_ids=uuid3 q = ListAssetsQuery.model_validate({"job_ids": [f"{a},{b}", c]}) assert q.job_ids == [a, b, c]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-unit/assets_test/queries/test_list_assets_query.py` around lines 10 - 60, Add a unit test to cover CSV-in-list behavior by creating a new test method (e.g., test_mixed_csv_in_list) in TestJobIdsValidator that calls ListAssetsQuery.model_validate with {"job_ids": [f"{a},{b}", c]} using three UUID strings and asserts the resulting q.job_ids equals [a, b, c]; this verifies the validator (ListAssetsQuery.model_validate) expands CSV entries inside list items (the behavior implemented around line 102) and preserves order/deduplication/canonicalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests-unit/assets_test/queries/test_list_assets_query.py`:
- Around line 10-60: Add a unit test to cover CSV-in-list behavior by creating a
new test method (e.g., test_mixed_csv_in_list) in TestJobIdsValidator that calls
ListAssetsQuery.model_validate with {"job_ids": [f"{a},{b}", c]} using three
UUID strings and asserts the resulting q.job_ids equals [a, b, c]; this verifies
the validator (ListAssetsQuery.model_validate) expands CSV entries inside list
items (the behavior implemented around line 102) and preserves
order/deduplication/canonicalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17f8a6f3-0f54-422e-b2ba-67fd185015b8
📒 Files selected for processing (2)
app/assets/api/schemas_in.pytests-unit/assets_test/queries/test_list_assets_query.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
job_idsquery param toGET /api/assetson the Python local server, mirroring the existing cloud behavior.include_tags,exclude_tags,name_contains,metadata_filter, etc.).AssetReference.job_idcolumn already exists.OpenAPI
Drops the
[cloud-only]description prefix and thex-runtime: [cloud]tag from thejob_idsparameter — per the field-drift convention, an absent runtime tag means both local and cloud populate/accept the field.Test plan
pytest tests-unit/assets_test/queries -x -q(133 passed) — includes two new tests covering single-job filter, multi-job IN, unknown-job (empty result), empty/None passthrough, and combination withname_contains.ListAssetsQuery.job_idsvalidator: CSV input, list input, mixed CSV-in-list, dedup, default-empty, and invalid-UUID rejection (raisesValidationError).x-runtime: [cloud]) matches the project's expectation for now-bi-runtime fields.HTTP smoke test
Ran local ComfyUI from this branch with
--enable-assets, seeded threeAssetReferencerows directly:smoke-job-a.png(JOB_A=11111111-1111-1111-1111-111111111111),smoke-job-b.png(JOB_B=22222222-2222-2222-2222-222222222222), andsmoke-no-job.png(no job). Hit the live HTTP endpoint with eight query variants:?job_ids={A}smoke-job-a.png[smoke-job-a.png]?job_ids={A},{B}(comma-separated)a+b[a, b]?job_ids={A}&job_ids={B}(repeated)a+b[a, b]?job_ids={ghost UUID}?job_ids=not-a-uuidcode: INVALID_QUERY, structureddetails.errors[*].loc=["job_ids"]?job_ids={A},{B}&name_contains=job-asmoke-job-a.png[smoke-job-a.png]smoke-job-a.png[smoke-job-a.png]Net: single, multi, comma-separated, repeated, unknown, invalid, and combined-filter cases all behave correctly. Validation error surfaces a clean structured 400. No regression on the no-filter path.