This is an automated email from the ASF dual-hosted git repository.
emaynard pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new bb472256 Fix minor issues like possible NPE, unused local variable,
typo. (#229)
bb472256 is described below
commit bb4722567846a03aed6c32dec55c4ae0fece9b0c
Author: Naveen Kumar <[email protected]>
AuthorDate: Tue Oct 8 04:08:04 2024 +0530
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]>
---
.../org/apache/polaris/core/entity/CatalogEntity.java | 1 -
.../storage/aws/AwsCredentialsStorageIntegration.java | 18 ++++++++----------
.../core/storage/aws/AwsStorageConfigurationInfo.java | 6 +++---
.../storage/azure/AzureStorageConfigurationInfo.java | 8 ++++----
.../core/storage/cache/StorageCredentialCacheKey.java | 4 ++--
.../core/storage/gcp/GcpStorageConfigurationInfo.java | 4 ++--
.../polaris/service/admin/PolarisAdminService.java | 2 +-
.../polaris/service/catalog/BasePolarisCatalog.java | 18 ++++++++++++------
8 files changed, 32 insertions(+), 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
index 4a622e30..924b2077 100644
---
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 class CatalogEntity extends PolarisEntity {
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/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
index d3d9bbad..32e028d0 100644
---
a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
+++
b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
@@ -22,7 +22,6 @@ import java.net.URI;
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 @@ public class AwsCredentialsStorageIntegration
.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 @@ public class AwsCredentialsStorageIntegration
arnPrefix +
StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/")));
final var bucket = arnPrefix + StorageUtil.getBucket(uri);
if (allowList) {
- bucketListStatmentBuilder
+ bucketListStatementBuilder
.computeIfAbsent(
bucket,
(String key) ->
@@ -125,7 +124,7 @@ public class AwsCredentialsStorageIntegration
"s3:prefix",
StorageUtil.concatFilePrefixes(trimLeadingSlash(uri.getPath()), "*", "/"));
}
- bucketGetLocationStatmentBuilder.computeIfAbsent(
+ bucketGetLocationStatementBuilder.computeIfAbsent(
bucket,
key ->
IamStatement.builder()
@@ -150,8 +149,8 @@ public class AwsCredentialsStorageIntegration
});
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 @@ public class AwsCredentialsStorageIntegration
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 @@ public class AwsCredentialsStorageIntegration
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/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
index 5bf43c91..c6b0f58e 100644
---
a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
+++
b/polaris-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 class AwsStorageConfigurationInfo extends
PolarisStorageConfigurationInfo
return externalId;
}
- public void setExternalId(String externalId) {
+ public void setExternalId(@Nullable String externalId) {
this.externalId = externalId;
}
diff --git
a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureStorageConfigurationInfo.java
b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureStorageConfigurationInfo.java
index f284dc64..325ad149 100644
---
a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureStorageConfigurationInfo.java
+++
b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureStorageConfigurationInfo.java
@@ -64,19 +64,19 @@ public class AzureStorageConfigurationInfo extends
PolarisStorageConfigurationIn
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/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
b/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
index bbe4d081..912a01b3 100644
---
a/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
+++
b/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
@@ -55,7 +55,7 @@ public class 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 class StorageCredentialCacheKey {
return allowedWriteLocations;
}
- public PolarisCallContext getCallContext() {
+ public @Nullable PolarisCallContext getCallContext() {
return callContext;
}
diff --git
a/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java
b/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java
index 717f15b8..d57f6d39 100644
---
a/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java
+++
b/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java
@@ -52,11 +52,11 @@ public class GcpStorageConfigurationInfo extends
PolarisStorageConfigurationInfo
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
index 023c96f1..c0dfcd81 100644
---
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 @@ public class PolarisAdminService {
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
index ff5dfeff..7b687e9a 100644
---
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 class BasePolarisCatalog extends
BaseMetastoreViewCatalog
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 class BasePolarisCatalog extends
BaseMetastoreViewCatalog
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());
