Copilot commented on code in PR #12991:
URL: https://github.com/apache/gravitino/pull/12991#discussion_r3956618033
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -49,7 +51,7 @@ static RuntimeException toSchemaException(GlueException e,
String context) {
if (e instanceof InvalidInputException) {
return new IllegalArgumentException(context + ": " + e.getMessage(), e);
}
- return new RuntimeException("Glue error: " + context, e);
+ return new RuntimeException("Glue error: " + context + ": " +
awsErrorDetail(e), e);
Review Comment:
Appending the full AWS error message can leak potentially sensitive
identifiers (e.g., IAM principal ARNs, account IDs, resource ARNs) into
user-facing exceptions. If these exceptions can cross trust boundaries (API
responses, multi-tenant logs), consider redacting known-sensitive patterns
(ARNs/account IDs) or gating the inclusion of full AWS messages behind a
configuration/log-level so operators can opt in without widening exposure.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -18,10 +18,12 @@
*/
package org.apache.gravitino.catalog.glue;
+import org.apache.commons.lang3.StringUtils;
Review Comment:
This introduces a new dependency on `commons-lang3` for blank checks. If
this module doesn’t already rely on it (or if you want to keep dependencies
minimal), Java’s built-in checks (e.g., `s != null && !s.isBlank()`) can avoid
the extra dependency and keep the utility logic self-contained.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -69,6 +71,32 @@ static RuntimeException toTableException(GlueException e,
String context) {
if (e instanceof InvalidInputException) {
return new IllegalArgumentException(context + ": " + e.getMessage(), e);
}
- return new RuntimeException("Glue error: " + context, e);
+ return new RuntimeException("Glue error: " + context + ": " +
awsErrorDetail(e), e);
Review Comment:
Appending the full AWS error message can leak potentially sensitive
identifiers (e.g., IAM principal ARNs, account IDs, resource ARNs) into
user-facing exceptions. If these exceptions can cross trust boundaries (API
responses, multi-tenant logs), consider redacting known-sensitive patterns
(ARNs/account IDs) or gating the inclusion of full AWS messages behind a
configuration/log-level so operators can opt in without widening exposure.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -69,6 +71,32 @@ static RuntimeException toTableException(GlueException e,
String context) {
if (e instanceof InvalidInputException) {
return new IllegalArgumentException(context + ": " + e.getMessage(), e);
}
- return new RuntimeException("Glue error: " + context, e);
+ return new RuntimeException("Glue error: " + context + ": " +
awsErrorDetail(e), e);
+ }
+
+ /**
+ * Renders the AWS-side detail of a Glue exception. AWS names the failing
action and the resource
+ * there, which is what the caller needs to act on; the error code is
prefixed so the failure can
+ * be classified at a glance.
+ *
+ * @param e the Glue exception to describe
+ * @return the AWS error code and message, or a best-effort description when
they are unavailable
+ */
+ private static String awsErrorDetail(GlueException e) {
+ AwsErrorDetails details = e.awsErrorDetails();
+ if (details != null) {
+ String code = details.errorCode();
+ String message = details.errorMessage();
+ if (StringUtils.isNotBlank(code) && StringUtils.isNotBlank(message)) {
+ return code + ": " + message;
+ }
+ if (StringUtils.isNotBlank(message)) {
+ return message;
+ }
Review Comment:
The `awsErrorDetail` branch that returns `AwsErrorDetails.errorMessage()`
when `errorCode` is blank/non-set isn’t covered by the added tests. Add a unit
test where `awsErrorDetails` has a non-blank `errorMessage` and a blank/null
`errorCode`, and assert the converted message includes that AWS message.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -69,6 +71,32 @@ static RuntimeException toTableException(GlueException e,
String context) {
if (e instanceof InvalidInputException) {
return new IllegalArgumentException(context + ": " + e.getMessage(), e);
}
- return new RuntimeException("Glue error: " + context, e);
+ return new RuntimeException("Glue error: " + context + ": " +
awsErrorDetail(e), e);
+ }
+
+ /**
+ * Renders the AWS-side detail of a Glue exception. AWS names the failing
action and the resource
+ * there, which is what the caller needs to act on; the error code is
prefixed so the failure can
+ * be classified at a glance.
+ *
+ * @param e the Glue exception to describe
+ * @return the AWS error code and message, or a best-effort description when
they are unavailable
+ */
+ private static String awsErrorDetail(GlueException e) {
+ AwsErrorDetails details = e.awsErrorDetails();
+ if (details != null) {
+ String code = details.errorCode();
+ String message = details.errorMessage();
+ if (StringUtils.isNotBlank(code) && StringUtils.isNotBlank(message)) {
+ return code + ": " + message;
Review Comment:
The fallback message formatting can become hard to read due to repeated
colon separators (e.g., `Glue error: <context>: <code>: <message>`). Consider
using a clearer delimiter/structure (such as parentheses for the code, or a
single separator that doesn’t stack) so the resulting message is unambiguous
and consistent.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -49,7 +51,7 @@ static RuntimeException toSchemaException(GlueException e,
String context) {
if (e instanceof InvalidInputException) {
return new IllegalArgumentException(context + ": " + e.getMessage(), e);
}
- return new RuntimeException("Glue error: " + context, e);
+ return new RuntimeException("Glue error: " + context + ": " +
awsErrorDetail(e), e);
Review Comment:
The fallback message formatting can become hard to read due to repeated
colon separators (e.g., `Glue error: <context>: <code>: <message>`). Consider
using a clearer delimiter/structure (such as parentheses for the code, or a
single separator that doesn’t stack) so the resulting message is unambiguous
and consistent.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -69,6 +71,32 @@ static RuntimeException toTableException(GlueException e,
String context) {
if (e instanceof InvalidInputException) {
return new IllegalArgumentException(context + ": " + e.getMessage(), e);
}
- return new RuntimeException("Glue error: " + context, e);
+ return new RuntimeException("Glue error: " + context + ": " +
awsErrorDetail(e), e);
+ }
+
+ /**
+ * Renders the AWS-side detail of a Glue exception. AWS names the failing
action and the resource
+ * there, which is what the caller needs to act on; the error code is
prefixed so the failure can
+ * be classified at a glance.
+ *
+ * @param e the Glue exception to describe
+ * @return the AWS error code and message, or a best-effort description when
they are unavailable
+ */
+ private static String awsErrorDetail(GlueException e) {
+ AwsErrorDetails details = e.awsErrorDetails();
+ if (details != null) {
+ String code = details.errorCode();
+ String message = details.errorMessage();
+ if (StringUtils.isNotBlank(code) && StringUtils.isNotBlank(message)) {
+ return code + ": " + message;
+ }
+ if (StringUtils.isNotBlank(message)) {
+ return message;
+ }
+ if (StringUtils.isNotBlank(code)) {
Review Comment:
This introduces a new dependency on `commons-lang3` for blank checks. If
this module doesn’t already rely on it (or if you want to keep dependencies
minimal), Java’s built-in checks (e.g., `s != null && !s.isBlank()`) can avoid
the extra dependency and keep the utility logic self-contained.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -69,6 +71,32 @@ static RuntimeException toTableException(GlueException e,
String context) {
if (e instanceof InvalidInputException) {
return new IllegalArgumentException(context + ": " + e.getMessage(), e);
}
- return new RuntimeException("Glue error: " + context, e);
+ return new RuntimeException("Glue error: " + context + ": " +
awsErrorDetail(e), e);
+ }
+
+ /**
+ * Renders the AWS-side detail of a Glue exception. AWS names the failing
action and the resource
+ * there, which is what the caller needs to act on; the error code is
prefixed so the failure can
+ * be classified at a glance.
+ *
+ * @param e the Glue exception to describe
+ * @return the AWS error code and message, or a best-effort description when
they are unavailable
+ */
+ private static String awsErrorDetail(GlueException e) {
+ AwsErrorDetails details = e.awsErrorDetails();
+ if (details != null) {
+ String code = details.errorCode();
+ String message = details.errorMessage();
+ if (StringUtils.isNotBlank(code) && StringUtils.isNotBlank(message)) {
+ return code + ": " + message;
+ }
+ if (StringUtils.isNotBlank(message)) {
+ return message;
+ }
+ if (StringUtils.isNotBlank(code)) {
+ return code;
+ }
+ }
+ return StringUtils.isNotBlank(e.getMessage()) ? e.getMessage() :
e.getClass().getSimpleName();
Review Comment:
This introduces a new dependency on `commons-lang3` for blank checks. If
this module doesn’t already rely on it (or if you want to keep dependencies
minimal), Java’s built-in checks (e.g., `s != null && !s.isBlank()`) can avoid
the extra dependency and keep the utility logic self-contained.
--
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]
