Copilot commented on code in PR #725:
URL:
https://github.com/apache/hugegraph-toolchain/pull/725#discussion_r3073802934
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/controller/load/FileUploadController.java:
##########
@@ -96,13 +96,14 @@ public FileUploadResult upload(@PathVariable("connId") int
connId,
@PathVariable("jobId") int jobId,
@RequestParam("file") MultipartFile file,
@RequestParam("name") String fileName,
+ @RequestParam("size") long fileSize,
@RequestParam("token") String token,
@RequestParam("total") int total,
@RequestParam("index") int index) {
Review Comment:
The new `size` request parameter is required, which makes this a breaking
API change (old clients will get a 400 missing parameter). Consider making it
optional (e.g., `required=false`) and falling back to a safe server-side value
(like `file.getSize()` for non-chunked uploads, or only enforcing `size` on
`index==0` for chunked uploads) to preserve backward compatibility while still
enabling correct quota validation.
##########
hugegraph-hubble/hubble-fe/src/utils/dataImportUpload.ts:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+const GB = 1024 * 1024 * 1024;
+
+type UploadedFile = {
+ name: string;
+ total_size_bytes: number;
+};
+
+type LocalUploadTask = {
+ name: string;
+ size: number;
+};
+
+export const getCurrentJobUploadSize = (
+ uploadedFiles: UploadedFile[],
+ localUploadTasks: LocalUploadTask[]
+) => {
+ const uploadedFileNames = uploadedFiles.map(({ name }) => name);
+ const uploadedFileSize = uploadedFiles.reduce(
+ (sum, { total_size_bytes }) => sum + total_size_bytes,
+ 0
+ );
+ const pendingFileSize = localUploadTasks
+ .filter(({ name }) => !uploadedFileNames.includes(name))
Review Comment:
`uploadedFileNames.includes(name)` makes the filter O(n*m) for local tasks
vs uploaded files. Using a `Set` for uploaded names would make lookups O(1) and
keeps the intent clearer (especially if these arrays can grow when many files
are queued).
```suggestion
const uploadedFileNames = new Set(
uploadedFiles.map(({ name }) => name)
);
const uploadedFileSize = uploadedFiles.reduce(
(sum, { total_size_bytes }) => sum + total_size_bytes,
0
);
const pendingFileSize = localUploadTasks
.filter(({ name }) => !uploadedFileNames.has(name))
```
##########
hugegraph-hubble/hubble-fe/src/utils/dataImportUpload.ts:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+const GB = 1024 * 1024 * 1024;
+
+type UploadedFile = {
+ name: string;
+ total_size_bytes: number;
+};
+
+type LocalUploadTask = {
+ name: string;
+ size: number;
+};
+
+export const getCurrentJobUploadSize = (
+ uploadedFiles: UploadedFile[],
+ localUploadTasks: LocalUploadTask[]
+) => {
+ const uploadedFileNames = uploadedFiles.map(({ name }) => name);
+ const uploadedFileSize = uploadedFiles.reduce(
+ (sum, { total_size_bytes }) => sum + total_size_bytes,
+ 0
+ );
+ const pendingFileSize = localUploadTasks
+ .filter(({ name }) => !uploadedFileNames.includes(name))
Review Comment:
`uploadedFileNames.includes(name)` makes the filter O(n*m) for local tasks
vs uploaded files. Using a `Set` for uploaded names would make lookups O(1) and
keeps the intent clearer (especially if these arrays can grow when many files
are queued).
```suggestion
const uploadedFileNames = new Set(
uploadedFiles.map(({ name }) => name)
);
const uploadedFileSize = uploadedFiles.reduce(
(sum, { total_size_bytes }) => sum + total_size_bytes,
0
);
const pendingFileSize = localUploadTasks
.filter(({ name }) => !uploadedFileNames.has(name))
```
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/controller/load/JobManagerController.java:
##########
@@ -104,10 +104,23 @@ public JobManager create(@PathVariable("connId") int
connId,
@DeleteMapping("{id}")
public void delete(@PathVariable("id") int id) {
- JobManager task = this.service.get(id);
- if (task == null) {
+ JobManager job = this.service.get(id);
+ if (job == null) {
throw new ExternalException("job.manager.not-exist.id", id);
}
+ List<LoadTask> loadTasks = this.taskService.taskListByJob(id);
+ for (LoadTask loadTask : loadTasks) {
+ if (loadTask.getStatus().inRunning() ||
+ loadTask.getStatus() == LoadStatus.PAUSED) {
+ this.taskService.stop(loadTask.getId());
+ }
+ this.taskService.remove(loadTask.getId());
+ }
+ List<FileMapping> mappings = this.fmService.listByJob(id);
+ for (FileMapping mapping : mappings) {
+ this.fmService.deleteDiskFile(mapping);
+ this.fmService.remove(mapping.getId());
+ }
this.service.remove(id);
}
Review Comment:
The delete endpoint now performs multi-step cascading cleanup (stop tasks,
delete DB records, delete disk files) directly in the controller. If any step
throws, the system can be left in a partially-deleted state (e.g., DB removed
but files left on disk, or vice versa). Consider moving this logic into a
dedicated service method and wrapping it in a transaction for DB operations
(and handling disk deletion failures explicitly, e.g., best-effort delete +
logging) to improve reliability and observability.
##########
hugegraph-hubble/hubble-fe/src/stores/GraphManagementStore/dataImportStore/dataImportRootStore.ts:
##########
@@ -230,6 +230,9 @@ export class DataImportRootStore {
this.requestStatus.uploadFiles = 'pending';
const formData = new FormData();
formData.append('file', fileChunkList.chunk);
+ const fileSize =
+ this.fileUploadTasks.find(({ name }) => name === fileName)?.size ??
+ fileChunkList.chunk.size;
Review Comment:
When the upload task entry can't be found, this falls back to
`fileChunkList.chunk.size` (chunk size), which can under-report the original
file size. Since the backend now uses `size` for quota enforcement, this
fallback can incorrectly allow uploads that should be rejected. Prefer passing
the original `File.size` consistently (e.g., ensure the per-file size is stored
alongside the hash and always available during chunk upload) and avoid using
per-chunk size as a substitute.
##########
hugegraph-hubble/hubble-fe/src/stores/GraphManagementStore/dataImportStore/dataImportRootStore.ts:
##########
@@ -238,7 +241,7 @@ export class DataImportRootStore {
try {
const result: AxiosResponse<responseData<FileUploadResult>> = yield axios
.post<responseData<FileUploadResult>>(
-
`${baseUrl}/${this.currentId}/job-manager/${this.currentJobId}/upload-file?total=${fileChunkTotal}&index=${fileChunkList.chunkIndex}&name=${fileName}&token=${this.fileHashes[fileName]}`,
+
`${baseUrl}/${this.currentId}/job-manager/${this.currentJobId}/upload-file?total=${fileChunkTotal}&index=${fileChunkList.chunkIndex}&name=${fileName}&size=${fileSize}&token=${this.fileHashes[fileName]}`,
Review Comment:
When the upload task entry can't be found, this falls back to
`fileChunkList.chunk.size` (chunk size), which can under-report the original
file size. Since the backend now uses `size` for quota enforcement, this
fallback can incorrectly allow uploads that should be rejected. Prefer passing
the original `File.size` consistently (e.g., ensure the per-file size is stored
alongside the hash and always available during chunk upload) and avoid using
per-chunk size as a substitute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
