adutra commented on code in PR #282:
URL: https://github.com/apache/polaris/pull/282#discussion_r1759312054
##########
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:
I added these overrides just to avoid warnings like this one:
```
> Task :polaris-core:compileJava
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:35:
warning: (immutables:subtype) Constructor parameters should be better defined
on the same level of inheritance hierarchy, otherwise generated constructor
API would be unstable: parameter list can change the order of arguments. It is
better redeclare (override) each inherited attribute parameter in this abstract
value type to avoid this warning. Or better have constructor parameters defined
by only single supertype.
public abstract class AwsStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
^
1 warning
```
Is there a better way to handle this?
--
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]
