kunwp1 commented on code in PR #3728:
URL: https://github.com/apache/texera/pull/3728#discussion_r2403531318
##########
core/gui/src/app/dashboard/service/user/download/download.service.ts:
##########
Review Comment:
Please remove `saveBlobFile` function as it is no longer used.
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/service/ResultExportService.scala:
##########
@@ -562,4 +594,20 @@ class ResultExportService(workflowIdentity:
WorkflowIdentity, computingUnitId: I
// remove path separators
StringUtils.replaceEach(rawName, Array("/", "\\"), Array("", ""))
}
+
+ /**
+ * Validate an export request by checking if any operators are selected.
+ * Return an error response if none are selected, otherwise None.
+ */
+ def validateExportRequest(request: ResultExportRequest): Option[Response] = {
Review Comment:
Consider inlining this function after removing duplicates (mentioned in the
comment above). I think the validation logic is too trivial to become a
function.
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -688,76 +691,53 @@ class WorkflowExecutionsResource {
}
@POST
- @Path("/result/export")
+ @Path("/result/export/dataset")
@RolesAllowed(Array("REGULAR", "ADMIN"))
- def exportResult(
- request: ResultExportRequest,
- @Auth user: SessionUser
- ): Response = {
-
- if (request.operators.size <= 0)
- Response
- .status(Response.Status.BAD_REQUEST)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "No operator selected").asJava)
- .build()
-
+ def exportResult(request: ResultExportRequest, @Auth user: SessionUser):
Response = {
Review Comment:
Better to change the name to `exportResultToDataset`
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -688,76 +691,53 @@ class WorkflowExecutionsResource {
}
@POST
- @Path("/result/export")
+ @Path("/result/export/dataset")
@RolesAllowed(Array("REGULAR", "ADMIN"))
- def exportResult(
- request: ResultExportRequest,
- @Auth user: SessionUser
- ): Response = {
-
- if (request.operators.size <= 0)
- Response
- .status(Response.Status.BAD_REQUEST)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "No operator selected").asJava)
- .build()
-
+ def exportResult(request: ResultExportRequest, @Auth user: SessionUser):
Response = {
try {
- request.destination match {
- case "local" =>
- // CASE A: multiple operators => produce ZIP
- if (request.operators.size > 1) {
- val resultExportService =
- new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
- val (zipStream, zipFileNameOpt) =
- resultExportService.exportOperatorsAsZip(request)
-
- if (zipStream == null) {
- throw new RuntimeException("Zip stream is null")
- }
+ val resultExportService =
+ new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
+ resultExportService.validateExportRequest(request) match {
+ case Some(errorResponse) => errorResponse
+ case None =>
resultExportService.exportToDataset(user.user, request)
+ }
+ } catch {
+ case ex: Exception =>
+ Response
+ .status(Response.Status.INTERNAL_SERVER_ERROR)
+ .`type`(MediaType.APPLICATION_JSON)
+ .entity(Map("error" -> ex.getMessage).asJava)
+ .build()
+ }
+ }
- val finalFileName = zipFileNameOpt.getOrElse("operators.zip")
- return Response
- .ok(zipStream, "application/zip")
- .header("Content-Disposition", "attachment; filename=\"" +
finalFileName + "\"")
- .build()
- }
+ @POST
+ @Path("/result/export/local")
+ @Consumes(Array(MediaType.APPLICATION_FORM_URLENCODED))
+ def exportResultViaBrowser(
Review Comment:
Rename it to `exportResultToLocal`. The browser native download is the
frontend's behavior
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/model/http/request/result/ResultExportRequest.scala:
##########
Review Comment:
Please remove `destination` from `ResultExportRequest` as it is no longer
used. You may also need to modify the corresponding frontend code that sets the
value of it.
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -688,76 +691,53 @@ class WorkflowExecutionsResource {
}
@POST
- @Path("/result/export")
+ @Path("/result/export/dataset")
@RolesAllowed(Array("REGULAR", "ADMIN"))
- def exportResult(
- request: ResultExportRequest,
- @Auth user: SessionUser
- ): Response = {
-
- if (request.operators.size <= 0)
- Response
- .status(Response.Status.BAD_REQUEST)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "No operator selected").asJava)
- .build()
-
+ def exportResult(request: ResultExportRequest, @Auth user: SessionUser):
Response = {
try {
- request.destination match {
- case "local" =>
- // CASE A: multiple operators => produce ZIP
- if (request.operators.size > 1) {
- val resultExportService =
- new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
- val (zipStream, zipFileNameOpt) =
- resultExportService.exportOperatorsAsZip(request)
-
- if (zipStream == null) {
- throw new RuntimeException("Zip stream is null")
- }
+ val resultExportService =
+ new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
+ resultExportService.validateExportRequest(request) match {
+ case Some(errorResponse) => errorResponse
+ case None =>
resultExportService.exportToDataset(user.user, request)
+ }
+ } catch {
+ case ex: Exception =>
+ Response
+ .status(Response.Status.INTERNAL_SERVER_ERROR)
+ .`type`(MediaType.APPLICATION_JSON)
+ .entity(Map("error" -> ex.getMessage).asJava)
+ .build()
+ }
+ }
- val finalFileName = zipFileNameOpt.getOrElse("operators.zip")
- return Response
- .ok(zipStream, "application/zip")
- .header("Content-Disposition", "attachment; filename=\"" +
finalFileName + "\"")
- .build()
- }
+ @POST
+ @Path("/result/export/local")
+ @Consumes(Array(MediaType.APPLICATION_FORM_URLENCODED))
+ def exportResultViaBrowser(
+ @FormParam("request") requestJson: String,
+ @FormParam("token") token: String
+ ): Response = {
- // CASE B: exactly one operator => single file
- if (request.operators.size != 1) {
- return Response
- .status(Response.Status.BAD_REQUEST)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "Local download does not support no
operator.").asJava)
- .build()
- }
- val singleOp = request.operators.head
-
- val resultExportService =
- new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
- val (streamingOutput, fileNameOpt) =
- resultExportService.exportOperatorResultAsStream(request, singleOp)
-
- if (streamingOutput == null) {
- return Response
- .status(Response.Status.INTERNAL_SERVER_ERROR)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "Failed to export operator").asJava)
- .build()
- }
+ try {
+ val userOpt = JwtParser.parseToken(token)
+ if (userOpt.isPresent) {
+ val user = userOpt.get()
+ val role = user.getUser.getRole
+ val RolesAllowed = Set(UserRoleEnum.REGULAR, UserRoleEnum.ADMIN)
Review Comment:
Just curious, how did you come up with this set? Did you refer to an
existing code or was it discussed?
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -688,76 +691,53 @@ class WorkflowExecutionsResource {
}
@POST
- @Path("/result/export")
+ @Path("/result/export/dataset")
@RolesAllowed(Array("REGULAR", "ADMIN"))
- def exportResult(
- request: ResultExportRequest,
- @Auth user: SessionUser
- ): Response = {
-
- if (request.operators.size <= 0)
- Response
- .status(Response.Status.BAD_REQUEST)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "No operator selected").asJava)
- .build()
-
+ def exportResult(request: ResultExportRequest, @Auth user: SessionUser):
Response = {
try {
- request.destination match {
- case "local" =>
- // CASE A: multiple operators => produce ZIP
- if (request.operators.size > 1) {
- val resultExportService =
- new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
- val (zipStream, zipFileNameOpt) =
- resultExportService.exportOperatorsAsZip(request)
-
- if (zipStream == null) {
- throw new RuntimeException("Zip stream is null")
- }
+ val resultExportService =
+ new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
+ resultExportService.validateExportRequest(request) match {
+ case Some(errorResponse) => errorResponse
+ case None =>
resultExportService.exportToDataset(user.user, request)
+ }
+ } catch {
+ case ex: Exception =>
+ Response
+ .status(Response.Status.INTERNAL_SERVER_ERROR)
+ .`type`(MediaType.APPLICATION_JSON)
+ .entity(Map("error" -> ex.getMessage).asJava)
+ .build()
+ }
+ }
- val finalFileName = zipFileNameOpt.getOrElse("operators.zip")
- return Response
- .ok(zipStream, "application/zip")
- .header("Content-Disposition", "attachment; filename=\"" +
finalFileName + "\"")
- .build()
- }
+ @POST
+ @Path("/result/export/local")
+ @Consumes(Array(MediaType.APPLICATION_FORM_URLENCODED))
+ def exportResultViaBrowser(
+ @FormParam("request") requestJson: String,
+ @FormParam("token") token: String
+ ): Response = {
- // CASE B: exactly one operator => single file
- if (request.operators.size != 1) {
- return Response
- .status(Response.Status.BAD_REQUEST)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "Local download does not support no
operator.").asJava)
- .build()
- }
- val singleOp = request.operators.head
-
- val resultExportService =
- new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
- val (streamingOutput, fileNameOpt) =
- resultExportService.exportOperatorResultAsStream(request, singleOp)
-
- if (streamingOutput == null) {
- return Response
- .status(Response.Status.INTERNAL_SERVER_ERROR)
- .`type`(MediaType.APPLICATION_JSON)
- .entity(Map("error" -> "Failed to export operator").asJava)
- .build()
- }
+ try {
+ val userOpt = JwtParser.parseToken(token)
+ if (userOpt.isPresent) {
+ val user = userOpt.get()
+ val role = user.getUser.getRole
+ val RolesAllowed = Set(UserRoleEnum.REGULAR, UserRoleEnum.ADMIN)
+ if (!RolesAllowed.contains(role)) {
+ throw new RuntimeException("User role is not allowed to perform this
download")
+ }
+ } else {
+ throw new RuntimeException("Invalid or expired token")
+ }
- val finalFileName = fileNameOpt.getOrElse("download.dat")
- Response
- .ok(streamingOutput, MediaType.APPLICATION_OCTET_STREAM)
- .header("Content-Disposition", "attachment; filename=\"" +
finalFileName + "\"")
- .build()
- case _ =>
- // destination = "dataset" by default
- val resultExportService =
- new ResultExportService(WorkflowIdentity(request.workflowId),
request.computingUnitId)
- val exportResponse =
- resultExportService.exportAllOperatorsResultToDataset(user.user,
request)
- Response.ok(exportResponse).build()
+ val request = Json.parse(requestJson).as[ResultExportRequest]
+ val resultExportService =
Review Comment:
I can see that the code from line 736 to 740 and line 698 to 703 are
duplicates and you can consider merging them into a single code.
--
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]
