Fix minor issues like possible NPE, unused local variable, typo. (#229)

* Fix minor issues like possible NPE, unused local variable, typo. * Fix few warnings for storage like typo, required nullable. * Addressed Comments. --------- Co-authored-by: Naveen Kumar <[email protected]> Co-authored-by: Eric Maynard <[email protected]>
apache · Oct 7, 2024 · bb47225 · bb47225
1 parent f58924b
commit bb47225
Show file tree

Hide file tree

Showing 8 changed files with 32 additions and 29 deletions.
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java
@@ -130,7 +130,6 @@ public Catalog asCatalog() {
   private StorageConfigInfo getStorageInfo(Map<String, String> internalProperties) {
     if (internalProperties.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName())) {
       PolarisStorageConfigurationInfo configInfo = getStorageConfigurationInfo();
-      PolarisStorageConfigurationInfo.StorageType storageType = configInfo.getStorageType();
       if (configInfo instanceof AwsStorageConfigurationInfo) {
         AwsStorageConfigurationInfo awsConfig = (AwsStorageConfigurationInfo) configInfo;
         return AwsStorageConfigInfo.builder()

diff --git a/...e/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/...e/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
@@ -22,7 +22,6 @@
 import java.util.EnumMap;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Stream;
 import org.apache.polaris.core.PolarisDiagnostics;
@@ -97,8 +96,8 @@ private IamPolicy policyString(
             .effect(IamEffect.ALLOW)
             .addAction("s3:GetObject")
             .addAction("s3:GetObjectVersion");
-    Map<String, IamStatement.Builder> bucketListStatmentBuilder = new HashMap<>();
-    Map<String, IamStatement.Builder> bucketGetLocationStatmentBuilder = new HashMap<>();
+    Map<String, IamStatement.Builder> bucketListStatementBuilder = new HashMap<>();
+    Map<String, IamStatement.Builder> bucketGetLocationStatementBuilder = new HashMap<>();
 
     String arnPrefix = getArnPrefixFor(roleArn);
     Stream.concat(readLocations.stream(), writeLocations.stream())
@@ -112,7 +111,7 @@ private IamPolicy policyString(
                       arnPrefix + StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/")));
               final var bucket = arnPrefix + StorageUtil.getBucket(uri);
               if (allowList) {
-                bucketListStatmentBuilder
+                bucketListStatementBuilder
                     .computeIfAbsent(
                         bucket,
                         (String key) ->
@@ -125,7 +124,7 @@ private IamPolicy policyString(
                         "s3:prefix",
                         StorageUtil.concatFilePrefixes(trimLeadingSlash(uri.getPath()), "*", "/"));
               }
-              bucketGetLocationStatmentBuilder.computeIfAbsent(
+              bucketGetLocationStatementBuilder.computeIfAbsent(
                   bucket,
                   key ->
                       IamStatement.builder()
@@ -150,8 +149,8 @@ private IamPolicy policyString(
           });
       policyBuilder.addStatement(allowPutObjectStatementBuilder.build());
     }
-    if (!bucketListStatmentBuilder.isEmpty()) {
-      bucketListStatmentBuilder
+    if (!bucketListStatementBuilder.isEmpty()) {
+      bucketListStatementBuilder
           .values()
           .forEach(statementBuilder -> policyBuilder.addStatement(statementBuilder.build()));
     } else if (allowList) {
@@ -160,7 +159,7 @@ private IamPolicy policyString(
           IamStatement.builder().effect(IamEffect.ALLOW).addAction("s3:ListBucket").build());
     }
 
-    bucketGetLocationStatmentBuilder
+    bucketGetLocationStatementBuilder
         .values()
         .forEach(statementBuilder -> policyBuilder.addStatement(statementBuilder.build()));
     return policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
@@ -179,8 +178,7 @@ private String getArnPrefixFor(String roleArn) {
   private static @NotNull String parseS3Path(URI uri) {
     String bucket = StorageUtil.getBucket(uri);
     String path = trimLeadingSlash(uri.getPath());
-    return String.join(
-        "/", Stream.of(bucket, path).filter(Objects::nonNull).toArray(String[]::new));
+    return String.join("/", bucket, path);
   }
 
   private static @NotNull String trimLeadingSlash(String path) {

diff --git a/...s-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/...s-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
@@ -42,11 +42,11 @@ public class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo
   private final @NotNull String roleARN;
 
   // AWS external ID, optional
-  @JsonProperty(value = "externalId", required = false)
+  @JsonProperty(value = "externalId")
   private @Nullable String externalId = null;
 
   /** User ARN for the service principal */
-  @JsonProperty(value = "userARN", required = false)
+  @JsonProperty(value = "userARN")
   private @Nullable String userARN = null;
 
   @JsonCreator
@@ -95,7 +95,7 @@ public void validateArn(String arn) {
     return externalId;
   }
 
-  public void setExternalId(String externalId) {
+  public void setExternalId(@Nullable String externalId) {
     this.externalId = externalId;
   }
 

diff --git a/...re/src/main/java/org/apache/polaris/core/storage/azure/AzureStorageConfigurationInfo.java b/...re/src/main/java/org/apache/polaris/core/storage/azure/AzureStorageConfigurationInfo.java
@@ -64,19 +64,19 @@ public String getFileIoImplClassName() {
     return tenantId;
   }
 
-  public String getMultiTenantAppName() {
+  public @Nullable String getMultiTenantAppName() {
     return multiTenantAppName;
   }
 
-  public void setMultiTenantAppName(String multiTenantAppName) {
+  public void setMultiTenantAppName(@Nullable String multiTenantAppName) {
     this.multiTenantAppName = multiTenantAppName;
   }
 
-  public String getConsentUrl() {
+  public @Nullable String getConsentUrl() {
     return consentUrl;
   }
 
-  public void setConsentUrl(String consentUrl) {
+  public void setConsentUrl(@Nullable String consentUrl) {
     this.consentUrl = consentUrl;
   }
 

diff --git a/...s-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java b/...s-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
@@ -55,7 +55,7 @@ public StorageCredentialCacheKey(
       boolean allowedListAction,
       Set<String> allowedReadLocations,
       Set<String> allowedWriteLocations,
-      PolarisCallContext callContext) {
+      @Nullable PolarisCallContext callContext) {
     this.catalogId = entity.getCatalogId();
     this.storageConfigSerializedStr =
         entity
@@ -95,7 +95,7 @@ public Set<String> getAllowedWriteLocations() {
     return allowedWriteLocations;
   }
 
-  public PolarisCallContext getCallContext() {
+  public @Nullable PolarisCallContext getCallContext() {
     return callContext;
   }
 

diff --git a/...s-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java b/...s-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java
@@ -52,11 +52,11 @@ public String getFileIoImplClassName() {
     return "org.apache.iceberg.gcp.gcs.GCSFileIO";
   }
 
-  public void setGcpServiceAccount(String gcpServiceAccount) {
+  public void setGcpServiceAccount(@Nullable String gcpServiceAccount) {
     this.gcpServiceAccount = gcpServiceAccount;
   }
 
-  public String getGcpServiceAccount() {
+  public @Nullable String getGcpServiceAccount() {
     return gcpServiceAccount;
   }
 

diff --git a/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java b/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java
@@ -617,7 +617,7 @@ private void validateUpdateCatalogDiffOrThrow(
         currentEntity.getStorageConfigurationInfo();
     PolarisStorageConfigurationInfo newStorageConfig = newEntity.getStorageConfigurationInfo();
 
-    if (currentStorageConfig == null && newStorageConfig == null) {
+    if (currentStorageConfig == null || newStorageConfig == null) {
       return;
     }
 

diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java
@@ -221,7 +221,7 @@ public void initialize(String name, Map<String, String> properties) {
         this.catalogName);
 
     // Base location from catalogEntity is primary source of truth, otherwise fall through
-    // to the same key from the properties map, annd finally fall through to WAREHOUSE_LOCATION.
+    // to the same key from the properties map, and finally fall through to WAREHOUSE_LOCATION.
     String baseLocation =
         Optional.ofNullable(catalogEntity.getDefaultBaseLocation())
             .orElse(
@@ -249,11 +249,17 @@ public void initialize(String name, Map<String, String> properties) {
           ioImplClassName,
           storageConfigurationInfo);
     } else {
-      ioImplClassName = storageConfigurationInfo.getFileIoImplClassName();
-      LOGGER.debug(
-          "Resolved ioImplClassName {} for storageConfiguration {}",
-          ioImplClassName,
-          storageConfigurationInfo);
+      if (storageConfigurationInfo != null) {
+        ioImplClassName = storageConfigurationInfo.getFileIoImplClassName();
+        LOGGER.debug(
+            "Resolved ioImplClassName {} from storageConfiguration {}",
+            ioImplClassName,
+            storageConfigurationInfo);
+      } else {
+        LOGGER.warn(
+            "Cannot resolve property '{}' for null storageConfiguration.",
+            CatalogProperties.FILE_IO_IMPL);
+      }
     }
     this.closeableGroup = CallContext.getCurrentContext().closeables();
     closeableGroup.addCloseable(metricsReporter());