Skip to content

S3 support classes take Supplier<ProfileFile> #3653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dave-fn merged 2 commits into from
Jan 6, 2023
Merged

S3 support classes take Supplier<ProfileFile> #3653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
310b856
S3 support classes take Supplier<ProfileFile>
dave-fn Dec 20, 2022
0cd22d0
Review comments
dave-fn Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 44 additions & 12 deletions services/s3/src/main/java/software/amazon/awssdk/services/s3/S3Configuration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@

package software.amazon.awssdk.services.s3;

import java.util.Optional;
import java.util.function.Supplier;
import software.amazon.awssdk.annotations.Immutable;
import software.amazon.awssdk.annotations.NotThreadSafe;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.annotations.ThreadSafe;
import software.amazon.awssdk.core.ServiceConfiguration;
import software.amazon.awssdk.profiles.ProfileFile;
import software.amazon.awssdk.profiles.ProfileFileSupplier;
import software.amazon.awssdk.profiles.ProfileFileSystemSetting;
import software.amazon.awssdk.services.s3.internal.FieldWithDefault;
import software.amazon.awssdk.services.s3.internal.settingproviders.DisableMultiRegionProviderChain;
Expand Down Expand Up @@ -66,9 +69,9 @@ public final class S3Configuration implements ServiceConfiguration, ToCopyableBu
private final FieldWithDefault<Boolean> dualstackEnabled;
private final FieldWithDefault<Boolean> checksumValidationEnabled;
private final FieldWithDefault<Boolean> chunkedEncodingEnabled;
private final FieldWithDefault<Boolean> useArnRegionEnabled;
private final FieldWithDefault<Boolean> multiRegionEnabled;
private final FieldWithDefault<ProfileFile> profileFile;
private final Boolean useArnRegionEnabled;
private final Boolean multiRegionEnabled;
private final FieldWithDefault<Supplier<ProfileFile>> profileFile;
private final FieldWithDefault<String> profileName;

private S3Configuration(DefaultS3ServiceConfigurationBuilder builder) {
Expand All @@ -78,11 +81,11 @@ private S3Configuration(DefaultS3ServiceConfigurationBuilder builder) {
this.checksumValidationEnabled = FieldWithDefault.create(builder.checksumValidationEnabled,
DEFAULT_CHECKSUM_VALIDATION_ENABLED);
this.chunkedEncodingEnabled = FieldWithDefault.create(builder.chunkedEncodingEnabled, DEFAULT_CHUNKED_ENCODING_ENABLED);
this.profileFile = FieldWithDefault.createLazy(builder.profileFile, ProfileFile::defaultProfileFile);
this.profileFile = FieldWithDefault.create(builder.profileFile, ProfileFile::defaultProfileFile);
this.profileName = FieldWithDefault.create(builder.profileName,
ProfileFileSystemSetting.AWS_PROFILE.getStringValueOrThrow());
this.useArnRegionEnabled = FieldWithDefault.createLazy(builder.useArnRegionEnabled, this::resolveUseArnRegionEnabled);
this.multiRegionEnabled = FieldWithDefault.createLazy(builder.multiRegionEnabled, this::resolveMultiRegionEnabled);
this.useArnRegionEnabled = builder.useArnRegionEnabled;
this.multiRegionEnabled = builder.multiRegionEnabled;
Comment on lines -84 to +88
Copy link
Contributor Author

@dave-fn dave-fn Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed so that these values are resolved per invocation, based on the active ProfileFile.


if (accelerateModeEnabled() && pathStyleAccessEnabled()) {
throw new IllegalArgumentException("Accelerate mode cannot be used with path style addressing");
Expand Down Expand Up @@ -189,7 +192,8 @@ public boolean chunkedEncodingEnabled() {
* @return True if a different region in the ARN can be used.
*/
public boolean useArnRegionEnabled() {
return useArnRegionEnabled.value();
return Optional.ofNullable(useArnRegionEnabled)
.orElseGet(this::resolveUseArnRegionEnabled);
}

/**
Expand All @@ -198,19 +202,20 @@ public boolean useArnRegionEnabled() {
* @return True if multi-region ARNs is enabled.
*/
public boolean multiRegionEnabled() {
return multiRegionEnabled.value();
return Optional.ofNullable(multiRegionEnabled)
.orElseGet(this::resolveMultiRegionEnabled);
}

@Override
public Builder toBuilder() {
return builder()
.dualstackEnabled(dualstackEnabled.valueOrNullIfDefault())
.multiRegionEnabled(multiRegionEnabled.valueOrNullIfDefault())
.multiRegionEnabled(multiRegionEnabled)
.accelerateModeEnabled(accelerateModeEnabled.valueOrNullIfDefault())
.pathStyleAccessEnabled(pathStyleAccessEnabled.valueOrNullIfDefault())
.checksumValidationEnabled(checksumValidationEnabled.valueOrNullIfDefault())
.chunkedEncodingEnabled(chunkedEncodingEnabled.valueOrNullIfDefault())
.useArnRegionEnabled(useArnRegionEnabled.valueOrNullIfDefault())
.useArnRegionEnabled(useArnRegionEnabled)
.profileFile(profileFile.valueOrNullIfDefault())
.profileName(profileName.valueOrNullIfDefault());
}
Expand Down Expand Up @@ -325,6 +330,19 @@ public interface Builder extends CopyableBuilder<Builder, S3Configuration> {
*/
Builder profileFile(ProfileFile profileFile);

Supplier<ProfileFile> profileFileSupplier();

/**
* The supplier of profile file instances that should be consulted to determine the default value of
* {@link #useArnRegionEnabled(Boolean)} or {@link #multiRegionEnabled(Boolean)}.
* This is not used, if those parameters are configured on the builder.
*
* <p>
* By default, the {@link ProfileFile#defaultProfileFile()} is used.
* </p>
*/
Builder profileFile(Supplier<ProfileFile> profileFile);

String profileName();

/**
Expand All @@ -347,7 +365,7 @@ static final class DefaultS3ServiceConfigurationBuilder implements Builder {
private Boolean chunkedEncodingEnabled;
private Boolean useArnRegionEnabled;
private Boolean multiRegionEnabled;
private ProfileFile profileFile;
private Supplier<ProfileFile> profileFile;
private String profileName;

@Override
Expand Down Expand Up @@ -449,11 +467,25 @@ public Builder multiRegionEnabled(Boolean multiRegionEnabled) {

@Override
public ProfileFile profileFile() {
return profileFile;
return Optional.ofNullable(profileFile)
.map(Supplier::get)
.orElse(null);
}

@Override
public Builder profileFile(ProfileFile profileFile) {
return profileFile(Optional.ofNullable(profileFile)
.map(ProfileFileSupplier::fixedProfileFile)
.orElse(null));
}

@Override
public Supplier<ProfileFile> profileFileSupplier() {
return profileFile;
}

@Override
public Builder profileFile(Supplier<ProfileFile> profileFile) {
this.profileFile = profileFile;
return this;
}
Expand Down
9 changes: 8 additions & 1 deletion .../amazon/awssdk/services/s3/internal/settingproviders/DisableMultiRegionProviderChain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.function.Supplier;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.profiles.ProfileFile;
import software.amazon.awssdk.profiles.ProfileFileSystemSetting;
Expand Down Expand Up @@ -50,7 +51,7 @@ private DisableMultiRegionProviderChain(List<DisableMultiRegionProvider> provide
* </ol>
*/
public static DisableMultiRegionProviderChain create() {
return create(ProfileFile.defaultProfileFile(),
return create(ProfileFile::defaultProfileFile,
ProfileFileSystemSetting.AWS_PROFILE.getStringValueOrThrow());
}

Expand All @@ -60,6 +61,12 @@ public static DisableMultiRegionProviderChain create(ProfileFile profileFile, St
ProfileDisableMultiRegionProvider.create(profileFile, profileName)));
}

public static DisableMultiRegionProviderChain create(Supplier<ProfileFile> profileFile, String profileName) {
return new DisableMultiRegionProviderChain(Arrays.asList(
SystemsSettingsDisableMultiRegionProvider.create(),
ProfileDisableMultiRegionProvider.create(profileFile, profileName)));
}

@Override
public Optional<Boolean> resolve() {
for (DisableMultiRegionProvider provider : providers) {
Expand Down
4 changes: 4 additions & 0 deletions ...mazon/awssdk/services/s3/internal/settingproviders/ProfileDisableMultiRegionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ public static ProfileDisableMultiRegionProvider create(ProfileFile profileFile,
return new ProfileDisableMultiRegionProvider(() -> profileFile, profileName);
}

public static ProfileDisableMultiRegionProvider create(Supplier<ProfileFile> profileFile, String profileName) {
return new ProfileDisableMultiRegionProvider(profileFile, profileName);
}

@Override
public Optional<Boolean> resolve() {
return profileFile.get()
Expand Down
6 changes: 5 additions & 1 deletion ...ware/amazon/awssdk/services/s3/internal/settingproviders/ProfileUseArnRegionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
@SdkInternalApi
public final class ProfileUseArnRegionProvider implements UseArnRegionProvider {
/**
* Property name for specifying whether or not use arn region should be enabled.
* Property name for specifying whether or not to use arn region should be enabled.
*/
private static final String AWS_USE_ARN_REGION = "s3_use_arn_region";

Expand All @@ -49,6 +49,10 @@ public static ProfileUseArnRegionProvider create(ProfileFile profileFile, String
return new ProfileUseArnRegionProvider(() -> profileFile, profileName);
}

public static ProfileUseArnRegionProvider create(Supplier<ProfileFile> profileFile, String profileName) {
return new ProfileUseArnRegionProvider(profileFile, profileName);
}

@Override
public Optional<Boolean> resolveUseArnRegion() {
return profileFile.get()
Expand Down
8 changes: 7 additions & 1 deletion ...ftware/amazon/awssdk/services/s3/internal/settingproviders/UseArnRegionProviderChain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.function.Supplier;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.profiles.ProfileFile;
import software.amazon.awssdk.profiles.ProfileFileSystemSetting;
Expand Down Expand Up @@ -49,7 +50,7 @@ private UseArnRegionProviderChain(List<UseArnRegionProvider> providers) {
* </ol>
*/
public static UseArnRegionProviderChain create() {
return create(ProfileFile.defaultProfileFile(),
return create(ProfileFile::defaultProfileFile,
ProfileFileSystemSetting.AWS_PROFILE.getStringValueOrThrow());
}

Expand All @@ -58,6 +59,11 @@ public static UseArnRegionProviderChain create(ProfileFile profileFile, String p
ProfileUseArnRegionProvider.create(profileFile, profileName)));
}

public static UseArnRegionProviderChain create(Supplier<ProfileFile> profileFile, String profileName) {
return new UseArnRegionProviderChain(Arrays.asList(SystemsSettingsUseArnRegionProvider.create(),
ProfileUseArnRegionProvider.create(profileFile, profileName)));
}

@Override
public Optional<Boolean> resolveUseArnRegion() {
for (UseArnRegionProvider provider : providers) {
Expand Down
28 changes: 28 additions & 0 deletions services/s3/src/test/java/software/amazon/awssdk/services/s3/S3ConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,32 @@ public void multiRegionEnabled_notDisabledInProviders_shouldResolveToTrue() {
assertThat(config.multiRegionEnabled()).isEqualTo(true);
}

@Test
public void multiRegionEnabled_enabledInCProfile_shouldResolveToConfigCorrectlyOncePerCall() {
S3Configuration config = S3Configuration.builder().build();

String trueProfileConfig = getClass().getResource("internal/settingproviders/ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), trueProfileConfig);
assertThat(config.multiRegionEnabled()).isEqualTo(false);

System.clearProperty(AWS_CONFIG_FILE.property());
String falseProfileConfig = getClass().getResource("internal/settingproviders/ProfileFile_false").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), falseProfileConfig);
assertThat(config.multiRegionEnabled()).isEqualTo(true);
}

@Test
public void useArnRegionEnabled_enabledInCProfile_shouldResolveToConfigCorrectlyOncePerCall() {
S3Configuration config = S3Configuration.builder().build();

String trueProfileConfig = getClass().getResource("internal/settingproviders/ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), trueProfileConfig);
assertThat(config.useArnRegionEnabled()).isEqualTo(true);

System.clearProperty(AWS_CONFIG_FILE.property());
String falseProfileConfig = getClass().getResource("internal/settingproviders/ProfileFile_false").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), falseProfileConfig);
assertThat(config.useArnRegionEnabled()).isEqualTo(false);
}

}
28 changes: 21 additions & 7 deletions ...zon/awssdk/services/s3/internal/settingproviders/DisableMultiRegionProviderChainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,23 @@
import org.junit.jupiter.api.Test;
import software.amazon.awssdk.testutils.EnvironmentVariableHelper;

public class DisableMultiRegionProviderChainTest {
class DisableMultiRegionProviderChainTest {
private final EnvironmentVariableHelper helper = new EnvironmentVariableHelper();

@AfterEach
public void clearSystemProperty() {
void clearSystemProperty() {
System.clearProperty(AWS_S3_DISABLE_MULTIREGION_ACCESS_POINTS.property());
System.clearProperty(AWS_CONFIG_FILE.property());
helper.reset();
}

@Test
public void notSpecified_shouldReturnEmptyOptional() {
void notSpecified_shouldReturnEmptyOptional() {
assertThat(DisableMultiRegionProviderChain.create().resolve()).isEqualTo(Optional.empty());
}

@Test
public void specifiedInBothProviders_systemPropertiesShouldTakePrecedence() {
void specifiedInBothProviders_systemPropertiesShouldTakePrecedence() {
System.setProperty(AWS_S3_DISABLE_MULTIREGION_ACCESS_POINTS.property(), "false");
String configFile = getClass().getResource("ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), configFile);
Expand All @@ -49,7 +49,7 @@ public void specifiedInBothProviders_systemPropertiesShouldTakePrecedence() {
}

@Test
public void systemPropertiesThrowException_shouldUseConfigFile() {
void systemPropertiesThrowException_shouldUseConfigFile() {
System.setProperty(AWS_S3_DISABLE_MULTIREGION_ACCESS_POINTS.property(), "foobar");
String configFile = getClass().getResource("ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), configFile);
Expand All @@ -58,15 +58,29 @@ public void systemPropertiesThrowException_shouldUseConfigFile() {
}

@Test
public void systemPropertiesNotSpecified_shouldUseConfigFile() {
void systemPropertiesNotSpecified_shouldUseConfigFile() {
String configFile = getClass().getResource("ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), configFile);

assertThat(DisableMultiRegionProviderChain.create().resolve()).isEqualTo(Optional.of(Boolean.TRUE));
}

@Test
public void bothProvidersThrowException_shouldReturnEmpty() {
void resolve_systemPropertiesNotSpecified_shouldResolveOncePerCall() {
String trueConfigFile = getClass().getResource("ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), trueConfigFile);

assertThat(DisableMultiRegionProviderChain.create().resolve()).isEqualTo(Optional.of(Boolean.TRUE));

System.clearProperty(AWS_CONFIG_FILE.property());
String falseConfigFile = getClass().getResource("ProfileFile_false").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), falseConfigFile);

assertThat(DisableMultiRegionProviderChain.create().resolve()).isEqualTo(Optional.of(Boolean.FALSE));
}

@Test
void bothProvidersThrowException_shouldReturnEmpty() {
System.setProperty(AWS_S3_DISABLE_MULTIREGION_ACCESS_POINTS.property(), "foobar");
String configFile = getClass().getResource("ProfileFile_unsupportedValue").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), configFile);
Expand Down
15 changes: 15 additions & 0 deletions ...n/awssdk/services/s3/internal/settingproviders/ProfileDisableMultiRegionProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package software.amazon.awssdk.services.s3.internal.settingproviders;

import static java.lang.Boolean.FALSE;
import static java.lang.Boolean.TRUE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand Down Expand Up @@ -72,6 +73,20 @@ public void unsupportedValue_shouldThrowException() {
assertThatThrownBy(() -> provider.resolve()).isInstanceOf(IllegalArgumentException.class);
}

@Test
public void resolve_specifiedMultipleValuesInConfigFile_shouldResolveOncePerCall() {
String trueConfigFile = getClass().getResource("ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), trueConfigFile);

assertThat(provider.resolve()).isEqualTo(Optional.of(TRUE));

System.clearProperty(AWS_CONFIG_FILE.property());
String falseConfigFile = getClass().getResource("ProfileFile_false").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), falseConfigFile);

assertThat(provider.resolve()).isEqualTo(Optional.of(FALSE));
}

@Test
public void specifiedInOverrideConfig_shouldUse() {
ExecutionInterceptor interceptor = Mockito.spy(AbstractExecutionInterceptor.class);
Expand Down
14 changes: 14 additions & 0 deletions .../amazon/awssdk/services/s3/internal/settingproviders/ProfileUseArnRegionProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,20 @@ public void commaNoSpace_shouldResolveCorrectly() {
assertThat(provider.resolveUseArnRegion()).isEqualTo(Optional.of(FALSE));
}

@Test
public void resolveUseArnRegion_specifiedMultipleValuesInConfigFile_shouldResolveOncePerCall() {
String trueConfigFile = getClass().getResource("ProfileFile_true").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), trueConfigFile);

assertThat(provider.resolveUseArnRegion()).isEqualTo(Optional.of(TRUE));

System.clearProperty(AWS_CONFIG_FILE.property());
String falseConfigFile = getClass().getResource("ProfileFile_false").getFile();
System.setProperty(AWS_CONFIG_FILE.property(), falseConfigFile);

assertThat(provider.resolveUseArnRegion()).isEqualTo(Optional.of(FALSE));
}

@Test
public void specifiedInOverrideConfig_shouldUse() {
ExecutionInterceptor interceptor = Mockito.spy(AbstractExecutionInterceptor.class);
Expand Down
Loading