jerryshao commented on code in PR #13787:
URL: https://github.com/apache/gravitino/pull/13787#discussion_r4248217079
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -451,6 +483,23 @@ public Function getFunction(String schemaName, String
functionName) {
if (!supportsFunctions()) {
throw new UnsupportedOperationException("Catalog does not support
functions");
}
- return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
+ try {
+ return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
+ } catch (ForbiddenException | IllegalArgumentException e) {
+ throw toTrinoException(e);
+ }
+ }
+
+ /**
+ * Converts a refusal reported by the Gravitino server into a Trino
exception that classifies it
+ * as a user-facing error instead of an internal one.
+ */
+ private static TrinoException toTrinoException(RuntimeException e) {
+ String message = GravitinoErrorCode.toSimpleErrorMessage(e);
Review Comment:
[Important] `toSimpleErrorMessage` garbles the very message this PR sets out
to preserve.
For a refusal that came over REST, the client builds the exception message
as "message + newline + server stack trace": `ErrorHandlers.formatErrorMessage`
(`clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:328-335`)
appends the stack lines carried by `ErrorResponse.forbidden`
(`common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java:367-371`,
built by `getStackTrace` at `:388-398`), and the server always passes the
throwable
(`server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java:321-323`).
`GravitinoErrorCode.toSimpleErrorMessage` (`GravitinoErrorCode.java:104-107`)
then returns `lines.get(0) + lines.get(1)` with no separator, and line 1 of a
`printStackTrace` dump is `org.apache.gravitino.exceptions.ForbiddenException:
<the same message>`.
So the PERMISSION_DENIED a user sees in production is roughly:
User joel is not authorized to perform operation loadTable on metadata
m.c.s.torg.apache.gravitino.exceptions.ForbiddenException: User joel is not
authorized to perform operation loadTable on metadata m.c.s.t
The two-line concatenation exists only for the murky
`IllegalArgumentException` case (see the TODO now at lines 302-304); for
`ForbiddenException` the server message is already complete and self-contained.
Take the first line for these mapped refusals, e.g.
`e.getMessage().lines().findFirst().orElse(e.toString())`, and pin it with a
test whose message contains a newline plus a stack line.
Same helper, second hazard: it dereferences `e.getMessage()`, so a null
message throws NPE inside the catch and turns the refusal back into
GENERIC_INTERNAL_ERROR - the exact symptom of #13782.
`IllegalNameIdentifierException` (an `IllegalArgumentException` subclass,
`api/src/main/java/org/apache/gravitino/exceptions/IllegalNameIdentifierException.java:25`)
has a no-arg constructor at `:61`, so a null guard here is cheap insurance
even though I found no caller using it today.
Verified by: reading ErrorHandlers.java:313-336 and :397-446,
ErrorResponse.java:367-398, ExceptionHandlers.java:312-323 and
GravitinoErrorCode.java:104-107 in this checkout.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -268,6 +296,8 @@ private void applyAlter(SchemaTableName tableName,
TableChange... change) {
} catch (NoSuchTableException e) {
throw new TrinoException(
GravitinoErrorCode.GRAVITINO_TABLE_NOT_EXISTS, "Table does not
exist");
+ } catch (ForbiddenException e) {
+ throw toTrinoException(e);
Review Comment:
[Nit] Now that the helper exists, the `IllegalArgumentException` branch
immediately below (lines 301-306) is a verbatim copy of `toTrinoException`'s
non-Forbidden path, so these two catches can collapse into the single `catch
(ForbiddenException | IllegalArgumentException e)` form used in the other
twelve call sites of this file. Keep the TODO about unclear server messages -
it is still true; just move it onto the merged catch or next to the helper. No
behavior change either way, purely the one spot in the diff where the old and
new styles sit side by side.
Verified by: comparing lines 296-307 with `toTrinoException` at lines
497-504 in this checkout.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -451,6 +483,23 @@ public Function getFunction(String schemaName, String
functionName) {
if (!supportsFunctions()) {
throw new UnsupportedOperationException("Catalog does not support
functions");
}
- return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
+ try {
+ return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
+ } catch (ForbiddenException | IllegalArgumentException e) {
+ throw toTrinoException(e);
+ }
+ }
+
+ /**
+ * Converts a refusal reported by the Gravitino server into a Trino
exception that classifies it
+ * as a user-facing error instead of an internal one.
+ */
+ private static TrinoException toTrinoException(RuntimeException e) {
+ String message = GravitinoErrorCode.toSimpleErrorMessage(e);
+ if (e instanceof ForbiddenException) {
+ return new TrinoException(PERMISSION_DENIED, message, e);
Review Comment:
[Question] Were the NOT_IN_USE refusals left out on purpose?
The same REST handlers that produce the `ForbiddenException` this branch
maps also produce `CatalogNotInUseException` / `MetalakeNotInUseException` /
`NotInUseException` for `ErrorConstants.NOT_IN_USE_CODE`
(`clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:429-439`
for tables, and the same block at `:537` and `:607` for schemas and catalogs).
Those are `GravitinoRuntimeException` subclasses, nothing in this class catches
them, so querying a disabled catalog or metalake still reaches the user as
GENERIC_INTERNAL_ERROR - the same "expected refusal looks like a server fault"
symptom #13782 is about.
Is that a deliberate follow-up, or an oversight? If it belongs here, a third
branch in this helper would cover it; if not, a line in the PR description
saying so would keep the next reader from re-deriving it.
Verified by: reading ErrorHandlers.java:397-446 and every catch clause in
CatalogConnectorMetadata.java in this checkout.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -451,6 +483,23 @@ public Function getFunction(String schemaName, String
functionName) {
if (!supportsFunctions()) {
throw new UnsupportedOperationException("Catalog does not support
functions");
}
- return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
+ try {
+ return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
+ } catch (ForbiddenException | IllegalArgumentException e) {
+ throw toTrinoException(e);
+ }
+ }
+
+ /**
+ * Converts a refusal reported by the Gravitino server into a Trino
exception that classifies it
+ * as a user-facing error instead of an internal one.
+ */
+ private static TrinoException toTrinoException(RuntimeException e) {
+ String message = GravitinoErrorCode.toSimpleErrorMessage(e);
+ if (e instanceof ForbiddenException) {
+ return new TrinoException(PERMISSION_DENIED, message, e);
+ }
+ // Server-side validation failures, such as an illegal object name or a
policy refusal.
+ return new TrinoException(GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT,
message, e);
Review Comment:
[Important] `GRAVITINO_ILLEGAL_ARGUMENT` is not a user error in Trino's
classification, so this half of the fix does not reach the PR's stated goal.
`GravitinoErrorCode` declares every one of its codes with
`ErrorType.EXTERNAL` (`GravitinoErrorCode.java:21` for the static import, `:63`
for this code). EXTERNAL tells Trino the failure came from an external system,
not from the user. The Forbidden branch two lines up uses
`StandardErrorCode.PERMISSION_DENIED`, which is USER_ERROR - that one does what
the title says; this one does not.
EXTERNAL is still an improvement over GENERIC_INTERNAL_ERROR
(INTERNAL_ERROR), but an illegal object name or a policy refusal is a user
error, and the error type is what downstream machinery keys off (event
listeners, failure accounting, retry decisions).
Suggestion: use `io.trino.spi.StandardErrorCode.INVALID_ARGUMENTS`
(USER_ERROR) here, mirroring how the Forbidden branch borrows a standard code,
or add a USER_ERROR-typed code to `GravitinoErrorCode`. If reusing
`GRAVITINO_ILLEGAL_ARGUMENT` was a deliberate choice to keep the existing error
name stable for clients, it would help to say so in the PR description.
Verified by: GravitinoErrorCode.java:21 and :63 in this checkout; Trino 435
(pinned at `gradle/libs.versions.toml:84`) declares in core/trino-spi
StandardErrorCode: PERMISSION_DENIED(4, USER_ERROR), INVALID_ARGUMENTS(76,
USER_ERROR), GENERIC_INTERNAL_ERROR(65536, INTERNAL_ERROR).
##########
trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/catalog/TestCatalogConnectorMetadataErrors.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.
+ */
+package org.apache.gravitino.trino.connector.catalog;
+
+import static io.trino.spi.StandardErrorCode.PERMISSION_DENIED;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import io.trino.spi.TrinoException;
+import io.trino.spi.connector.SchemaTableName;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.SupportsSchemas;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.exceptions.NoSuchTableException;
+import org.apache.gravitino.rel.TableCatalog;
+import org.apache.gravitino.trino.connector.GravitinoErrorCode;
+import org.apache.gravitino.trino.connector.metadata.GravitinoTable;
+import org.junit.jupiter.api.Test;
+
+public class TestCatalogConnectorMetadataErrors {
+
+ private static final String DENIED_MESSAGE =
+ "User joel is not authorized to perform operation loadTable on metadata
m.c.s.t";
+
+ @Test
+ public void testLoadCatalogDeniedIsPermissionDenied() {
+ GravitinoMetalake metalake = mock(GravitinoMetalake.class);
+ when(metalake.loadCatalog(anyString())).thenThrow(new
ForbiddenException(DENIED_MESSAGE));
+
+ TrinoException e =
+ assertThrows(
+ TrinoException.class,
+ () -> new CatalogConnectorMetadata(metalake,
NameIdentifier.of("m", "c")));
+ assertEquals(PERMISSION_DENIED.toErrorCode(), e.getErrorCode());
+ assertEquals(DENIED_MESSAGE, e.getMessage());
+ }
+
+ @Test
+ public void testGetTableDeniedIsPermissionDenied() {
+ TableCatalog tableCatalog = mock(TableCatalog.class);
+ when(tableCatalog.loadTable(any())).thenThrow(new
ForbiddenException(DENIED_MESSAGE));
+ CatalogConnectorMetadata metadata = createMetadata(tableCatalog);
+
+ TrinoException e = assertThrows(TrinoException.class, () ->
metadata.getTable("s", "t"));
+ assertEquals(PERMISSION_DENIED.toErrorCode(), e.getErrorCode());
+ assertEquals(DENIED_MESSAGE, e.getMessage());
+ }
+
+ @Test
+ public void testGetMissingTableKeepsTableNotExists() {
+ TableCatalog tableCatalog = mock(TableCatalog.class);
+ when(tableCatalog.loadTable(any())).thenThrow(new
NoSuchTableException("missing"));
+ CatalogConnectorMetadata metadata = createMetadata(tableCatalog);
+
+ TrinoException e = assertThrows(TrinoException.class, () ->
metadata.getTable("s", "t"));
+ assertEquals(GravitinoErrorCode.GRAVITINO_TABLE_NOT_EXISTS.toErrorCode(),
e.getErrorCode());
+ }
+
+ @Test
+ public void testTableExistsDeniedIsPermissionDenied() {
+ TableCatalog tableCatalog = mock(TableCatalog.class);
+ when(tableCatalog.tableExists(any())).thenThrow(new
ForbiddenException(DENIED_MESSAGE));
+ CatalogConnectorMetadata metadata = createMetadata(tableCatalog);
+
+ TrinoException e = assertThrows(TrinoException.class, () ->
metadata.tableExists("s", "t"));
+ assertEquals(PERMISSION_DENIED.toErrorCode(), e.getErrorCode());
+ }
+
+ @Test
+ public void testCreateTableWithIllegalNameIsIllegalArgument() {
+ String message = "The TABLE name 9lives is illegal. Illegal name: 9lives";
+ TableCatalog tableCatalog = mock(TableCatalog.class);
+ when(tableCatalog.createTable(any(), any(), any(), any(), any(), any(),
any(), any()))
+ .thenThrow(new IllegalArgumentException(message));
+ CatalogConnectorMetadata metadata = createMetadata(tableCatalog);
+ GravitinoTable table = mock(GravitinoTable.class);
+ when(table.getSchemaName()).thenReturn("s");
+ when(table.getName()).thenReturn("9lives");
+
+ TrinoException e = assertThrows(TrinoException.class, () ->
metadata.createTable(table, false));
+ assertEquals(GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT.toErrorCode(),
e.getErrorCode());
+ assertEquals(message, e.getMessage());
+ }
+
+ @Test
+ public void testDropTableDeniedIsPermissionDenied() {
+ TableCatalog tableCatalog = mock(TableCatalog.class);
+ when(tableCatalog.dropTable(any())).thenThrow(new
ForbiddenException(DENIED_MESSAGE));
+ CatalogConnectorMetadata metadata = createMetadata(tableCatalog);
+
+ TrinoException e =
+ assertThrows(TrinoException.class, () -> metadata.dropTable(new
SchemaTableName("s", "t")));
+ assertEquals(PERMISSION_DENIED.toErrorCode(), e.getErrorCode());
+ }
+
+ @Test
+ public void testAlterTableDeniedIsPermissionDenied() {
+ TableCatalog tableCatalog = mock(TableCatalog.class);
+ when(tableCatalog.alterTable(any(), any())).thenThrow(new
ForbiddenException(DENIED_MESSAGE));
+ CatalogConnectorMetadata metadata = createMetadata(tableCatalog);
+
+ TrinoException e =
+ assertThrows(
+ TrinoException.class,
+ () ->
+ metadata.renameTable(
+ new SchemaTableName("s", "t"), new SchemaTableName("s",
"t2")));
+ assertEquals(PERMISSION_DENIED.toErrorCode(), e.getErrorCode());
+ }
+
+ private static CatalogConnectorMetadata createMetadata(TableCatalog
tableCatalog) {
Review Comment:
[Nit] Coverage gaps worth closing while this helper is fresh.
The seven tests cover the table paths well, but seven of the thirteen newly
guarded call sites are untested: `listSchemaNames`, `getSchema`,
`createSchema`, `dropSchema` (the schema side, which needs a mocked
`SupportsSchemas` - `createMetadata` already stubs `catalog.asSchemas()`, so it
is a two-line change), `listTables`, and `listFunctionInfos` / `getFunction`
(which need a `FunctionCatalog` mock instead of the
`UnsupportedOperationException` stub on line 139).
More valuable than breadth, though: a test whose exception message is
multi-line. Every test here uses the single-line `DENIED_MESSAGE` constant,
while the REST client always hands over "message + newline + stack" (see my
comment on `toTrinoException`), so the current assertions on `e.getMessage()`
cannot catch the mangling that users will actually see.
Verified by: reading this file in full and matching its tests against the
thirteen call sites changed in CatalogConnectorMetadata.java.
--
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]
