Copilot commented on code in PR #5929:
URL: https://github.com/apache/texera/pull/5929#discussion_r3478144348
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1030,6 +1034,55 @@ class DatasetResource extends LazyLogging {
}
}
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/existing-upload-files")
+ @Consumes(Array(MediaType.APPLICATION_JSON))
+ def findExistingUploadFiles(
+ @PathParam("did") did: Integer,
+ request: ExistingUploadFilesRequest,
+ @Auth user: SessionUser
+ ): Response = {
+ val uid = user.getUid
+ withTransaction(context) { ctx =>
+ if (!userHasWriteAccess(ctx, did, uid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE)
+ }
+
+ val requested = Option(request)
+ .map(_.files)
+ .getOrElse(List.empty)
+ .map { file =>
Review Comment:
`Option(request).map(_.files)` can still yield `Some(null)` when the JSON
body omits `files` (e.g., `{}`), which then makes `.getOrElse(List.empty)`
return `null` and causes an NPE on the next `.map`. Prefer `flatMap` to guard
against `request.files == null` as well.
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1030,6 +1034,55 @@ class DatasetResource extends LazyLogging {
}
}
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/existing-upload-files")
+ @Consumes(Array(MediaType.APPLICATION_JSON))
+ def findExistingUploadFiles(
+ @PathParam("did") did: Integer,
+ request: ExistingUploadFilesRequest,
+ @Auth user: SessionUser
+ ): Response = {
+ val uid = user.getUid
+ withTransaction(context) { ctx =>
+ if (!userHasWriteAccess(ctx, did, uid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE)
+ }
+
+ val requested = Option(request)
+ .map(_.files)
+ .getOrElse(List.empty)
+ .map { file =>
+ val path = validateAndNormalizeFilePathOrThrow(file.path)
+ if (file.sizeBytes < 0L) throw new BadRequestException("sizeBytes
must be >= 0")
+ path -> file.sizeBytes
+ }
+ .toMap
+
+ val dataset = getDatasetByID(ctx, did)
+ val committed = getLatestDatasetVersion(ctx, did)
+ .map(v =>
+ LakeFSStorageClient
+ .retrieveObjectsOfVersion(dataset.getRepositoryName,
v.getVersionHash)
+ .map(obj => obj.getPath -> obj.getSizeBytes.longValue())
+ )
+ .getOrElse(List.empty)
+
+ val staged = LakeFSStorageClient
+ .retrieveUncommittedObjects(dataset.getRepositoryName)
+ .filterNot(diff =>
Option(diff.getType).exists(_.getValue.equalsIgnoreCase("removed")))
+ .flatMap(diff => Option(diff.getSizeBytes).map(size => diff.getPath ->
size.longValue()))
Review Comment:
Similarly, `retrieveUncommittedObjects` should be wrapped with
`withLakeFSErrorHandling` so LakeFS `ApiException`s become consistent JAX-RS
`NotFound/Forbidden/...` responses (instead of bubbling up as unhandled errors).
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -1030,6 +1034,55 @@ class DatasetResource extends LazyLogging {
}
}
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{did}/existing-upload-files")
+ @Consumes(Array(MediaType.APPLICATION_JSON))
+ def findExistingUploadFiles(
+ @PathParam("did") did: Integer,
+ request: ExistingUploadFilesRequest,
+ @Auth user: SessionUser
+ ): Response = {
+ val uid = user.getUid
+ withTransaction(context) { ctx =>
+ if (!userHasWriteAccess(ctx, did, uid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE)
+ }
+
+ val requested = Option(request)
+ .map(_.files)
+ .getOrElse(List.empty)
+ .map { file =>
+ val path = validateAndNormalizeFilePathOrThrow(file.path)
+ if (file.sizeBytes < 0L) throw new BadRequestException("sizeBytes
must be >= 0")
+ path -> file.sizeBytes
+ }
+ .toMap
+
+ val dataset = getDatasetByID(ctx, did)
+ val committed = getLatestDatasetVersion(ctx, did)
+ .map(v =>
+ LakeFSStorageClient
+ .retrieveObjectsOfVersion(dataset.getRepositoryName,
v.getVersionHash)
+ .map(obj => obj.getPath -> obj.getSizeBytes.longValue())
+ )
+ .getOrElse(List.empty)
Review Comment:
LakeFS SDK failures (e.g., 404 missing repo/branch) are normally converted
into appropriate HTTP exceptions via `withLakeFSErrorHandling`. This new
endpoint calls `retrieveObjectsOfVersion` directly, which can leak raw LakeFS
errors and break the existing NotFound-style behavior expected elsewhere in
this resource.
--
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]
