Skip to content

Commit 15f23ca

Browse files
poojanilangekarpoojanilangekar
authored
Make PolarisConfiguration member variables private (#2007)
* Make PolarisConfiguration members private * Make methods final
1 parent ccc97bf commit 15f23ca

File tree

10 files changed

+57
-45
lines changed

10 files changed

+57
-45
lines changed

‎polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public static void enforceFeatureEnabledOrThrow(
5555
.getConfigurationStore()
5656
.getConfiguration(callContext.getRealmContext(), featureConfig);
5757
if (!enabled) {
58-
throw new UnsupportedOperationException("Feature not enabled: " + featureConfig.key);
58+
throw new UnsupportedOperationException("Feature not enabled: " + featureConfig.key());
5959
}
6060
}
6161

@@ -211,7 +211,7 @@ public static void enforceFeatureEnabledOrThrow(
211211
.key("STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS")
212212
.description(
213213
"How long to store storage credentials in the local cache. This should be less than "
214-
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key)
214+
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key())
215215
.defaultValue(30 * 60) // 30 minutes
216216
.buildFeatureConfiguration();
217217

‎polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,9 @@ public abstract class PolarisConfiguration<T> {
3939

4040
private static final List<PolarisConfiguration<?>> allConfigurations = new ArrayList<>();
4141

42-
public final String key;
43-
public final String description;
44-
public final T defaultValue;
42+
private final String key;
43+
private final String description;
44+
private final T defaultValue;
4545
private final Optional<String> catalogConfigImpl;
4646
private final Optional<String> catalogConfigUnsafeImpl;
4747
private final Class<T> typ;
@@ -98,22 +98,22 @@ protected PolarisConfiguration(
9898
this.typ = (Class<T>) defaultValue.getClass();
9999
}
100100

101-
public boolean hasCatalogConfig() {
101+
public final boolean hasCatalogConfig() {
102102
return catalogConfigImpl.isPresent();
103103
}
104104

105-
public String catalogConfig() {
105+
public final String catalogConfig() {
106106
return catalogConfigImpl.orElseThrow(
107107
() ->
108108
new IllegalStateException(
109109
"Attempted to read a catalog config key from a configuration that doesn't have one."));
110110
}
111111

112-
public boolean hasCatalogConfigUnsafe() {
112+
public final boolean hasCatalogConfigUnsafe() {
113113
return catalogConfigUnsafeImpl.isPresent();
114114
}
115115

116-
public String catalogConfigUnsafe() {
116+
public final String catalogConfigUnsafe() {
117117
return catalogConfigUnsafeImpl.orElseThrow(
118118
() ->
119119
new IllegalStateException(
@@ -124,6 +124,18 @@ T cast(Object value) {
124124
return this.typ.cast(value);
125125
}
126126

127+
public final String key() {
128+
return key;
129+
}
130+
131+
public final String description() {
132+
return description;
133+
}
134+
135+
public final T defaultValue() {
136+
return defaultValue;
137+
}
138+
127139
public static class Builder<T> {
128140
private String key;
129141
private String description;

‎polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,18 +71,18 @@ public interface PolarisConfigurationStore {
7171
*/
7272
private <T> @Nonnull T tryCast(PolarisConfiguration<T> config, Object value) {
7373
if (value == null) {
74-
return config.defaultValue;
74+
return config.defaultValue();
7575
}
7676

77-
if (config.defaultValue instanceof Boolean) {
77+
if (config.defaultValue() instanceof Boolean) {
7878
return config.cast(Boolean.valueOf(String.valueOf(value)));
79-
} else if (config.defaultValue instanceof Integer) {
79+
} else if (config.defaultValue() instanceof Integer) {
8080
return config.cast(Integer.valueOf(String.valueOf(value)));
81-
} else if (config.defaultValue instanceof Long) {
81+
} else if (config.defaultValue() instanceof Long) {
8282
return config.cast(Long.valueOf(String.valueOf(value)));
83-
} else if (config.defaultValue instanceof Double) {
83+
} else if (config.defaultValue() instanceof Double) {
8484
return config.cast(Double.valueOf(String.valueOf(value)));
85-
} else if (config.defaultValue instanceof List<?>) {
85+
} else if (config.defaultValue() instanceof List<?>) {
8686
return config.cast(new ArrayList<>((List<?>) value));
8787
} else {
8888
return config.cast(value);
@@ -99,7 +99,7 @@ public interface PolarisConfigurationStore {
9999
*/
100100
default <T> @Nonnull T getConfiguration(
101101
@Nonnull RealmContext realmContext, PolarisConfiguration<T> config) {
102-
T result = getConfiguration(realmContext, config.key, config.defaultValue);
102+
T result = getConfiguration(realmContext, config.key(), config.defaultValue());
103103
return tryCast(config, result);
104104
}
105105

‎polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,8 @@ private long maxCacheDurationMs() {
9191
throw new IllegalArgumentException(
9292
String.format(
9393
"%s should be less than %s",
94-
FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS.key,
95-
FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS.key));
94+
FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS.key(),
95+
FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS.key()));
9696
} else {
9797
return cacheDurationSeconds * 1000L;
9898
}

‎polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ public void testConfigsCanBeCastedFromString() {
5656
@Override
5757
public <T> @Nullable T getConfiguration(@Nonnull RealmContext ctx, String configName) {
5858
for (PolarisConfiguration<?> c : configs) {
59-
if (c.key.equals(configName)) {
60-
return (T) String.valueOf(c.defaultValue);
59+
if (c.key().equals(configName)) {
60+
return (T) String.valueOf(c.defaultValue());
6161
}
6262
}
6363

@@ -71,7 +71,7 @@ public void testConfigsCanBeCastedFromString() {
7171
// Ensure that we can fetch all the configs and that the value is what we expect, which
7272
// is the config's default value based on how we've implemented PolarisConfigurationStore above.
7373
for (PolarisConfiguration<?> c : configs) {
74-
Assertions.assertEquals(c.defaultValue, store.getConfiguration(testRealmContext, c));
74+
Assertions.assertEquals(c.defaultValue(), store.getConfiguration(testRealmContext, c));
7575
}
7676
}
7777

‎runtime/service/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -217,26 +217,26 @@ public ProductionReadinessCheck checkInsecureStorageSettings(
217217
var insecure = FeatureConfiguration.ALLOW_INSECURE_STORAGE_TYPES;
218218

219219
var errors = new ArrayList<Error>();
220-
if (Boolean.parseBoolean(featureConfiguration.defaults().get(insecure.key))) {
220+
if (Boolean.parseBoolean(featureConfiguration.defaults().get(insecure.key()))) {
221221
errors.add(
222222
Error.ofSevere(
223223
"Must not enable a configuration that exposes known and severe security risks: "
224-
+ insecure.description,
225-
format("polaris.features.\"%s\"", insecure.key)));
224+
+ insecure.description(),
225+
format("polaris.features.\"%s\"", insecure.key())));
226226
}
227227

228228
featureConfiguration
229229
.realmOverrides()
230230
.forEach(
231231
(realmId, overrides) -> {
232-
if (Boolean.parseBoolean(overrides.overrides().get(insecure.key))) {
232+
if (Boolean.parseBoolean(overrides.overrides().get(insecure.key()))) {
233233
errors.add(
234234
Error.ofSevere(
235235
"Must not enable a configuration that exposes known and severe security risks: "
236-
+ insecure.description,
236+
+ insecure.description(),
237237
format(
238238
"polaris.features.realm-overrides.\"%s\".overrides.\"%s\"",
239-
realmId, insecure.key)));
239+
realmId, insecure.key())));
240240
}
241241
});
242242

@@ -245,7 +245,7 @@ public ProductionReadinessCheck checkInsecureStorageSettings(
245245
var defaults = featureConfiguration.parseDefaults(mapper);
246246
var realmOverrides = featureConfiguration.parseRealmOverrides(mapper);
247247
@SuppressWarnings("unchecked")
248-
var supported = (List<String>) defaults.getOrDefault(storageTypes.key, List.of());
248+
var supported = (List<String>) defaults.getOrDefault(storageTypes.key(), List.of());
249249
supported.stream()
250250
.filter(n -> !IcebergPropertiesValidation.safeStorageType(n))
251251
.forEach(
@@ -255,11 +255,11 @@ public ProductionReadinessCheck checkInsecureStorageSettings(
255255
format(
256256
"The storage type '%s' is considered insecure and exposes the service to severe security risks!",
257257
t),
258-
format("polaris.features.\"%s\"", storageTypes.key))));
258+
format("polaris.features.\"%s\"", storageTypes.key()))));
259259
realmOverrides.forEach(
260260
(realmId, overrides) -> {
261261
@SuppressWarnings("unchecked")
262-
var s = (List<String>) overrides.getOrDefault(storageTypes.key, List.of());
262+
var s = (List<String>) overrides.getOrDefault(storageTypes.key(), List.of());
263263
s.stream()
264264
.filter(n -> !IcebergPropertiesValidation.safeStorageType(n))
265265
.forEach(
@@ -271,7 +271,7 @@ public ProductionReadinessCheck checkInsecureStorageSettings(
271271
t),
272272
format(
273273
"polaris.features.realm-overrides.\"%s\".overrides.\"%s\"",
274-
realmId, storageTypes.key))));
274+
realmId, storageTypes.key()))));
275275
});
276276
return errors.isEmpty()
277277
? ProductionReadinessCheck.OK

‎runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisOverlappingTableTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ void testInvalidSetupsForObjectStorageLocation(
340340
"true",
341341
"SUPPORTED_CATALOG_STORAGE_TYPES",
342342
List.of("FILE", "S3"),
343-
OPTIMIZED_SIBLING_CHECK.key,
343+
OPTIMIZED_SIBLING_CHECK.key(),
344344
"false");
345345

346346
TestServices services =
@@ -369,7 +369,7 @@ public void testHashedTableLocations(@TempDir Path tempDir) {
369369
"true",
370370
"SUPPORTED_CATALOG_STORAGE_TYPES",
371371
List.of("FILE", "S3"),
372-
OPTIMIZED_SIBLING_CHECK.key,
372+
OPTIMIZED_SIBLING_CHECK.key(),
373373
"true");
374374
Map<String, String> hashedAndOverlapButNoOptimizedCatalog =
375375
Map.of(

‎runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1956,7 +1956,7 @@ public void testDropTableWithPurgeDisabled() {
19561956
// Attempt to drop the table:
19571957
Assertions.assertThatThrownBy(() -> noPurgeCatalog.dropTable(TABLE, true))
19581958
.isInstanceOf(ForbiddenException.class)
1959-
.hasMessageContaining(FeatureConfiguration.DROP_WITH_PURGE_ENABLED.key);
1959+
.hasMessageContaining(FeatureConfiguration.DROP_WITH_PURGE_ENABLED.key());
19601960
}
19611961

19621962
private TableMetadata createSampleTableMetadata(String tableLocation) {

‎service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -949,8 +949,8 @@ private String applyDefaultLocationObjectStoragePrefix(
949949
throw new IllegalStateException(
950950
String.format(
951951
"The configuration %s is enabled, but %s is not enabled",
952-
FeatureConfiguration.DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED.key,
953-
FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.key));
952+
FeatureConfiguration.DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED.key(),
953+
FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.key()));
954954
} else if (!allowTableLocationOverlap) {
955955
// TODO consider doing this check any time ALLOW_EXTERNAL_TABLE_LOCATION is enabled, not just
956956
// here
@@ -961,17 +961,17 @@ private String applyDefaultLocationObjectStoragePrefix(
961961
+ " performed, but only within each namespace. However, %s is enabled, which indicates"
962962
+ " that tables may be created outside of their parent namespace. This is not a safe"
963963
+ " combination of configurations.",
964-
FeatureConfiguration.ALLOW_TABLE_LOCATION_OVERLAP.key,
965-
FeatureConfiguration.OPTIMIZED_SIBLING_CHECK.key,
966-
FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.key));
964+
FeatureConfiguration.ALLOW_TABLE_LOCATION_OVERLAP.key(),
965+
FeatureConfiguration.OPTIMIZED_SIBLING_CHECK.key(),
966+
FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.key()));
967967
} else if (!loggedPrefixOverlapWarning.getAndSet(true)) {
968968
LOGGER.warn(
969969
"A table is being created with {} and {} enabled, but with {} disabled. "
970970
+ "This is a safe combination of configurations which may prevent table overlap, but only if the "
971971
+ "underlying persistence actually implements %s. Exercise caution.",
972-
FeatureConfiguration.DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED.key,
973-
FeatureConfiguration.OPTIMIZED_SIBLING_CHECK.key,
974-
FeatureConfiguration.ALLOW_TABLE_LOCATION_OVERLAP.key);
972+
FeatureConfiguration.DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED.key(),
973+
FeatureConfiguration.OPTIMIZED_SIBLING_CHECK.key(),
974+
FeatureConfiguration.ALLOW_TABLE_LOCATION_OVERLAP.key());
975975
}
976976
return buildPrefixedLocation(tableIdentifier);
977977
} else {
@@ -2460,7 +2460,7 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) {
24602460
"Unable to purge entity: %s. To enable this feature, set the Polaris configuration %s "
24612461
+ "or the catalog configuration %s",
24622462
identifier.name(),
2463-
FeatureConfiguration.DROP_WITH_PURGE_ENABLED.key,
2463+
FeatureConfiguration.DROP_WITH_PURGE_ENABLED.key(),
24642464
FeatureConfiguration.DROP_WITH_PURGE_ENABLED.catalogConfig()));
24652465
}
24662466
}

‎service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ public static AccessConfig refreshAccessConfig(
8888
boolean skipCredentialSubscopingIndirection =
8989
configurationStore.getConfiguration(
9090
callContext.getRealmContext(),
91-
FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.key,
92-
FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.defaultValue);
91+
FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.key(),
92+
FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.defaultValue());
9393
if (skipCredentialSubscopingIndirection) {
9494
LOGGER
9595
.atDebug()

0 commit comments

Comments
 (0)