carloea2 commented on code in PR #4177:
URL: https://github.com/apache/texera/pull/4177#discussion_r2725032491
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -175,6 +176,47 @@ object DatasetResource {
normalized
}
+ /**
+ * Converts LakeFS ApiException to appropriate HTTP exception
+ */
+ private def handleLakeFSException(e: io.lakefs.clients.sdk.ApiException):
Nothing = {
+ val rawBody =
Option(e.getResponseBody).filter(_.nonEmpty).getOrElse(e.getMessage)
+
+ val message =
+ Try(new
ObjectMapper().readTree(rawBody).get("message").asText()).getOrElse(rawBody)
Review Comment:
I see we are creating a new ObjectMapper everytime, can we avoid it and
reuse it somehow to reduce the tiny overhead?
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -384,11 +428,13 @@ class DatasetResource {
}
// Create a commit in LakeFS
- val commit = LakeFSStorageClient.createCommit(
- repoName = repositoryName,
- branch = "main",
- commitMessage = s"Created dataset version: $newVersionName"
- )
+ val commit = withLakeFSErrorHandling {
+ LakeFSStorageClient.createCommit(
+ repoName = repositoryName,
+ branch = "main",
+ commitMessage = s"Created dataset version: $newVersionName"
+ )
+ }
Review Comment:
@xuang7 There is another approach with scala implicit, it will look like the
next code snippet, in your preference, is this cleaner?
```
val commit =
LakeFSStorageClient.safeCall(_.createCommit(
repoName = repositoryName,
branch = "main",
commitMessage = s"Created dataset version: $newVersionName"
))
```
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -175,6 +176,47 @@ object DatasetResource {
normalized
}
+ /**
+ * Converts LakeFS ApiException to appropriate HTTP exception
+ */
+ private def handleLakeFSException(e: io.lakefs.clients.sdk.ApiException):
Nothing = {
+ val rawBody =
Option(e.getResponseBody).filter(_.nonEmpty).getOrElse(e.getMessage)
Review Comment:
Is there any security issue if we return `e.getMessage`?
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -175,6 +176,47 @@ object DatasetResource {
normalized
}
+ /**
+ * Converts LakeFS ApiException to appropriate HTTP exception
+ */
+ private def handleLakeFSException(e: io.lakefs.clients.sdk.ApiException):
Nothing = {
+ val rawBody =
Option(e.getResponseBody).filter(_.nonEmpty).getOrElse(e.getMessage)
+
+ val message =
+ Try(new
ObjectMapper().readTree(rawBody).get("message").asText()).getOrElse(rawBody)
+
+ def errorResponse(status: Int): Response =
+ Response
+ .status(status)
+ .entity(Map("message" -> message).asJava)
+ .`type`(MediaType.APPLICATION_JSON)
+ .build()
+
+ throw (e.getCode match {
+ case 400 => new BadRequestException(errorResponse(400))
+ case 401 => new NotAuthorizedException(errorResponse(401))
+ case 403 => new ForbiddenException(errorResponse(403))
+ case 404 => new NotFoundException(errorResponse(404))
+ case 409 => new WebApplicationException(errorResponse(409))
+ case 410 => new WebApplicationException(errorResponse(410))
+ case 412 => new WebApplicationException(errorResponse(412))
+ case 416 => new WebApplicationException(errorResponse(416))
+ case 420 => new WebApplicationException(errorResponse(420))
+ case _ => new InternalServerErrorException(errorResponse(500))
+ })
Review Comment:
Can the codes be extracted dynamically from code (Maybe from LakeFs) so for
future changes from their side there is no need to maintain this piece of code?
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -175,6 +176,47 @@ object DatasetResource {
normalized
}
+ /**
+ * Converts LakeFS ApiException to appropriate HTTP exception
+ */
+ private def handleLakeFSException(e: io.lakefs.clients.sdk.ApiException):
Nothing = {
+ val rawBody =
Option(e.getResponseBody).filter(_.nonEmpty).getOrElse(e.getMessage)
+
+ val message =
+ Try(new
ObjectMapper().readTree(rawBody).get("message").asText()).getOrElse(rawBody)
Review Comment:
Instead of `.get("message")`, can we use `.path("message")` or similar if
available in case the rawBody has nested structure? Or LakeFS exceptions always
use the same structure?
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -175,6 +176,47 @@ object DatasetResource {
normalized
}
+ /**
+ * Converts LakeFS ApiException to appropriate HTTP exception
+ */
+ private def handleLakeFSException(e: io.lakefs.clients.sdk.ApiException):
Nothing = {
+ val rawBody =
Option(e.getResponseBody).filter(_.nonEmpty).getOrElse(e.getMessage)
+
+ val message =
+ Try(new
ObjectMapper().readTree(rawBody).get("message").asText()).getOrElse(rawBody)
+
+ def errorResponse(status: Int): Response =
+ Response
+ .status(status)
+ .entity(Map("message" -> message).asJava)
+ .`type`(MediaType.APPLICATION_JSON)
+ .build()
+
+ throw (e.getCode match {
+ case 400 => new BadRequestException(errorResponse(400))
+ case 401 => new NotAuthorizedException(errorResponse(401))
+ case 403 => new ForbiddenException(errorResponse(403))
+ case 404 => new NotFoundException(errorResponse(404))
+ case 409 => new WebApplicationException(errorResponse(409))
+ case 410 => new WebApplicationException(errorResponse(410))
+ case 412 => new WebApplicationException(errorResponse(412))
+ case 416 => new WebApplicationException(errorResponse(416))
+ case 420 => new WebApplicationException(errorResponse(420))
+ case _ => new InternalServerErrorException(errorResponse(500))
+ })
+ }
+
+ /**
+ * Wraps a LakeFS call with centralized error handling.
+ */
+ private def withLakeFSErrorHandling[T](lakeFsCall: => T): T = {
+ try {
+ lakeFsCall
+ } catch {
+ case e: io.lakefs.clients.sdk.ApiException => handleLakeFSException(e)
+ }
+ }
+
Review Comment:
It will be good to detach this from DatasetResource so other sites can use
it, but still near LakeFSStorageClient
--
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]
