This is an automated email from the ASF dual-hosted git repository. diqiu50 pushed a commit to branch glue-pr03 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 085a6b4ba49ffc6085efcb9a50c3754bbc2341cf Author: diqiu50 <[email protected]> AuthorDate: Thu Apr 9 10:13:56 2026 +0800 [MINOR] fix(catalog-glue): address comprehensive PR review findings - Fix stale JavaDoc: DEFAULT_TABLE_FORMAT "Defaults to iceberg" -> "Defaults to hive" - GlueClientProvider: fail-fast on partial credentials (one key without the other) - GlueClientProvider: wrap URI.create with property-context error message - GlueCatalogCapability: remove COLUMN from case-insensitive scope (no AWS docs backing) - GlueTablePropertiesMetadata: remove ephemeral PR-05 forward reference from comment - TestGlueClientProvider: use try-with-resources; update partial-cred test to expect exception; add tests for secret-only and invalid endpoint cases - Add TestGlueCatalogCapability: covers all capability method contracts - Add TestGlueCatalogPropertiesMetadata: covers required/hidden/immutable flags and defaults --- .../catalog/glue/GlueCatalogCapability.java | 2 +- .../gravitino/catalog/glue/GlueClientProvider.java | 30 +++++-- .../gravitino/catalog/glue/GlueConstants.java | 4 +- .../catalog/glue/GlueTablePropertiesMetadata.java | 2 +- .../catalog/glue/TestGlueCatalogCapability.java | 75 ++++++++++++++++ .../glue/TestGlueCatalogPropertiesMetadata.java | 99 ++++++++++++++++++++++ .../catalog/glue/TestGlueClientProvider.java | 57 +++++++++---- 7 files changed, 242 insertions(+), 27 deletions(-) diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogCapability.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogCapability.java index 527b36de0a..98283c0463 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogCapability.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogCapability.java @@ -64,9 +64,9 @@ public class GlueCatalogCapability implements Capability { switch (scope) { case SCHEMA: case TABLE: - case COLUMN: // Glue folds database/table names to lowercase on storage. // See https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-catalog-databases.html + // See https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-catalog-tables.html return CapabilityResult.unsupported( "AWS Glue Data Catalog is case-insensitive for " + scope.name().toLowerCase(java.util.Locale.ROOT) diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java index e865ec2287..c0ade3e196 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java @@ -51,7 +51,8 @@ public final class GlueClientProvider { * * @param config Catalog configuration properties. * @return A configured and ready-to-use {@link GlueClient}. - * @throws IllegalArgumentException if {@code aws-region} is missing or blank. + * @throws IllegalArgumentException if {@code aws-region} is missing or blank, if only one of the + * credential keys is provided, or if {@code aws-glue-endpoint} is not a valid URI. */ public static GlueClient buildClient(Map<String, String> config) { String region = config.get(GlueConstants.AWS_REGION); @@ -63,12 +64,20 @@ public final class GlueClientProvider { GlueClientBuilder builder = GlueClient.builder().region(Region.of(region)); // Static credentials take priority over the default credential chain. + // Both keys must be provided together — a partial pair is always a misconfiguration. String accessKey = config.get(GlueConstants.AWS_ACCESS_KEY_ID); String secretKey = config.get(GlueConstants.AWS_SECRET_ACCESS_KEY); - if (!Strings.isNullOrEmpty(accessKey) - && !accessKey.isBlank() - && !Strings.isNullOrEmpty(secretKey) - && !secretKey.isBlank()) { + boolean hasAccessKey = !Strings.isNullOrEmpty(accessKey) && !accessKey.isBlank(); + boolean hasSecretKey = !Strings.isNullOrEmpty(secretKey) && !secretKey.isBlank(); + Preconditions.checkArgument( + hasAccessKey == hasSecretKey, + "Incomplete static credentials: '%s' requires '%s'. " + + "Either provide both keys for static authentication, " + + "or omit both to use the default credential chain.", + hasAccessKey ? GlueConstants.AWS_ACCESS_KEY_ID : GlueConstants.AWS_SECRET_ACCESS_KEY, + hasAccessKey ? GlueConstants.AWS_SECRET_ACCESS_KEY : GlueConstants.AWS_ACCESS_KEY_ID); + + if (hasAccessKey) { builder.credentialsProvider( StaticCredentialsProvider.create(AwsBasicCredentials.create(accessKey, secretKey))); } else { @@ -78,7 +87,16 @@ public final class GlueClientProvider { // Optional custom endpoint override for VPC endpoints or LocalStack testing. String endpoint = config.get(GlueConstants.AWS_GLUE_ENDPOINT); if (!Strings.isNullOrEmpty(endpoint) && !endpoint.isBlank()) { - builder.endpointOverride(URI.create(endpoint)); + try { + builder.endpointOverride(URI.create(endpoint)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + String.format( + "Property '%s' contains an invalid URI: '%s'. " + + "Expected a valid URL, e.g. 'http://localhost:4566'. Cause: %s", + GlueConstants.AWS_GLUE_ENDPOINT, endpoint, e.getMessage()), + e); + } } return builder.build(); diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java index a7a9e99783..d02f03bf7e 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java @@ -45,11 +45,11 @@ public final class GlueConstants { /** * Default table format used when creating tables via Gravitino's {@code createTable()} API - * (optional). Accepted values: {@code iceberg}, {@code hive}. Defaults to {@code iceberg}. + * (optional). Accepted values: {@code iceberg}, {@code hive}. Defaults to {@code hive}. */ public static final String DEFAULT_TABLE_FORMAT = "default-table-format"; - /** Default value for {@link #DEFAULT_TABLE_FORMAT}. */ + /** Default value for {@link #DEFAULT_TABLE_FORMAT}: {@code "hive"}. */ public static final String DEFAULT_TABLE_FORMAT_VALUE = "hive"; /** diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTablePropertiesMetadata.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTablePropertiesMetadata.java index 1b8fc6c738..60399e444d 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTablePropertiesMetadata.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTablePropertiesMetadata.java @@ -36,7 +36,7 @@ import org.apache.gravitino.connector.PropertyEntry; * operations layer and are not validated here. * * <p>Note: storage location ({@code StorageDescriptor.location}) varies by table format and is - * handled per-format in the Table CRUD layer (PR-05), not declared here. + * handled per-format in the Table CRUD layer, not declared here. */ public class GlueTablePropertiesMetadata extends BasePropertiesMetadata { diff --git a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogCapability.java b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogCapability.java new file mode 100644 index 0000000000..9581a89e2a --- /dev/null +++ b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogCapability.java @@ -0,0 +1,75 @@ +/* + * 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.catalog.glue; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.connector.capability.CapabilityResult; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class TestGlueCatalogCapability { + + private GlueCatalogCapability capability; + + @BeforeEach + void setUp() { + capability = new GlueCatalogCapability(); + } + + @Test + void testColumnNotNullIsUnsupported() { + CapabilityResult result = capability.columnNotNull(); + assertFalse(result.supported(), "Glue does not enforce NOT NULL constraints"); + assertNotNull(result.unsupportedMessage()); + assertFalse(result.unsupportedMessage().isEmpty()); + } + + @Test + void testColumnDefaultValueIsUnsupported() { + CapabilityResult result = capability.columnDefaultValue(); + assertFalse(result.supported(), "Glue does not support DEFAULT values on columns"); + assertNotNull(result.unsupportedMessage()); + assertFalse(result.unsupportedMessage().isEmpty()); + } + + @Test + void testCaseSensitiveOnSchemaIsUnsupported() { + CapabilityResult result = capability.caseSensitiveOnName(Capability.Scope.SCHEMA); + assertFalse(result.supported(), "Glue folds schema names to lowercase"); + assertNotNull(result.unsupportedMessage()); + } + + @Test + void testCaseSensitiveOnTableIsUnsupported() { + CapabilityResult result = capability.caseSensitiveOnName(Capability.Scope.TABLE); + assertFalse(result.supported(), "Glue folds table names to lowercase"); + assertNotNull(result.unsupportedMessage()); + } + + @Test + void testCaseSensitiveOnColumnIsSupported() { + // Column name case folding is not documented in Glue Column API — treated as supported. + CapabilityResult result = capability.caseSensitiveOnName(Capability.Scope.COLUMN); + assertTrue(result.supported()); + } +} diff --git a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java new file mode 100644 index 0000000000..e150c5a57c --- /dev/null +++ b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java @@ -0,0 +1,99 @@ +/* + * 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.catalog.glue; + +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_ACCESS_KEY_ID; +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_CATALOG_ID; +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_ENDPOINT; +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_REGION; +import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_SECRET_ACCESS_KEY; +import static org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_TABLE_FORMAT; +import static org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_TABLE_FORMAT_VALUE; +import static org.apache.gravitino.catalog.glue.GlueConstants.TABLE_TYPE_FILTER; +import static org.apache.gravitino.catalog.glue.GlueConstants.TABLE_TYPE_FILTER_ALL; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class TestGlueCatalogPropertiesMetadata { + + private GlueCatalogPropertiesMetadata metadata; + + @BeforeEach + void setUp() { + metadata = new GlueCatalogPropertiesMetadata(); + } + + @Test + void testAwsRegionIsRequired() { + assertTrue(metadata.isRequiredProperty(AWS_REGION)); + } + + @Test + void testAwsGlueCatalogIdIsRequired() { + assertTrue(metadata.isRequiredProperty(AWS_GLUE_CATALOG_ID)); + } + + @Test + void testAwsRegionIsImmutable() { + assertTrue(metadata.isImmutableProperty(AWS_REGION)); + } + + @Test + void testAwsGlueCatalogIdIsImmutable() { + assertTrue(metadata.isImmutableProperty(AWS_GLUE_CATALOG_ID)); + } + + @Test + void testCredentialsAreHidden() { + assertTrue(metadata.isHiddenProperty(AWS_ACCESS_KEY_ID)); + assertTrue(metadata.isHiddenProperty(AWS_SECRET_ACCESS_KEY)); + } + + @Test + void testCredentialsAreOptional() { + assertFalse(metadata.isRequiredProperty(AWS_ACCESS_KEY_ID)); + assertFalse(metadata.isRequiredProperty(AWS_SECRET_ACCESS_KEY)); + } + + @Test + void testEndpointIsOptionalAndNotHidden() { + assertFalse(metadata.isRequiredProperty(AWS_GLUE_ENDPOINT)); + assertFalse(metadata.isHiddenProperty(AWS_GLUE_ENDPOINT)); + } + + @Test + void testDefaultTableFormatDefaultValue() { + assertEquals( + DEFAULT_TABLE_FORMAT_VALUE, + metadata.getDefaultValue(DEFAULT_TABLE_FORMAT), + "Default table format should be 'hive'"); + } + + @Test + void testTableTypeFilterDefaultValue() { + assertEquals( + TABLE_TYPE_FILTER_ALL, + metadata.getDefaultValue(TABLE_TYPE_FILTER), + "Default table type filter should be 'all'"); + } +} diff --git a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueClientProvider.java b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueClientProvider.java index 022a155690..046f1d616d 100644 --- a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueClientProvider.java +++ b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueClientProvider.java @@ -24,6 +24,7 @@ import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_REGION; import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_SECRET_ACCESS_KEY; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.HashMap; import java.util.Map; @@ -39,9 +40,9 @@ class TestGlueClientProvider { config.put(AWS_ACCESS_KEY_ID, "AKIAIOSFODNN7EXAMPLE"); config.put(AWS_SECRET_ACCESS_KEY, "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"); - GlueClient client = GlueClientProvider.buildClient(config); - assertNotNull(client); - client.close(); + try (GlueClient client = GlueClientProvider.buildClient(config)) { + assertNotNull(client); + } } @Test @@ -50,9 +51,9 @@ class TestGlueClientProvider { Map<String, String> config = new HashMap<>(); config.put(AWS_REGION, "eu-west-1"); - GlueClient client = GlueClientProvider.buildClient(config); - assertNotNull(client); - client.close(); + try (GlueClient client = GlueClientProvider.buildClient(config)) { + assertNotNull(client); + } } @Test @@ -63,15 +64,14 @@ class TestGlueClientProvider { config.put(AWS_SECRET_ACCESS_KEY, "test"); config.put(AWS_GLUE_ENDPOINT, "http://localhost:4566";); - GlueClient client = GlueClientProvider.buildClient(config); - assertNotNull(client); - client.close(); + try (GlueClient client = GlueClientProvider.buildClient(config)) { + assertNotNull(client); + } } @Test void testBuildClientMissingRegionThrows() { Map<String, String> config = new HashMap<>(); - // No AWS_REGION set. assertThrows(IllegalArgumentException.class, () -> GlueClientProvider.buildClient(config)); } @@ -85,16 +85,39 @@ class TestGlueClientProvider { } @Test - void testBuildClientOnlyAccessKeyFallsBackToDefaultChain() { - // Only one of the key pair provided → both must be present to use static creds. - // Falls back to default chain, which just builds without error. + void testBuildClientOnlyAccessKeyThrows() { + // Providing only one of the credential pair is always a misconfiguration — must fail fast. Map<String, String> config = new HashMap<>(); config.put(AWS_REGION, "ap-southeast-1"); config.put(AWS_ACCESS_KEY_ID, "AKIAIOSFODNN7EXAMPLE"); - // No AWS_SECRET_ACCESS_KEY → default chain is used instead. + // No AWS_SECRET_ACCESS_KEY. - GlueClient client = GlueClientProvider.buildClient(config); - assertNotNull(client); - client.close(); + IllegalArgumentException ex = + assertThrows(IllegalArgumentException.class, () -> GlueClientProvider.buildClient(config)); + assertTrue(ex.getMessage().contains(AWS_SECRET_ACCESS_KEY)); + } + + @Test + void testBuildClientOnlySecretKeyThrows() { + // Providing only the secret without the access key is also a misconfiguration. + Map<String, String> config = new HashMap<>(); + config.put(AWS_REGION, "ap-southeast-1"); + config.put(AWS_SECRET_ACCESS_KEY, "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"); + // No AWS_ACCESS_KEY_ID. + + IllegalArgumentException ex = + assertThrows(IllegalArgumentException.class, () -> GlueClientProvider.buildClient(config)); + assertTrue(ex.getMessage().contains(AWS_ACCESS_KEY_ID)); + } + + @Test + void testBuildClientInvalidEndpointThrows() { + Map<String, String> config = new HashMap<>(); + config.put(AWS_REGION, "us-east-1"); + config.put(AWS_GLUE_ENDPOINT, "not a valid uri ://"); + + IllegalArgumentException ex = + assertThrows(IllegalArgumentException.class, () -> GlueClientProvider.buildClient(config)); + assertTrue(ex.getMessage().contains(AWS_GLUE_ENDPOINT)); } }
