feat: implement remaining listAssets contract fields (display_name, hash filter, include_public)#14690
feat: implement remaining listAssets contract fields (display_name, hash filter, include_public)#14690mattmillerai wants to merge 2 commits into
Conversation
…ash filter, include_public) Bring GET /api/assets into param-for-param parity with the projected openapi.yaml listAssets contract for the three remaining fields: - Add display_name to the Asset response schema (nullable, mirrors name) and populate it from ref.name in _build_asset_response, covering list, get, create, update, and upload responses uniformly. - Add the hash query param to ListAssetsQuery (named hash per the spec, not asset_hash) with before-validation strip/lower normalization, and thread it through list_assets_page -> list_references_page, filtering both the page and count statements on Asset.hash for consistency. - Accept include_public (bool, default true) for contract parity; it is inert in core (no public asset pool) and intentionally not passed to the service layer. Cursor pagination and size optionality are untouched. Add integration tests covering display_name mirroring, exact hash match, unknown-hash empty page, and include_public acceptance.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request adds hash-based filtering to the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests-unit/assets_test/test_list_filter.py (1)
329-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for hash normalization.
This PR adds lowercase/trim normalization in
ListAssetsQuery._normalize_hash, but the new hash tests only cover already-normalized input. A case like" BLAKE3:... "would lock in the contract you just introduced.🤖 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/test_list_filter.py` around lines 329 - 367, The new hash filter tests in test_list_assets_hash_filter_exact_match and test_list_assets_hash_filter_no_match only verify already-normalized values, so they do not cover the normalization behavior added in ListAssetsQuery._normalize_hash. Add a test in the same area that passes a hash with uppercase letters and surrounding whitespace (for example a BLAKE3 value with extra spaces) to /api/assets, then assert it matches the expected asset and still returns 200. Keep the existing exact-match and no-match coverage, but extend it to lock in the trim/lowercase normalization contract.
🤖 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/test_list_filter.py`:
- Around line 329-367: The new hash filter tests in
test_list_assets_hash_filter_exact_match and
test_list_assets_hash_filter_no_match only verify already-normalized values, so
they do not cover the normalization behavior added in
ListAssetsQuery._normalize_hash. Add a test in the same area that passes a hash
with uppercase letters and surrounding whitespace (for example a BLAKE3 value
with extra spaces) to /api/assets, then assert it matches the expected asset and
still returns 200. Keep the existing exact-match and no-match coverage, but
extend it to lock in the trim/lowercase normalization contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 106dd53a-aa03-4d25-9c89-da39cf12b954
📒 Files selected for processing (6)
app/assets/api/routes.pyapp/assets/api/schemas_in.pyapp/assets/api/schemas_out.pyapp/assets/database/queries/asset_reference.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_list_filter.py
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
Address review findings on the listAssets contract PR: - Empty `?hash=` no longer collapses to "no filter" (which returned a full unfiltered page); it now stays "" and is matched exactly, yielding an empty page — consistent with the documented "malformed -> empty page" contract. Omitting the param entirely still disables the filter. - Guard `list_references_page` on `asset_hash is not None` (page + count) so the present-empty vs omitted distinction is preserved. - Add tests for hash normalization (uppercase/whitespace) and for the explicit-empty -> empty-page behavior. - Clarify the `include_public` comment: core reads are owner-scoped with no separate public pool, so the flag is inert here; cloud enforces it in its own service layer.
988d1db to
058692a
Compare
ELI-5
The list-assets endpoint (
GET /api/assets) was missing three fields that the OpenAPI spec already promises. This adds them so the API matches the spec:display_name— a copy ofnamein each asset, for older clients that expect that field.hash— a query filter so you can ask "give me only the asset with exactly this content hash."include_public— a query flag accepted for compatibility with the cloud API. Core has no public/shared asset pool, so it does nothing here; it's accepted (not rejected) so frontends don't need a special-case branch.What changed
Brings
GET /api/assetsinto param-for-param / field-for-field parity with thelistAssetsoperation andAssetschema already projected intoopenapi.yaml. Cursor pagination (after/next_cursor) and the optionalsizefield were already implemented and are untouched.app/assets/api/schemas_out.py— adddisplay_name: str | NonetoAsset(nullable, mirrorsname).AssetCreatedinherits it.app/assets/api/routes.py— populatedisplay_name=result.ref.namein_build_asset_response, which uniformly covers list, get, create, update, and upload responses.app/assets/api/schemas_in.py— addhash(the spec's query-param name — notasset_hash, which remains the response-body field) andinclude_public: bool = TruetoListAssetsQuery. Amode="before"validator normalizeshashwith.strip().lower()to match stored lowercaseblake3:<hex>values; malformed values are not rejected (the spec param is a plain string with no pattern), they simply yield an empty page.app/assets/services/asset_management.py+app/assets/database/queries/asset_reference.py— threadasset_hashthroughlist_assets_page→list_references_page, applyingAsset.hash == asset_hashto both the page and count statements (which already joinAsset) so page rows andtotalstay consistent.include_publicis intentionally not passed to the service layer — it is inert.Tests
Added integration tests in
tests-unit/assets_test/test_list_filter.pymirroring the existing http-session style:display_nameis emitted and mirrorsname.hashfilter returns only the exact content-hash match (total == 1).hashreturns an empty page (assets == [],total == 0, 200).include_public=falseandinclude_public=trueboth return 200 and still return the caller's own assets (parity, inert).No
openapi.yamlchange is needed — the spec already declares all three; this PR closes the implementation gap against it.is_immutable,file_path, andshort_urlare out of scope.Verification notes / judgment calls
hash, notasset_hash: the parent ticket prose saidasset_hash, but the projectedopenapi.yamlnames the query paramhash("Filter assets by exact content hash."). The spec is the acceptance source of truth, so the query param ishash. The response-body fieldasset_hashis left as-is.torchand Python ≥ 3.10. The build host here has only Python 3.9 and notorch, so the suite could not be executed locally — CI must exercise it. What was verified locally:ruff checkpasses on all changed files;py_compilepasses; the surface was diffed param-for-param againstopenapi.yaml; all existing callers of the two touched query functions pass arguments by keyword (so insertingasset_hashmid-signature is safe); andAsset.hashis an existing column already used elsewhere in the same query module.