snazy commented on code in PR #282:
URL: https://github.com/apache/polaris/pull/282#discussion_r1759110643
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
@@ -94,26 +88,11 @@ public void testGetSubscopedCreds() {
.containsEntry(PolarisCredentialProperty.AWS_SECRET_KEY, "secretKey");
}
- @ParameterizedTest
- @ValueSource(strings = {AWS_PARTITION, "aws-cn", "aws-us-gov"})
Review Comment:
Maybe just comment out the last two ones for now?
Or probably better: override `validate()` in `StorageConfigurationOverride`,
which seems to be the only case where the locations are not validated.
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -19,89 +19,55 @@
package org.apache.polaris.core;
import java.util.Optional;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
-public class PolarisConfiguration<T> {
-
- public final String key;
- public final String description;
- public final T defaultValue;
- private final Optional<String> catalogConfigImpl;
- private final Class<T> typ;
-
- @SuppressWarnings("unchecked")
- public PolarisConfiguration(
- String key, String description, T defaultValue, Optional<String>
catalogConfig) {
- this.key = key;
- this.description = description;
- this.defaultValue = defaultValue;
- this.catalogConfigImpl = catalogConfig;
- this.typ = (Class<T>) defaultValue.getClass();
- }
+@PolarisImmutable
+public interface PolarisConfiguration<T> {
- public boolean hasCatalogConfig() {
- return catalogConfigImpl.isPresent();
- }
+ String key();
+
+ String description();
+
+ T defaultValue();
+
+ Optional<String> catalogConfig();
- public String catalogConfig() {
- return catalogConfigImpl.orElseThrow(
- () ->
- new IllegalStateException(
- "Attempted to read a catalog config key from a configuration
that doesn't have one."));
+ @Value.Derived
+ default boolean hasCatalogConfig() {
+ return catalogConfig().isPresent();
}
- T cast(Object value) {
- return this.typ.cast(value);
+ @Value.Lazy
Review Comment:
```suggestion
@Value.Derived
```
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java:
##########
@@ -61,50 +62,25 @@
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME)
@JsonSubTypes({
- @JsonSubTypes.Type(value = AwsStorageConfigurationInfo.class),
- @JsonSubTypes.Type(value = AzureStorageConfigurationInfo.class),
- @JsonSubTypes.Type(value = GcpStorageConfigurationInfo.class),
- @JsonSubTypes.Type(value = FileStorageConfigurationInfo.class),
+ @JsonSubTypes.Type(value = ImmutableAwsStorageConfigurationInfo.class),
Review Comment:
We're changing the types in this hierarchy quite a bit.
Can you add a test class (or tests in the individual sub-type test classes)
to ensure that the serialized JSON format is still the same?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -18,64 +18,77 @@
*/
package org.apache.polaris.core.storage.aws;
-import com.fasterxml.jackson.annotation.JsonCreator;
-import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.MoreObjects;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import java.util.List;
import java.util.regex.Pattern;
import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
-import org.jetbrains.annotations.NotNull;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+import org.immutables.value.Value.Check;
import org.jetbrains.annotations.Nullable;
/** Aws Polaris Storage Configuration information */
-public class AwsStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
+@PolarisImmutable
+@JsonSerialize(as = ImmutableAwsStorageConfigurationInfo.class)
+@JsonDeserialize(as = ImmutableAwsStorageConfigurationInfo.class)
+public abstract class AwsStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
// 5 is the approximate max allowed locations for the size of AccessPolicy
when LIST is required
// for allowed read and write locations for subscoping creds.
- @JsonIgnore private static final int MAX_ALLOWED_LOCATIONS = 5;
+ private static final int MAX_ALLOWED_LOCATIONS = 5;
// Technically, it should be
^arn:(aws|aws-cn|aws-us-gov):iam::\d{12}:role/.+$,
- @JsonIgnore public static final String ROLE_ARN_PATTERN =
"^arn:aws:iam::\\d{12}:role/.+$";
+ private static final String ROLE_ARN_PATTERN =
"^arn:aws:iam::\\d{12}:role/.+$";
- // AWS role to be assumed
- private final @NotNull String roleARN;
-
- // AWS external ID, optional
- @JsonProperty(value = "externalId", required = false)
- private @Nullable String externalId = null;
-
- /** User ARN for the service principal */
- @JsonProperty(value = "userARN", required = false)
- private @Nullable String userARN = null;
+ public static AwsStorageConfigurationInfo of(Iterable<String>
allowedLocations, String roleARN) {
+ return of(allowedLocations, roleARN, null);
+ }
- @JsonCreator
- public AwsStorageConfigurationInfo(
- @JsonProperty(value = "storageType", required = true) @NotNull
StorageType storageType,
- @JsonProperty(value = "allowedLocations", required = true) @NotNull
- List<String> allowedLocations,
- @JsonProperty(value = "roleARN", required = true) @NotNull String
roleARN) {
- this(storageType, allowedLocations, roleARN, null);
+ public static AwsStorageConfigurationInfo of(
+ Iterable<String> allowedLocations, String roleARN, @Nullable String
externalId) {
+ return ImmutableAwsStorageConfigurationInfo.builder()
+ .allowedLocations(allowedLocations)
+ .roleARN(roleARN)
+ .externalId(externalId)
+ .build();
}
- public AwsStorageConfigurationInfo(
- @NotNull StorageType storageType,
- @NotNull List<String> allowedLocations,
- @NotNull String roleARN,
- @Nullable String externalId) {
- super(storageType, allowedLocations);
- this.roleARN = roleARN;
- this.externalId = externalId;
- validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
+ @Override
+ public abstract List<String> getAllowedLocations();
Review Comment:
Nit: I think those overrides can go away.
--
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]
