sririshindra commented on code in PR #4023: URL: https://github.com/apache/polaris/pull/4023#discussion_r3002981976
########## polaris-core/src/main/java/org/apache/polaris/core/storage/StorageNameValidator.java: ########## @@ -0,0 +1,78 @@ +/* + * 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.polaris.core.storage; + +import jakarta.annotation.Nullable; +import java.util.regex.Pattern; + +/** + * Validates storage name references used in {@code polaris.storage.name} property overrides. + * + * <p>Storage names are symbolic references to server-side credential configurations (e.g. {@code + * polaris.storage.aws.<storageName>.access-key}). They must be valid Quarkus config key segments. + */ +public final class StorageNameValidator { + + /** Maximum length for a storage name. */ + public static final int MAX_LENGTH = 128; + + /** Alphanumeric characters, hyphens, and underscores only. */ + private static final Pattern VALID_PATTERN = Pattern.compile("^[a-zA-Z0-9_-]+$"); + + private StorageNameValidator() {} + + /** + * Validates a storage name format. + * + * @param storageName the name to validate + * @throws IllegalArgumentException if the name is invalid + */ + public static void validate(String storageName) { + if (storageName == null || storageName.isEmpty()) { + throw new IllegalArgumentException("Storage name must not be null or empty"); + } + if (storageName.length() > MAX_LENGTH) { + throw new IllegalArgumentException( + String.format( + "Storage name must not exceed %d characters, got %d", + MAX_LENGTH, storageName.length())); + } + if (!VALID_PATTERN.matcher(storageName).matches()) { + throw new IllegalArgumentException( + String.format( + "Storage name '%s' contains invalid characters. " + + "Only alphanumeric characters, hyphens, and underscores are allowed.", + storageName)); + } + } + + /** + * Normalizes a blank or empty string to {@code null}. Non-blank values are trimmed. + * + * @param value the input value + * @return {@code null} if the input is null, empty, or blank; otherwise the trimmed value + */ + public static @Nullable String normalizeBlankToNull(@Nullable String value) { Review Comment: They serve different purposes: validate() is a pure check, while normalizeBlankToNull() is a transformation. Combining them would make validate() impure — callers wouldn't expect a validation method to also mutate/transform the input. That said, I could rename normalizeBlankToNull to something clearer like toNullIfBlank or inline it at the call sites if you'd prefer. Open to suggestions. ########## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java: ########## @@ -755,8 +806,19 @@ public boolean removeProperties(Namespace namespace, Set<String> properties) Map<String, String> updatedProperties = new HashMap<>(entity.getPropertiesAsMap()); properties.forEach(updatedProperties::remove); - PolarisEntity updatedEntity = - new PolarisEntity.Builder(entity).setProperties(updatedProperties).build(); + PolarisEntity.Builder updatedEntityBuilder = + new PolarisEntity.Builder(entity).setProperties(updatedProperties); + + // If polaris.storage.name is being removed, clear the storage config override + if (properties.contains(POLARIS_STORAGE_NAME_PROPERTY)) { + LOGGER.debug("Clearing storage name override on namespace {}", namespace); + Map<String, String> updatedInternalProperties = + new HashMap<>(entity.getInternalPropertiesAsMap()); + updatedInternalProperties.remove(PolarisEntityConstants.getStorageConfigInfoPropertyName()); Review Comment: I am not sure I understood your point correctly. But from what I understood, the challenge is that polaris.storage.name is a user-facing property (in getPropertiesAsMap()) while storage_name_override is an internal property (in getInternalPropertiesAsMap()). These are separate maps, so a key transformation before the first removal pass wouldn't help. We'd still need to reach into both maps. The special if-block is the way we bridge the user-facing property removal to its internal property counterpart. But maybe I am getting confused. Could you elaborate your point a bit more. ########## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java: ########## @@ -520,15 +534,35 @@ private void createNamespaceInternal( baseLocation += "/"; } - NamespaceEntity entity = + enforceStorageNameOverrideEnabledIfRequested(metadata); + + PolarisStorageConfigurationInfo namespaceStorageConfig = + storageConfigFromPropertyOverride(metadata, resolvedParent.getRawFullPath()); + + NamespaceEntity.Builder nsBuilder = new NamespaceEntity.Builder(namespace) .setCatalogId(getCatalogId()) .setId(getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()) .setParentId(resolvedParent.getRawLeafEntity().getId()) .setProperties(metadata) .setCreateTimestamp(System.currentTimeMillis()) - .setBaseLocation(baseLocation) - .build(); + .setBaseLocation(baseLocation); + NamespaceEntity entity; + if (namespaceStorageConfig != null) { + LOGGER.debug( + "Applying storage name override '{}' on namespace {}", + namespaceStorageConfig.getStorageName(), + namespace); + entity = + new NamespaceEntity( + new PolarisEntity.Builder(nsBuilder.build()) + .addInternalProperty( + PolarisEntityConstants.getStorageConfigInfoPropertyName(), + namespaceStorageConfig.serialize()) Review Comment: Agreed — this is fixed in the latest revision. Entities now store only a lightweight storage_name_override string reference, not a full config copy. The full config resolution happens on-demand at access time. ########## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java: ########## @@ -1745,7 +1811,78 @@ private String newTableMetadataFilePath(TableMetadata meta, int newVersion) { } } - private static Map<String, String> buildTableMetadataPropertiesMap(TableMetadata metadata) { + /** + * Resolves a storage configuration override from the {@code polaris.storage.name} property. If + * the property is present, walks the parent entity path to find the nearest ancestor with a + * storage configuration and returns a copy with the storageName overridden. + * + * @param properties the namespace or table properties to inspect + * @param parentPath the parent entity path (root-to-leaf) to search for inherited storage config + * @return the overridden config, or null if no override is requested + */ + private static @Nullable PolarisStorageConfigurationInfo storageConfigFromPropertyOverride( + Map<String, String> properties, List<PolarisEntity> parentPath) { + if (!properties.containsKey(POLARIS_STORAGE_NAME_PROPERTY)) { + return null; + } + + String rawValue = properties.get(POLARIS_STORAGE_NAME_PROPERTY); + String storageName = StorageNameValidator.normalizeBlankToNull(rawValue); + if (storageName != null) { + StorageNameValidator.validate(storageName); + } + + PolarisStorageConfigurationInfo baseConfig = deserializeStorageConfigFromEntityPath(parentPath); Review Comment: In the updated implementation, polaris.storage.name is the user-facing API property that gets translated into an internal storage_name_override reference. At resolution time, StorageConfigResolver finds the nearest override in the hierarchy and applies it to the catalog's base config via withStorageName(). The purpose is to allow different namespaces/tables to use different named storage configurations while the actual configs remain managed centrally at the catalog level. The property is no longer redundant — it's the mechanism by which users select which storage configuration to use. ########## runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/StorageNameOverrideTest.java: ########## @@ -0,0 +1,309 @@ +/* + * 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.polaris.service.catalog.iceberg; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.iceberg.exceptions.BadRequestException; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.auth.PolarisPrincipal; +import org.apache.polaris.core.config.FeatureConfiguration; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PolarisEntityConstants; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; +import org.apache.polaris.core.persistence.resolver.ResolverFactory; +import org.apache.polaris.core.storage.FileStorageConfigurationInfo; +import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.apache.polaris.service.catalog.io.FileIOFactory; +import org.apache.polaris.service.catalog.io.FileIOUtil; +import org.apache.polaris.service.catalog.io.StorageAccessConfigProvider; +import org.apache.polaris.service.events.PolarisEventMetadataFactory; +import org.apache.polaris.service.events.listeners.PolarisEventListener; +import org.apache.polaris.service.task.TaskExecutor; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for the static helper methods in {@link IcebergCatalog} that handle {@code + * polaris.storage.name} property overrides. These test the pure logic without requiring a full + * Quarkus test context. + */ +class StorageNameOverrideTest { + + private static final String STORAGE_CONFIG_KEY = + PolarisEntityConstants.getStorageConfigInfoPropertyName(); + + @Test + void storageConfigFromPropertyOverride_noProperty_returnsNull() throws Exception { + // Use reflection since the method is package-private/static + var method = Review Comment: Agreed — replaced the reflection with @VisibleForTesting on the method. ########## polaris-core/src/test/java/org/apache/polaris/core/storage/WithStorageNameTest.java: ########## @@ -0,0 +1,171 @@ +/* + * 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.polaris.core.storage; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo; +import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo; +import org.junit.jupiter.api.Test; + +class WithStorageNameTest { + + @Test + void awsFieldsPreserved() { + AwsStorageConfigurationInfo base = + AwsStorageConfigurationInfo.builder() + .allowedLocations(List.of("s3://bucket/path")) Review Comment: Done — extracted the expected values into local variables and referenced them in both the setup and assertion. ########## polaris-core/src/test/java/org/apache/polaris/core/storage/WithStorageNameTest.java: ########## @@ -0,0 +1,171 @@ +/* + * 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.polaris.core.storage; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo; +import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo; +import org.junit.jupiter.api.Test; + +class WithStorageNameTest { + + @Test + void awsFieldsPreserved() { + AwsStorageConfigurationInfo base = + AwsStorageConfigurationInfo.builder() + .allowedLocations(List.of("s3://bucket/path")) + .storageName("original") + .roleARN("arn:aws:iam::123456789012:role/test-role") + .externalId("ext-id-123") + .region("us-west-2") + .endpoint("https://s3.example.com";) + .endpointInternal("https://s3-internal.example.com";) + .stsEndpoint("https://sts.example.com";) + .pathStyleAccess(true) + .stsUnavailable(true) + .kmsUnavailable(false) + .currentKmsKey("arn:aws:kms:us-west-2:123456789012:key/test") + .allowedKmsKeys(List.of("arn:aws:kms:us-west-2:123456789012:key/test")) + .build(); + + PolarisStorageConfigurationInfo result = + PolarisStorageConfigurationInfo.withStorageName(base, "new-storage"); + + assertThat(result).isInstanceOf(AwsStorageConfigurationInfo.class); + AwsStorageConfigurationInfo awsResult = (AwsStorageConfigurationInfo) result; + assertThat(awsResult.getStorageName()).isEqualTo("new-storage"); + assertThat(awsResult.getAllowedLocations()).containsExactly("s3://bucket/path"); + assertThat(awsResult.getRoleARN()).isEqualTo("arn:aws:iam::123456789012:role/test-role"); + assertThat(awsResult.getExternalId()).isEqualTo("ext-id-123"); + assertThat(awsResult.getRegion()).isEqualTo("us-west-2"); + assertThat(awsResult.getEndpoint()).isEqualTo("https://s3.example.com";); + assertThat(awsResult.getEndpointInternal()).isEqualTo("https://s3-internal.example.com";); + assertThat(awsResult.getStsEndpoint()).isEqualTo("https://sts.example.com";); + assertThat(awsResult.getPathStyleAccess()).isTrue(); + assertThat(awsResult.getStsUnavailable()).isTrue(); + assertThat(awsResult.getKmsUnavailable()).isFalse(); + assertThat(awsResult.getCurrentKmsKey()) + .isEqualTo("arn:aws:kms:us-west-2:123456789012:key/test"); + assertThat(awsResult.getAllowedKmsKeys()) + .containsExactly("arn:aws:kms:us-west-2:123456789012:key/test"); + } + + @Test + void awsNullStorageNameClearsOverride() { + AwsStorageConfigurationInfo base = + AwsStorageConfigurationInfo.builder() + .allowedLocations(List.of("s3://bucket/path")) + .storageName("original") + .roleARN("arn:aws:iam::123456789012:role/test-role") + .build(); + + PolarisStorageConfigurationInfo result = + PolarisStorageConfigurationInfo.withStorageName(base, null); + + assertThat(result.getStorageName()).isNull(); + assertThat(result.getAllowedLocations()).containsExactly("s3://bucket/path"); + } + + @Test + void azureFieldsPreserved() { + AzureStorageConfigurationInfo base = + AzureStorageConfigurationInfo.builder() + .allowedLocations(List.of("abfss://[email protected]/path")) + .storageName("original") + .tenantId("tenant-123") + .multiTenantAppName("my-app") + .consentUrl("https://consent.example.com";) + .hierarchical(true) + .build(); + + PolarisStorageConfigurationInfo result = + PolarisStorageConfigurationInfo.withStorageName(base, "azure-storage"); + + assertThat(result).isInstanceOf(AzureStorageConfigurationInfo.class); + AzureStorageConfigurationInfo azureResult = (AzureStorageConfigurationInfo) result; + assertThat(azureResult.getStorageName()).isEqualTo("azure-storage"); + assertThat(azureResult.getAllowedLocations()) + .containsExactly("abfss://[email protected]/path"); + assertThat(azureResult.getTenantId()).isEqualTo("tenant-123"); + assertThat(azureResult.getMultiTenantAppName()).isEqualTo("my-app"); + assertThat(azureResult.getConsentUrl()).isEqualTo("https://consent.example.com";); + assertThat(azureResult.isHierarchical()).isTrue(); + } + + @Test + void gcpFieldsPreserved() { + GcpStorageConfigurationInfo base = + GcpStorageConfigurationInfo.builder() + .allowedLocations(List.of("gs://bucket/path")) + .storageName("original") + .gcpServiceAccount("[email protected]") + .build(); + + PolarisStorageConfigurationInfo result = + PolarisStorageConfigurationInfo.withStorageName(base, "gcp-storage"); + + assertThat(result).isInstanceOf(GcpStorageConfigurationInfo.class); + GcpStorageConfigurationInfo gcpResult = (GcpStorageConfigurationInfo) result; + assertThat(gcpResult.getStorageName()).isEqualTo("gcp-storage"); + assertThat(gcpResult.getAllowedLocations()).containsExactly("gs://bucket/path"); + assertThat(gcpResult.getGcpServiceAccount()).isEqualTo("[email protected]"); + } + + @Test + void fileFieldsPreserved() { + FileStorageConfigurationInfo base = + FileStorageConfigurationInfo.builder() + .allowedLocations(List.of("file:///tmp/warehouse")) + .storageName("original") + .build(); + + PolarisStorageConfigurationInfo result = + PolarisStorageConfigurationInfo.withStorageName(base, "file-storage"); + + assertThat(result).isInstanceOf(FileStorageConfigurationInfo.class); + assertThat(result.getStorageName()).isEqualTo("file-storage"); + assertThat(result.getAllowedLocations()).containsExactly("file:///tmp/warehouse"); + } + + @Test + void serializationRoundTrip() { Review Comment: You're right. The intent was to verify that a config produced by withStorageName() survives serialize() → deserialize() since these objects use Jackson's @JsonTypeInfo/@JsonSubTypes for polymorphic deserialization. But in practice, builder().from() produces the same concrete type, so Jackson will always serialize/deserialize it correctly. Looks like awsFieldsPreserved test already covers field preservation. Happy to remove the round-trip test if you think it's redundant. ########## polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java: ########## @@ -169,6 +169,22 @@ public static void enforceFeatureEnabledOrThrow( .defaultValue(false) .buildFeatureConfiguration(); + public static final FeatureConfiguration<Boolean> ALLOW_STORAGE_NAME_OVERRIDE = + PolarisConfiguration.<Boolean>builder() + .key("ALLOW_STORAGE_NAME_OVERRIDE") Review Comment: added a CHANGELOG entry for ALLOW_STORAGE_NAME_OVERRIDE ########## runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/StorageNameOverrideTest.java: ########## @@ -0,0 +1,309 @@ +/* + * 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.polaris.service.catalog.iceberg; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.iceberg.exceptions.BadRequestException; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.auth.PolarisPrincipal; +import org.apache.polaris.core.config.FeatureConfiguration; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PolarisEntityConstants; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; +import org.apache.polaris.core.persistence.resolver.ResolverFactory; +import org.apache.polaris.core.storage.FileStorageConfigurationInfo; +import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.apache.polaris.service.catalog.io.FileIOFactory; +import org.apache.polaris.service.catalog.io.FileIOUtil; +import org.apache.polaris.service.catalog.io.StorageAccessConfigProvider; +import org.apache.polaris.service.events.PolarisEventMetadataFactory; +import org.apache.polaris.service.events.listeners.PolarisEventListener; +import org.apache.polaris.service.task.TaskExecutor; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for the static helper methods in {@link IcebergCatalog} that handle {@code + * polaris.storage.name} property overrides. These test the pure logic without requiring a full + * Quarkus test context. + */ +class StorageNameOverrideTest { + + private static final String STORAGE_CONFIG_KEY = + PolarisEntityConstants.getStorageConfigInfoPropertyName(); + + @Test + void storageConfigFromPropertyOverride_noProperty_returnsNull() throws Exception { + // Use reflection since the method is package-private/static + var method = + IcebergCatalog.class.getDeclaredMethod( + "storageConfigFromPropertyOverride", Map.class, List.class); + method.setAccessible(true); + + PolarisStorageConfigurationInfo result = + (PolarisStorageConfigurationInfo) method.invoke(null, Map.of("other", "value"), List.of()); + assertThat(result).isNull(); + } + + @Test + void storageConfigFromPropertyOverride_withValidStorageName() throws Exception { + var method = + IcebergCatalog.class.getDeclaredMethod( + "storageConfigFromPropertyOverride", Map.class, List.class); + method.setAccessible(true); + + AwsStorageConfigurationInfo catalogConfig = + AwsStorageConfigurationInfo.builder() + .allowedLocations(List.of("s3://bucket/path")) + .roleARN("arn:aws:iam::123456789012:role/test-role") + .storageName("catalog-storage") + .build(); + + PolarisEntity catalogEntity = createEntityWithStorageConfig(catalogConfig); + + Map<String, String> properties = new HashMap<>(); + properties.put(IcebergCatalog.POLARIS_STORAGE_NAME_PROPERTY, "ns-storage"); + + PolarisStorageConfigurationInfo result = + (PolarisStorageConfigurationInfo) method.invoke(null, properties, List.of(catalogEntity)); + + assertThat(result).isNotNull(); + assertThat(result.getStorageName()).isEqualTo("ns-storage"); + assertThat(result).isInstanceOf(AwsStorageConfigurationInfo.class); + assertThat(((AwsStorageConfigurationInfo) result).getRoleARN()) + .isEqualTo("arn:aws:iam::123456789012:role/test-role"); + } + + @Test + void storageConfigFromPropertyOverride_blankValue_clearsStorageName() throws Exception { + var method = + IcebergCatalog.class.getDeclaredMethod( + "storageConfigFromPropertyOverride", Map.class, List.class); + method.setAccessible(true); + + AwsStorageConfigurationInfo catalogConfig = + AwsStorageConfigurationInfo.builder() + .allowedLocations(List.of("s3://bucket/path")) + .roleARN("arn:aws:iam::123456789012:role/test-role") + .storageName("catalog-storage") + .build(); + + PolarisEntity catalogEntity = createEntityWithStorageConfig(catalogConfig); + + Map<String, String> properties = new HashMap<>(); + properties.put(IcebergCatalog.POLARIS_STORAGE_NAME_PROPERTY, " "); + + PolarisStorageConfigurationInfo result = + (PolarisStorageConfigurationInfo) method.invoke(null, properties, List.of(catalogEntity)); + + assertThat(result).isNotNull(); + assertThat(result.getStorageName()).isNull(); + } + + @Test + void storageConfigFromPropertyOverride_noParentConfig_throws() throws Exception { + var method = + IcebergCatalog.class.getDeclaredMethod( + "storageConfigFromPropertyOverride", Map.class, List.class); + method.setAccessible(true); + + Map<String, String> properties = new HashMap<>(); + properties.put(IcebergCatalog.POLARIS_STORAGE_NAME_PROPERTY, "ns-storage"); + + assertThatThrownBy(() -> method.invoke(null, properties, List.of())) + .hasCauseInstanceOf(org.apache.iceberg.exceptions.BadRequestException.class) Review Comment: Fixed — removed the fully qualified name ########## polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java: ########## @@ -180,6 +180,63 @@ public static Optional<LocationRestrictions> forEntityPath( return Optional.empty(); } + /** + * Creates a copy of the given storage configuration with only the {@code storageName} field + * replaced. All other fields (allowed locations, cloud identity, etc.) are preserved from the + * base config. + * + * @param baseConfig the storage configuration to copy + * @param storageName the new storage name (may be null to clear an override) + * @return a new configuration instance with the updated storage name + * @throws IllegalArgumentException if the base config type is not supported + */ + public static PolarisStorageConfigurationInfo withStorageName( + PolarisStorageConfigurationInfo baseConfig, @Nullable String storageName) { + if (baseConfig instanceof AwsStorageConfigurationInfo awsConfig) { + return AwsStorageConfigurationInfo.builder() Review Comment: refactored withStorageName() to use builder().from() for each storage type. -- 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]
