Copilot commented on code in PR #11294:
URL: https://github.com/apache/gravitino/pull/11294#discussion_r3332617946
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -83,6 +102,63 @@ public String getValue() {
private IcebergRESTUtils() {}
+ /**
+ * Builds an Iceberg REST {@link
org.apache.iceberg.rest.credentials.Credential} for load-table,
+ * scan-plan, or credentials API responses.
+ *
+ * @param catalogName IRC catalog name used in the refresh path
+ * @param tableIdentifier table receiving the credential
+ * @param credential Gravitino credential to vend
+ * @param tableMetadata table metadata used to derive the storage prefix
+ * @return Iceberg REST credential with prefix and config
+ */
+ public static org.apache.iceberg.rest.credentials.Credential
toRestCredential(
+ String catalogName,
+ TableIdentifier tableIdentifier,
+ Credential credential,
+ TableMetadata tableMetadata) {
+ Map<String, String> config =
+ new HashMap<>(CredentialPropertyUtils.toIcebergProperties(credential));
+
+ String refreshProperty = null;
+ if (credential instanceof GCSTokenCredential) {
+ refreshProperty = GCS_OAUTH2_REFRESH_CREDENTIALS_ENDPOINT;
+ } else if (credential instanceof ADLSTokenCredential) {
+ refreshProperty = ADLS_REFRESH_CREDENTIALS_ENDPOINT;
+ } else if (credential instanceof S3TokenCredential
+ || credential instanceof AwsIrsaCredential
+ || credential instanceof OSSTokenCredential) {
+ refreshProperty = CLIENT_REFRESH_CREDENTIALS_ENDPOINT;
+ }
+
+ if (refreshProperty != null) {
+ config.put(
+ refreshProperty,
+ String.format(
+ "v1/%s/namespaces/%s/tables/%s/credentials",
+ RESTUtil.encodeString(catalogName),
+ RESTUtil.encodeNamespace(
+ tableIdentifier.namespace(),
NAMESPACE_SEPARATOR_URLENCODED_UTF_8),
+ RESTUtil.encodeString(tableIdentifier.name())));
+ }
+
+ String prefix = Strings.CS.appendIfMissing(tableMetadata.location(), "/");
Review Comment:
`Strings.CS.appendIfMissing(...)` introduces a dependency on
`org.apache.commons.lang3.Strings` while this class already uses `StringUtils`.
To reduce API surface area and keep compatibility with common `commons-lang3`
baselines, prefer `StringUtils.appendIfMissing(tableMetadata.location(),
\"/\")` here.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java:
##########
@@ -87,4 +99,152 @@ void testSerdeIcebergRESTObject() {
Assertions.assertEquals(field, clonedField);
}
}
+
+ @Test
+ void testTableCredentialsPath() {
+ TableIdentifier table = TableIdentifier.of(Namespace.of("ns"), "tbl");
+ TableMetadata tableMetadata = mock(TableMetadata.class);
+ when(tableMetadata.location()).thenReturn("s3://bucket/t/");
+ org.apache.iceberg.rest.credentials.Credential credential =
+ IcebergRESTUtils.toRestCredential(
+ "my_catalog", table, new S3TokenCredential("k", "s", "t", 99L),
tableMetadata);
+ Assertions.assertEquals(
+ "v1/my_catalog/namespaces/ns/tables/tbl/credentials",
+ credential.config().get("client.refresh-credentials-endpoint"));
+ }
+
+ @Test
+ void testToRestCredential() {
+ TableIdentifier table = TableIdentifier.of(Namespace.of("ns"), "tbl");
+ String refreshPath = "v1/cat/namespaces/ns/tables/tbl/credentials";
+ TableMetadata metadataWithSlash = mock(TableMetadata.class);
+ when(metadataWithSlash.location()).thenReturn("s3://bucket/t/");
+ org.apache.iceberg.rest.credentials.Credential credentialWithSlash =
+ IcebergRESTUtils.toRestCredential(
+ "cat", table, new S3TokenCredential("k", "s", "t", 99L),
metadataWithSlash);
+ Assertions.assertEquals("s3://bucket/t/", credentialWithSlash.prefix());
+ Assertions.assertEquals(
+ "99",
credentialWithSlash.config().get("s3.session-token-expires-at-ms"));
+ Assertions.assertEquals(
+ refreshPath,
credentialWithSlash.config().get("client.refresh-credentials-endpoint"));
+
+ TableMetadata metadataWithoutSlash = mock(TableMetadata.class);
+
when(metadataWithoutSlash.location()).thenReturn("s3://bucket/path/to/table");
+ org.apache.iceberg.rest.credentials.Credential credentialWithoutSlash =
+ IcebergRESTUtils.toRestCredential(
+ "cat", table, new S3TokenCredential("k", "s", "t", 99L),
metadataWithoutSlash);
+ Assertions.assertEquals("s3://bucket/path/to/table/",
credentialWithoutSlash.prefix());
+ }
+
+ @Test
+ void testToRestCredentialForS3Token() {
+ TableIdentifier table = TableIdentifier.of(Namespace.of("ns"), "tbl");
+ TableMetadata tableMetadata = mock(TableMetadata.class);
+ when(tableMetadata.location()).thenReturn("s3://bucket/t/");
+ Map<String, String> config =
+ IcebergRESTUtils.toRestCredential(
+ "aws", table, new S3TokenCredential("key", "secret", "token",
1234L), tableMetadata)
+ .config();
Review Comment:
Several tests repeat the same table/tableMetadata mocking and
refresh-endpoint assertion pattern for different credential types. Consider
extracting a small helper (e.g., building `TableIdentifier` + mocking
`TableMetadata.location()`) or using a parameterized test to reduce duplication
and make it easier to add new credential types consistently.
##########
common/src/main/java/org/apache/gravitino/credential/CredentialPropertyUtils.java:
##########
@@ -49,7 +55,13 @@ public class CredentialPropertyUtils {
@VisibleForTesting
static final String ICEBERG_ADLS_ACCOUNT_KEY =
"adls.auth.shared-key.account.key";
- private static final String GCS_OAUTH_2_TOKEN_EXPIRES_AT =
"gcs.oauth2.token-expires-at";
+ @VisibleForTesting
+ static final String ICEBERG_ADLS_SAS_TOKEN_EXPIRES_AT_MS_PREFIX =
"adls.sas-token-expires-at-ms.";
Review Comment:
This constant definition exceeds typical style guide line-length limits in
many Java builds and is likely to trigger formatting/checkstyle violations if
enforced. Consider wrapping it across lines similarly to the other multi-line
constants in this file.
--
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]
