refactor(api): make media image requests single-item#845
Merged
Conversation
📝 WalkthroughWalkthroughThis PR makes media.meta metadata-only (omits binary bytes, adds BlobSize), removes media.image batch support (single-image per request), enforces per-platform image size caps via capped blob/file reads, and adds metadata-only DB read APIs plus corresponding tests and mock updates. ChangesMedia API Refactor: Metadata-Only
🎯 3 (Moderate) | ⏱️ ~25 minutes Sequence DiagramsequenceDiagram
participant Client
participant API_Server
participant MediaDB
participant Filesystem
Client->>API_Server: POST media.image (single request)
API_Server->>MediaDB: Resolve media refs / GetMediaPropertyMetadata
alt blob-backed property
API_Server->>MediaDB: GetMediaBlobDataCapped(blobDBID, maxBytes)
MediaDB-->>API_Server: (data or ErrMediaBlobTooLarge)
else file-backed property
API_Server->>Filesystem: Read file (capped)
Filesystem-->>API_Server: (bytes or not found)
end
API_Server-->>Client: media.image response (base64 image or error)
Possibly Related PRs
Suggested Reviewers
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/methods/media_image_test.go (1)
226-283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd one test that hits the
params.Batchrejection path explicitly.These two tests validate the raw
itemsguard, but not the laterparams.Batchbranch. A dedicated case for that path would lock behavior for both rejection mechanisms.As per coding guidelines, “
**/*.go: Write tests for all new code — see TESTING.md and pkg/testing/README.md”.🤖 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 `@pkg/api/methods/media_image_test.go` around lines 226 - 283, Add a new unit test that exercises the params.Batch rejection path by calling HandleMediaImage with params that set "batch": true (e.g., env := makeMediaImageEnv(t, mockDB, json.RawMessage(`{"batch": true}`)) or include "batch": true alongside minimal fields), then assert an error is returned, that it is a *models.ClientError (use require.ErrorAs) and that err.Error() contains "batch requests are no longer supported"; follow the existing test patterns (create test helper mockDB via testhelpers.NewMockMediaDBI(), call HandleMediaImage(env), and call mockDB.AssertExpectations(t)) and name it like TestHandleMediaImage_BatchParamReturnsClientError to mirror other tests.
🧹 Nitpick comments (1)
pkg/api/methods/media_meta_test.go (1)
135-166: ⚡ Quick winAssert metadata DB method usage directly in these tests.
These cases still stub legacy
GetMediaProperties*/GetMediaTitleProperties*calls, so they can pass via mock fallback without provingHandleMediaMetais using the new metadata query methods. Please set expectations onGetMediaPropertyMetadata*/GetMediaTitlePropertyMetadata*in these paths.Suggested expectation pattern update
- mockDB.On("GetMediaProperties", mock.Anything, int64(3)). + mockDB.On("GetMediaPropertyMetadata", mock.Anything, int64(3)). Return([]database.MediaProperty{ {TypeTag: "property:image", ContentType: "image/png", BlobSize: int64(len(blobData))}, }, nil) - mockDB.On("GetMediaTitleProperties", mock.Anything, int64(30)). + mockDB.On("GetMediaTitlePropertyMetadata", mock.Anything, int64(30)). Return([]database.MediaProperty{}, nil)As per coding guidelines,
**/*.go: "Write tests for all new code — see TESTING.md and pkg/testing/README.md".Also applies to: 425-425, 444-444
🤖 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 `@pkg/api/methods/media_meta_test.go` around lines 135 - 166, The tests TestHandleMediaMeta_BinaryPropertyMetadataOnly (and the OversizedBinary variant) currently only stub legacy GetMediaProperties/GetMediaTitleProperties, allowing HandleMediaMeta to pass via fallback; update these tests to assert the new metadata queries are used by adding mock expectations for GetMediaPropertyMetadata (for media DBID 3) and GetMediaTitlePropertyMetadata (for title DBID 30) returning the same MediaProperty metadata (or empty slice where appropriate), and remove or tighten the legacy GetMediaProperties*/GetMediaTitleProperties* stubs so the test fails if HandleMediaMeta still calls the old methods; keep the existing checks on resp.Media.Properties, BlobSize, ContentType, and Extension unchanged.
🤖 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 `@pkg/api/methods/media_image.go`:
- Around line 324-360: Summary: Replace direct os filesystem calls in
loadMediaImageFile with an injected afero.Fs to make filesystem access testable.
Change the signature of loadMediaImageFile to accept an afero.Fs (e.g., fs
afero.Fs), replace os.Stat with fs.Stat, keep the errors.Is(err, os.ErrNotExist)
checks, and call a version of readMediaBinaryFile that accepts the same afero.Fs
(update readMediaBinaryFile signature and implementation to use
afero.ReadFile/Open). Ensure mediaImageReadError, deleteStaleMediaImageProperty,
and callers of loadMediaImageFile are updated to pass the fs through so all
filesystem access in this path uses the injected afero.Fs.
- Around line 299-305: GetMediaBlobDataCapped currently returns an empty byte
slice for both "too large" and "missing" blobs, so the handler wrongly maps
empty results to mediaImageBlobTooLargeError. Update the handler around the
GetMediaBlobDataCapped call to distinguish missing blobs from capped blobs: if
binary is empty, first check for the blob's existence (e.g., call a DB method
that can return ErrNotFound or a boolean for *prop.BlobDBID) or change
GetMediaBlobDataCapped to return a sentinel ErrBlobNotFound; when the blob is
missing, delete the stale media metadata (remove the row referencing
*prop.BlobDBID) and fall back to the alternate handling path instead of
returning mediaImageBlobTooLargeError, and only use mediaImageBlobTooLargeError
when the DB signals an explicit size cap condition.
In `@pkg/api/server_logging_test.go`:
- Around line 105-110: The test uses a sentinel string secretBlob but never
injects it into the fixture, so the NotContains(secretBlob) assertion is
vacuous; update the fixture so the sentinel is actually present (e.g., set
textValue to secretBlob or assign secretBlob to
models.MediaMetaPropertyItem.Text in the metaProp instance) so the redaction
check exercises per-item details (also apply the same change for the duplicate
fixture at lines referenced around the other occurrence).
In `@pkg/database/mediadb/sql_blobs_test.go`:
- Around line 166-195: Add a test that verifies GetMediaBlobDataCapped returns
(nil, "", nil) for an unknown blob DB id: create a new test function (e.g.,
TestGetMediaBlobDataCapped_NotFound) that uses setupScraperTestDB and
context.Background(), call mediaDB.GetMediaBlobDataCapped with a non-existent
dbid (e.g., 0 or a large unlikely id), and assert that err is nil, the returned
data is nil and the contentType is empty (use require.NoError(t, err),
assert.Nil(t, got), assert.Empty(t, contentType)); reference the existing test
file functions TestGetMediaBlobDataCapped_FoundWithinCap and
TestGetMediaBlobDataCapped_OverCapReturnsNil for structure and naming.
In `@pkg/database/mediadb/sql_scraper_test.go`:
- Around line 1053-1074: Add parallel table tests mirroring
TestGetMediaTitlePropertyMetadata_WithBlobOmitsBinary for the other query paths:
GetMediaPropertyMetadata, GetMediaPropertyMetadataByMediaDBIDs, and
GetMediaTitlePropertyMetadataByMediaTitleDBIDs. For each test use
setupScraperTestDB, UpsertMediaBlob to create a blob and
UpsertMediaTitleProperties/UpsertMediaProperties (as appropriate) to attach a
property with BlobDBID, then call the target getter and assert BlobDBID equals
the inserted id, BlobSize equals len(data), ContentType equals "image/png", and
Binary is nil; keep the same ctx/cleanup pattern and parallel t.Parallel()
setup. Ensure one test covers the grouped-query variants (ByMediaDBIDs and
ByMediaTitleDBIDs) by passing slices of IDs to those functions so grouped
scanning/composition is exercised.
---
Outside diff comments:
In `@pkg/api/methods/media_image_test.go`:
- Around line 226-283: Add a new unit test that exercises the params.Batch
rejection path by calling HandleMediaImage with params that set "batch": true
(e.g., env := makeMediaImageEnv(t, mockDB, json.RawMessage(`{"batch": true}`))
or include "batch": true alongside minimal fields), then assert an error is
returned, that it is a *models.ClientError (use require.ErrorAs) and that
err.Error() contains "batch requests are no longer supported"; follow the
existing test patterns (create test helper mockDB via
testhelpers.NewMockMediaDBI(), call HandleMediaImage(env), and call
mockDB.AssertExpectations(t)) and name it like
TestHandleMediaImage_BatchParamReturnsClientError to mirror other tests.
---
Nitpick comments:
In `@pkg/api/methods/media_meta_test.go`:
- Around line 135-166: The tests TestHandleMediaMeta_BinaryPropertyMetadataOnly
(and the OversizedBinary variant) currently only stub legacy
GetMediaProperties/GetMediaTitleProperties, allowing HandleMediaMeta to pass via
fallback; update these tests to assert the new metadata queries are used by
adding mock expectations for GetMediaPropertyMetadata (for media DBID 3) and
GetMediaTitlePropertyMetadata (for title DBID 30) returning the same
MediaProperty metadata (or empty slice where appropriate), and remove or tighten
the legacy GetMediaProperties*/GetMediaTitleProperties* stubs so the test fails
if HandleMediaMeta still calls the old methods; keep the existing checks on
resp.Media.Properties, BlobSize, ContentType, and Extension unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67e1147e-5e46-44ed-bd47-2b0d0a9de03c
📒 Files selected for processing (15)
docs/api/methods.mddocs/scraper.mdpkg/api/methods/media_image.gopkg/api/methods/media_image_test.gopkg/api/methods/media_meta.gopkg/api/methods/media_meta_test.gopkg/api/models/responses.gopkg/api/server.gopkg/api/server_logging_test.gopkg/database/database.gopkg/database/mediadb/sql_blobs.gopkg/database/mediadb/sql_blobs_test.gopkg/database/mediadb/sql_scraper.gopkg/database/mediadb/sql_scraper_test.gopkg/testing/helpers/db_mocks.go
💤 Files with no reviewable changes (1)
- pkg/api/server.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/api/methods/media_image.go (1)
300-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not treat all empty blobs as stale records.
len(binary) == 0conflates “blob exists but empty” with “blob missing”, and can delete valid metadata. Gate stale deletion on an explicit not-found signal (e.g., emptycontentTypefrom DB contract), not byte length alone.Suggested fix
- if len(binary) == 0 { + // Not found returns (nil, "", nil); only then mark stale. + if contentType == "" { deleteStaleMediaImageProperty(ctx, db, row, prop, src.isMedia, typeTag) return nil, true, nil }🤖 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 `@pkg/api/methods/media_image.go` around lines 300 - 303, The code currently treats len(binary) == 0 as a "missing blob" and calls deleteStaleMediaImageProperty, which can delete valid metadata for an existing-but-empty blob; instead gate stale-deletion on an explicit not-found signal from the DB contract (e.g., check the contentType field), not byte length. Modify the conditional around deleteStaleMediaImageProperty in the function handling media image rows to use the DB-provided not-found indicator (for example check row.ContentType == "" or whatever explicit field the DB uses) and only call deleteStaleMediaImageProperty(row, prop, src.isMedia, typeTag) when that explicit not-found flag is present; leave cases where binary is empty but contentType indicates existence untouched and return as before.
🤖 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.
Duplicate comments:
In `@pkg/api/methods/media_image.go`:
- Around line 300-303: The code currently treats len(binary) == 0 as a "missing
blob" and calls deleteStaleMediaImageProperty, which can delete valid metadata
for an existing-but-empty blob; instead gate stale-deletion on an explicit
not-found signal from the DB contract (e.g., check the contentType field), not
byte length. Modify the conditional around deleteStaleMediaImageProperty in the
function handling media image rows to use the DB-provided not-found indicator
(for example check row.ContentType == "" or whatever explicit field the DB uses)
and only call deleteStaleMediaImageProperty(row, prop, src.isMedia, typeTag)
when that explicit not-found flag is present; leave cases where binary is empty
but contentType indicates existence untouched and return as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfd024d8-fb54-4e01-8f1d-dbc1f6f718de
📒 Files selected for processing (10)
docs/api/methods.mddocs/scraper.mdpkg/api/methods/media_image.gopkg/api/methods/media_image_test.gopkg/api/methods/media_meta_test.gopkg/api/server_logging_test.gopkg/database/database.gopkg/database/mediadb/sql_blobs.gopkg/database/mediadb/sql_blobs_test.gopkg/database/mediadb/sql_scraper_test.go
💤 Files with no reviewable changes (1)
- docs/api/methods.md
✅ Files skipped from review due to trivial changes (1)
- docs/scraper.md
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
1 participant
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
media.imageresponses and reject image batch requestsmedia.metapropertiesBreaking changes
media.imageno longer acceptsitems; clients must request one image per callmedia.meta.properties.*.datais removed; image bytes must come frommedia.imageSummary by CodeRabbit
New Features
Documentation