Support bulk Context Center document operations#29380
Conversation
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Jest test CoverageUI tests summary
|
| private String zipEntryName(DownloadEntry entry, Set<String> usedNames) { | ||
| String sourceName = | ||
| entry.asset().getFileName() != null && !entry.asset().getFileName().isBlank() | ||
| ? entry.asset().getFileName() | ||
| : entry.file().getDisplayName() != null && !entry.file().getDisplayName().isBlank() | ||
| ? entry.file().getDisplayName() | ||
| : entry.file().getName(); | ||
| String safeName = sanitizeFileName(sourceName).replace('/', '_'); | ||
| String candidate = safeName; | ||
| int count = 2; | ||
| while (!usedNames.add(candidate)) { | ||
| candidate = zipEntryNameWithSuffix(safeName, count); | ||
| count++; | ||
| } | ||
| return candidate; |
There was a problem hiding this comment.
💡 Edge Case: Zip dedup is case-sensitive; collisions on case-insensitive FS
zipEntryName/usedNames (a HashSet<String>) deduplicates generated zip entry names case-sensitively. Two selected files whose sanitized names differ only in case (e.g. Report.txt and report.txt) are both accepted as distinct zip entries. On case-insensitive filesystems (Windows, default macOS) the extractor will treat them as the same path, and one file silently overwrites the other on extraction — the user loses content with no error.
Suggested fix: track a case-insensitive key in the dedup set (e.g. usedNames.add(candidate.toLowerCase(Locale.ROOT))) while still emitting the original-cased name as the entry, so disambiguation suffixes are applied for case-only collisions too.
Was this helpful? React with 👍 / 👎
| List<UUID> ids = validateBulkIds(request == null ? null : request.ids); | ||
| limits.enforceBulkSizeLimit(entityType, ids.size()); | ||
|
|
||
| AssetService assetService = AssetServiceFactory.getService(); | ||
| if (assetService == null) { | ||
| return Response.status(Response.Status.SERVICE_UNAVAILABLE) | ||
| .entity("{\"message\":\"Object storage is not configured\"}") | ||
| .type(MediaType.APPLICATION_JSON) | ||
| .build(); | ||
| } | ||
|
|
||
| List<ResolvedDownloadEntry> resolvedEntries = new ArrayList<>(); | ||
| for (UUID id : ids) { | ||
| ContextFile file = getInternal(uriInfo, securityContext, id, "", include); | ||
| Asset asset = resolveAsset(file); |
There was a problem hiding this comment.
💡 Edge Case: Bulk download stages every file to local temp disk with no size cap
bulkDownloadFiles reads each requested file fully into a temp file on the server before streaming the zip. The bulk-size limit (limits.enforceBulkSizeLimit) caps the number of items, but not the total bytes. A request for many large files can exhaust the server's temp filesystem before any response is streamed. The temp files are cleaned up in a finally, but the peak on-disk footprint is unbounded relative to total content size.
Consider streaming entries directly into the ZipOutputStream (accepting that a mid-stream read failure can't change the already-sent 200), or enforcing a cumulative byte budget while staging and aborting with 413/400 once exceeded.
Was this helpful? React with 👍 / 👎
| private Response buildBulkOperationResponse(BulkOperationResult result) { | ||
| if (result.getStatus() == ApiStatus.FAILURE) { | ||
| return Response.status(Response.Status.BAD_REQUEST).entity(result).build(); | ||
| } | ||
| return Response.ok(result).build(); | ||
| } |
There was a problem hiding this comment.
💡 Quality: All-failure bulk response returns HTTP 400 regardless of cause
buildBulkOperationResponse returns HTTP 400 whenever every item fails (ApiStatus.FAILURE), even when the per-item failures are 403 (authorization) or 404 (not found). The accurate per-item statuses are preserved in the response body, but clients/proxies keying off the HTTP status will misclassify authorization or not-found failures as bad-request. Consider deriving the envelope status from the dominant per-item status, or documenting that callers must inspect failedRequest[].status.
Was this helpful? React with 👍 / 👎
|
|
Code Review 👍 Approved with suggestions 5 resolved / 8 findingsAdds bulk delete, move, and zip-download endpoints with improved document listing and case-insensitive filename validation. Please address the potential zip deduplication collisions and the lack of size limits for server-side temp file staging during bulk downloads. 💡 Edge Case: Zip dedup is case-sensitive; collisions on case-insensitive FS📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/drive/ContextFileResource.java:818-832 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/drive/ContextFileResource.java:603-607
Suggested fix: track a case-insensitive key in the dedup set (e.g. 💡 Edge Case: Bulk download stages every file to local temp disk with no size cap
Consider streaming entries directly into the 💡 Quality: All-failure bulk response returns HTTP 400 regardless of cause📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/drive/ContextFileResource.java:756-761 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/drive/ContextFileResource.java:791-805
✅ 5 resolved✅ Edge Case: Bulk zip download streams HTTP 200 even when a file read fails
✅ Quality: Bulk download error toast may render an unreadable Blob body
✅ Bug: Duplicate-name check compares raw filename to sanitized stored name
✅ Bug: Duplicate-filename check is TOCTOU; no DB uniqueness guard
✅ Edge Case: Folder DELETE silently ignores
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
4 participants
Describe your changes:
Adds bulk delete, move, and zip download APIs for Context Center drive files, fixes current-content resolution for downloads, supports
orderByon document listing, and validates duplicate uploaded filenames case-insensitively per folder or root without usingdisplayName.The UI now calls the bulk APIs for selected document actions and defaults document listing to recent-first ordering.
Fixes 29489
Screen.Recording.2026-06-25.at.11.06.53.PM.mov
Type of change:
High-level design:
The backend keeps per-file authorization and per-row success/failure reporting for bulk delete and move, streams bulk downloads as a zip, and performs duplicate checks by joining
context_fileto the current asset filename.Existing single-file endpoints remain compatible, while list ordering only changes when
orderByis supplied.Tests:
Use cases covered
updatedAt.Unit tests
Backend integration tests
openmetadata-integration-tests/for new/changed API endpoints.openmetadata-integration-tests/src/test/java/org/openmetadata/it/drive/ContextFileIT.javaopenmetadata-integration-tests/src/test/java/org/openmetadata/it/drive/DriveFileUploadIT.javaIngestion integration tests
Playwright (UI) tests
Manual testing performed
mvn spotless:applyorganize-imports-cli,eslint --fix,prettier --writemvn -pl openmetadata-integration-tests -am -DskipTests -DskipITs packagegit diff --checklocalhost:9000was not reachable in this workspace.UI screen recording / screenshots:
Not attached. UI changes wire existing controls to bulk APIs; no recording was captured in this workspace.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Greptile Summary
This PR adds bulk delete, bulk move, and bulk ZIP-download endpoints for Context Center drive files, fixes asset resolution for downloads by adding a full content-version fallback, introduces
orderBysupport on document listing with proper cursor validation, and enforces case-insensitive duplicate filename checks per folder. The UI is updated to call the new bulk APIs instead of issuing one request per file./bulk/delete,/bulk/move,/bulk/download) are added with per-item authorization, structuredBulkOperationResultresponses for delete/move, and streamed ZIP output for download; temp files used during ZIP creation are cleaned up in all paths.headContentId→assetId→ full content-list scan sorted byisCurrenttheningestedAt, preventing silent 404s when head-content is stale.sanitizeEntityNameno longer appends a UUID suffix, relying instead on the newvalidateNoDuplicateFileNamecheck to enforce uniqueness by querying thecontext_filetable case-insensitively.Confidence Score: 3/5
Two unresolved defects from prior reviews remain in the changed code: the duplicate-name check and the DB insert are not atomic (a concurrent upload pair can both pass the check and collide at the unique index), and folder delete now always cascades recursively without any signal to callers that previously relied on the non-recursive safety guard.
The bulk APIs themselves are well-structured, the cursor-mismatch crash is fixed, and the revokeObjectURL timing issue is addressed. However, removing the UUID suffix from sanitizeEntityName while leaving a non-atomic duplicate check creates a real data-integrity gap on concurrent uploads. Separately, FolderResource silently changed the semantics of DELETE /folders/{id} from fail-if-non-empty to always-recursive, which breaks any caller that passed ?recursive=false or omitted the parameter to protect non-empty folders.
ContextFileResource.java (upload path, non-atomic duplicate check) and FolderResource.java (recursive-delete behavioral change) warrant the most scrutiny before merging.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant UI as Browser / UI participant API as ContextFileResource participant Repo as ContextFileRepository participant DB as Database participant S3 as AssetService (S3/MinIO) note over UI,S3: Bulk Delete UI->>API: "POST /bulk/delete {ids, hardDelete}" loop per id API->>API: delete(id) auth check inside API-->>DB: soft/hard delete end API-->>UI: BulkOperationResult (SUCCESS/PARTIAL_SUCCESS/FAILURE) note over UI,S3: Bulk Move UI->>API: "PUT /bulk/move {ids, folder}" loop per id API->>API: authorizer.authorize(EDIT_ALL) API->>Repo: moveContextFile(id, newFolder) Repo->>DB: validateNoDuplicateFileName Repo->>DB: UPDATE context_file / entity_relationship end API-->>UI: BulkOperationResult note over UI,S3: Bulk Download UI->>API: "POST /bulk/download {ids}" loop per id (resolve assets) API->>Repo: getInternal(id) + resolveAsset(file) Repo->>DB: getContentById / getAssetById end loop per id (download to temp) API->>S3: assetService.read(asset).join() S3-->>API: InputStream API->>API: write to temp file end API->>UI: stream ZipOutputStream (temp files to zip to cleanup)%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant UI as Browser / UI participant API as ContextFileResource participant Repo as ContextFileRepository participant DB as Database participant S3 as AssetService (S3/MinIO) note over UI,S3: Bulk Delete UI->>API: "POST /bulk/delete {ids, hardDelete}" loop per id API->>API: delete(id) auth check inside API-->>DB: soft/hard delete end API-->>UI: BulkOperationResult (SUCCESS/PARTIAL_SUCCESS/FAILURE) note over UI,S3: Bulk Move UI->>API: "PUT /bulk/move {ids, folder}" loop per id API->>API: authorizer.authorize(EDIT_ALL) API->>Repo: moveContextFile(id, newFolder) Repo->>DB: validateNoDuplicateFileName Repo->>DB: UPDATE context_file / entity_relationship end API-->>UI: BulkOperationResult note over UI,S3: Bulk Download UI->>API: "POST /bulk/download {ids}" loop per id (resolve assets) API->>Repo: getInternal(id) + resolveAsset(file) Repo->>DB: getContentById / getAssetById end loop per id (download to temp) API->>S3: assetService.read(asset).join() S3-->>API: InputStream API->>API: write to temp file end API->>UI: stream ZipOutputStream (temp files to zip to cleanup)Comments Outside Diff (1)
openmetadata-service/src/main/java/org/openmetadata/service/resources/drive/ContextFileResource.java, line 1030-1047 (link)The
usedNamesmap tracks thesafeName→ count but never checks whether a generated suffix name (e.g."foo (2).txt") already exists as its own entry. If the download batch contains both a file named"foo (2).txt"and two files named"foo.txt", the second"foo.txt"is also renamed to"foo (2).txt", silently creating a duplicate zip entry. Most extraction tools will only unpack one of them, causing the other file's content to be lost without any error to the user.Reviews (20): Last reviewed commit: "lint fix" | Re-trigger Greptile