fix: return existing resource on duplicate upload - #320
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIn ChangesResource Upload Duplicate Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceServiceImpl
participant Database
Client->>ResourceServiceImpl: resourceUpload(hash, appId, resourceData)
ResourceServiceImpl->>Database: query Resource by hash, app_id, tenant_id
alt existing resource found
Database-->>ResourceServiceImpl: existing resourceResult
ResourceServiceImpl-->>Client: return existing resourceResult
else no duplicate
Database-->>ResourceServiceImpl: no match
ResourceServiceImpl->>ResourceServiceImpl: set tenantId, resourceUrl, thumbnailUrl, thumbnailData
ResourceServiceImpl-->>Client: return new resourceResult
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java (2)
178-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd/confirm test coverage for repeated-hash uploads.
No test changes are included in this PR context. Since the fix's whole purpose is "allow repeated uploads by hash," a regression test asserting two uploads with the same
hashboth succeed (and previously would have failed with CM003) would lock in this behavior.Want me to draft a unit test for
resourceUploadcovering this case?🤖 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 `@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java` around lines 178 - 210, Add or update a regression test for ResourceServiceImpl.resourceUpload that verifies two uploads with the same hash both succeed and no CM003 is thrown; the test should exercise the repeated-upload path in resourceUpload and assert the second call is accepted rather than rejected by the hash check. Use the resourceUpload method and the baseMapper.createResource/queryResourceById flow to locate the behavior under test, and cover the case where the same hash is uploaded twice.
190-202: 🧹 Nitpick | 🔵 TrivialUnbounded storage growth from duplicate uploads.
Without a dedup check, each repeat upload of the same image persists another full
resourceData/thumbnailDatabase64 blob row. This is the intended tradeoff per the PR, but worth flagging for operational awareness (storage/cost growth, no cleanup path for orphaned duplicates).🤖 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 `@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java` around lines 190 - 202, The ResourceServiceImpl upload flow currently always persists a new resourceData and thumbnailData blob for repeat image uploads, causing unbounded duplicate storage growth. In the code around the resourceUrl/thumbnailUrl setup in ResourceServiceImpl, add a deduplication check before saving or reusing the existing record so repeated uploads of the same content do not create another full base64 row. Use the existing resource/resourceData handling and the thumbnail generation path to locate the save logic, and make sure duplicate detection is applied before calling createThumbnail or persisting the new data.
🤖 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
`@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java`:
- Around line 178-210: Add or update a regression test for
ResourceServiceImpl.resourceUpload that verifies two uploads with the same hash
both succeed and no CM003 is thrown; the test should exercise the
repeated-upload path in resourceUpload and assert the second call is accepted
rather than rejected by the hash check. Use the resourceUpload method and the
baseMapper.createResource/queryResourceById flow to locate the behavior under
test, and cover the case where the same hash is uploaded twice.
- Around line 190-202: The ResourceServiceImpl upload flow currently always
persists a new resourceData and thumbnailData blob for repeat image uploads,
causing unbounded duplicate storage growth. In the code around the
resourceUrl/thumbnailUrl setup in ResourceServiceImpl, add a deduplication check
before saving or reusing the existing record so repeated uploads of the same
content do not create another full base64 row. Use the existing
resource/resourceData handling and the thumbnail generation path to locate the
save logic, and make sure duplicate detection is applied before calling
createThumbnail or persisting the new data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84636700-93bb-43ba-b24b-3292a5b54e03
📒 Files selected for processing (1)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java (1)
203-216: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftTOCTOU on hash dedupe can still create duplicate resources.
selectOnefollowed bycreateResourceis still a check-then-act race, andt_resourcehas no unique constraint onhash(onlycategory,name,tenant_id), so concurrent uploads of the same file can insert duplicate rows. Add a unique constraint onhash(or the tenant-scoped equivalent) and handle duplicate-key conflicts in the insert path.🤖 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 `@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java` around lines 203 - 216, The hash-based deduplication in ResourceServiceImpl’s create/read flow is still vulnerable to concurrent inserts because it checks with selectOne before calling createResource. Add a database-level unique constraint on hash (or the correct tenant-scoped hash key) and update the create path to catch and translate duplicate-key failures around baseMapper.createResource so concurrent uploads return the existing resource instead of creating duplicates.
🧹 Nitpick comments (1)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java (1)
198-209: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove duplicate-hash check before thumbnail generation.
The hash lookup (Lines 203-209) runs after Line 201 already generates the thumbnail via
ImageThumbnailGenerator.createThumbnail(...). Since this PR's goal is to let repeated uploads of the same image short-circuit to the existing resource, that expensive image processing work is wasted on every duplicate re-upload — the exact case this change is meant to optimize.♻️ Proposed reorder
- if (!StringUtils.isEmpty(resourceData)) { - resource.setResourceUrl(resourceUrl); - resource.setThumbnailUrl(thumbnailUrl); - resource.setThumbnailData(ImageThumbnailGenerator.createThumbnail(resource.getResourceData(), 200, 200)); - } QueryWrapper<Resource> queryWrapper = new QueryWrapper<>(); queryWrapper.eq("hash", resource.getHash()); // 接入租户系统需添加租户id查询 Resource resourceResult = this.baseMapper.selectOne(queryWrapper); if (resourceResult != null) { return resourceResult; } + if (!StringUtils.isEmpty(resourceData)) { + resource.setResourceUrl(resourceUrl); + resource.setThumbnailUrl(thumbnailUrl); + resource.setThumbnailData(ImageThumbnailGenerator.createThumbnail(resource.getResourceData(), 200, 200)); + } int createResult = this.baseMapper.createResource(resource);🤖 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 `@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java` around lines 198 - 209, The duplicate-hash lookup in ResourceServiceImpl should run before any thumbnail generation so repeated uploads can short-circuit early. Move the QueryWrapper/selectOne hash check ahead of the ImageThumbnailGenerator.createThumbnail call in the resource handling flow, and only set resourceUrl/thumbnailUrl/thumbnailData after confirming no existing Resource is returned. Use the ResourceServiceImpl resource processing block and the createThumbnail/hash lookup logic to locate the reorder point.
🤖 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.
Outside diff comments:
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java`:
- Around line 203-216: The hash-based deduplication in ResourceServiceImpl’s
create/read flow is still vulnerable to concurrent inserts because it checks
with selectOne before calling createResource. Add a database-level unique
constraint on hash (or the correct tenant-scoped hash key) and update the create
path to catch and translate duplicate-key failures around
baseMapper.createResource so concurrent uploads return the existing resource
instead of creating duplicates.
---
Nitpick comments:
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java`:
- Around line 198-209: The duplicate-hash lookup in ResourceServiceImpl should
run before any thumbnail generation so repeated uploads can short-circuit early.
Move the QueryWrapper/selectOne hash check ahead of the
ImageThumbnailGenerator.createThumbnail call in the resource handling flow, and
only set resourceUrl/thumbnailUrl/thumbnailData after confirming no existing
Resource is returned. Use the ResourceServiceImpl resource processing block and
the createThumbnail/hash lookup logic to locate the reorder point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ab8dfe3-4fd0-4778-94f5-bdc5352a6f1b
📒 Files selected for processing (1)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java (1)
200-218: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard the resource dedup path at the database layer.
selectOne+createResourcestill has a race: two concurrent uploads with the same(hash, app_id, tenant_id)can both miss the existing row and insert duplicates. Add a unique constraint on those columns and catch duplicate-key errors by re-querying the existing resource.🤖 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 `@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java` around lines 200 - 218, The dedup flow in ResourceServiceImpl’s create/query path still has a race because selectOne can miss a concurrent insert; add a unique constraint on the Resource table for the hash, app_id, and tenant_id columns, then update the createResource logic to catch duplicate-key exceptions and re-run the existing QueryWrapper-based lookup (the same one used before selectOne) to return the already-created Resource instead of inserting a duplicate.
🧹 Nitpick comments (2)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java (2)
206-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment contradicts the code above it.
Line 207's comment says tenant_id query still needs to be added ("接入租户系统需添加租户id查询"), but line 206 already added
queryWrapper.eq("tenant_id", ...). Remove or update the comment to avoid confusion.🤖 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 `@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java` around lines 206 - 207, The comment in ResourceServiceImpl around the queryWrapper tenant filter is stale and contradicts the existing queryWrapper.eq("tenant_id", loginUserContext.getTenantId()) call. Remove that comment or rewrite it so it accurately reflects the current tenant_id filtering logic in the surrounding code.
201-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse parameterized logging and lower verbosity level.
log.info("hash: " + resource.getHash())etc. use string concatenation instead of SLF4J placeholders, and log atinfofor what is effectively per-request debug detail (hash, appId, tenantId, full result object). Considerlog.debugwith{}placeholders to avoid unconditional string building and log noise in production.♻️ Proposed fix
- log.info("hash: " + resource.getHash()); - log.info("app id : " + resource.getAppId()); - log.info("tenantId : " + loginUserContext.getTenantId()); + log.debug("hash: {}, appId: {}, tenantId: {}", resource.getHash(), resource.getAppId(), loginUserContext.getTenantId()); queryWrapper.eq("hash", resource.getHash()); queryWrapper.eq("app_id", resource.getAppId()); queryWrapper.eq("tenant_id", loginUserContext.getTenantId()); // 接入租户系统需添加租户id查询 Resource resourceResult = this.baseMapper.selectOne(queryWrapper); - log.info("resourceResult: " + resourceResult); + log.debug("resourceResult: {}", resourceResult);🤖 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 `@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java` around lines 201 - 209, The logging in ResourceServiceImpl is too verbose and uses string concatenation instead of SLF4J placeholders. Update the per-request logs around the query in the ResourceServiceImpl method to use parameterized logging with {} and lower the level from info to debug for hash, appId, tenantId, and resourceResult. Keep the query logic unchanged, and apply the same logging style consistently wherever these values are emitted.
🤖 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.
Outside diff comments:
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java`:
- Around line 200-218: The dedup flow in ResourceServiceImpl’s create/query path
still has a race because selectOne can miss a concurrent insert; add a unique
constraint on the Resource table for the hash, app_id, and tenant_id columns,
then update the createResource logic to catch duplicate-key exceptions and
re-run the existing QueryWrapper-based lookup (the same one used before
selectOne) to return the already-created Resource instead of inserting a
duplicate.
---
Nitpick comments:
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java`:
- Around line 206-207: The comment in ResourceServiceImpl around the
queryWrapper tenant filter is stale and contradicts the existing
queryWrapper.eq("tenant_id", loginUserContext.getTenantId()) call. Remove that
comment or rewrite it so it accurately reflects the current tenant_id filtering
logic in the surrounding code.
- Around line 201-209: The logging in ResourceServiceImpl is too verbose and
uses string concatenation instead of SLF4J placeholders. Update the per-request
logs around the query in the ResourceServiceImpl method to use parameterized
logging with {} and lower the level from info to debug for hash, appId,
tenantId, and resourceResult. Keep the query logic unchanged, and apply the same
logging style consistently wherever these values are emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e52b9aa2-ef01-4dd6-8392-3e62f3efb977
📒 Files selected for processing (1)
base/src/main/java/com/tinyengine/it/service/material/impl/ResourceServiceImpl.java
msslulu
changed the title
chilingling
changed the title
2 participants
重复上传图片返回已存在的图片url
Summary by CodeRabbit