Fixing issues #115 and #161 (#174)

* fixed issues #3 and #5 and little cleanup

* Configuration description improvements

README.md

@@ -122,12 +122,35 @@ on I/O, up to the limits of your network connection.
 
 ### Configuration
 
-The read-ahead buffer asynchronously prefetches `n` sequential fragments of `m` bytes from S3. The
-values of `n` and `m` can be configured to your needs by using command line properties or environment variables.
+There are two types of configuration parameters: local and system. Local configuration parameters are set when creating
+a file system calling _FileSystems.newFileSystem(uri, env)_ or directly calling _newFileSystem(uri, env)_ on a
+_S3FileSystemProvider_ or _S3XFileSystemProvider_ instance. System configuration parameters can be set like local paramenters
+or as environment variables or java system properties. Therefore these parameters apply to all file systems created with
+S3 and S3X providers. Some parameters can have both system and local scopes; for these, local values overwrite the system
+values.
 
 If no configuration is supplied the values in `resources/s3-nio-spi.properties` are used. Currently, 50 fragments of 5MB.
 Each fragment is downloaded concurrently on a unique thread.
 
+#### Local parameters
+**aws.region** specifies the default region for API calls
+**aws.accessKey** specifies the key id to use for authentication
+**aws.secretAccessKey** specifies the secret to use for authentication
+
+**s3.spi.read.fragment-number** buffer asynchronously prefetches `n` sequential fragments from S3 (currently 50)
+**s3.spi.read.fragment-size** size of each fragment (currently 5MB)
+**s3.spi.endpoint** the endpoint to use to access the bucket; this is extracted from the uri by the S3X provider
+**s3.spi.force-path-style** (true|false) to set if path-style shall be used instead of host style; unless otherwise
+specified, this is undefined for the S3 provider and set to true when using the S3X provider.
+
+#### System parameter ####
+**aws.region** specifies the default region for API calls
+**aws.accessKey** specifies the key id to use for authentication
+**aws.secretAccessKey** specifies the secret to use for authentication
+
+**s3.spi.read.fragment-number** buffer asynchronously prefetches `n` sequential fragments from S3 (currently 50)
+**s3.spi.read.fragment-size** size of each fragment (currently 5MB)
+
 #### Environment Variables
 
 You may use `S3_SPI_READ_MAX_FRAGMENT_NUMBER` and `S3_SPI_READ_MAX_FRAGMENT_SIZE` to set the maximum umber of cached
@@ -152,9 +175,10 @@ java -Djava.ext.dirs=$JAVA_HOME/jre/lib/ext:<location-of-this-spi-jar> -Ds3.spi.
 
 Configurations use the following order of precedence from highest to lowest:
 
-1. Java properties
-2. Environment variables
-3. Default values
+1. Local parameters
+2. Java properties
+3. Environment variables
+4. Default values
 
 #### S3 limits
 

src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java

@@ -294,13 +294,14 @@ private S3Client clientForRegion(String regionName) {
         logger.debug("bucket region is: '{}'", region.id());
 
         S3ClientBuilder clientBuilder =  S3Client.builder()
+            .forcePathStyle(configuration.getForcePathStyle())
             .region(region)
             .overrideConfiguration(
                 conf -> conf.retryPolicy(
                     builder -> builder.retryCondition(retryCondition).backoffStrategy(backoffStrategy)
                 )
             );
-
+        
         if (!isBlank(endpoint)) {
             clientBuilder.endpointOverride(URI.create(configuration.getEndpointProtocol() + "://" + endpoint));
         }
@@ -328,6 +329,6 @@ private S3AsyncClient asyncClientForRegion(String regionName) {
             asyncClientBuilder.credentialsProvider(() -> credentials);
         }
 
-        return asyncClientBuilder.region(region).build();
+        return asyncClientBuilder.forcePathStyle(configuration.getForcePathStyle()).region(region).build();
     }
 }

src/main/java/software/amazon/nio/spi/s3/config/S3NioSpiConfiguration.java

@@ -65,16 +65,19 @@ public class S3NioSpiConfiguration extends HashMap<String, Object> {
      * The default value of the endpoint protocol property
      */
     public static final String S3_SPI_ENDPOINT_PROTOCOL_DEFAULT = "https";
-
     /**
-     * The default value of the endpoint protocol property
+     * The name of the force path style property
+     */
+    public static final String S3_SPI_FORCE_PATH_STYLE_PROPERTY = "s3.spi.force-path-style";
+    /**
+     * The default value of the credentials property
      */
     public static final String S3_SPI_CREDENTIALS_PROPERTY = "s3.spi.credentials";
 
     private final Pattern ENDPOINT_REGEXP = Pattern.compile("(\\w[\\w\\-\\.]*)?(:(\\d+))?");
 
     private String bucketName;
-
+    
 
     /**
      * Create a new, empty configuration
@@ -85,15 +88,17 @@ public S3NioSpiConfiguration(){
 
     /**
      * Create a new, empty configuration
+     * 
+     * @param overrides configuration to override default values
      */
     public S3NioSpiConfiguration(Map<String, ?> overrides) {
         Objects.requireNonNull(overrides);
 
         //
         // setup defaults
         //
-        put(S3_SPI_READ_MAX_FRAGMENT_NUMBER_PROPERTY, String .valueOf(S3_SPI_READ_MAX_FRAGMENT_NUMBER_DEFAULT));
-        put(S3_SPI_READ_MAX_FRAGMENT_SIZE_PROPERTY, String .valueOf(S3_SPI_READ_MAX_FRAGMENT_SIZE_DEFAULT));
+        put(S3_SPI_READ_MAX_FRAGMENT_NUMBER_PROPERTY, String.valueOf(S3_SPI_READ_MAX_FRAGMENT_NUMBER_DEFAULT));
+        put(S3_SPI_READ_MAX_FRAGMENT_SIZE_PROPERTY, String.valueOf(S3_SPI_READ_MAX_FRAGMENT_SIZE_DEFAULT));
         put(S3_SPI_ENDPOINT_PROTOCOL_PROPERTY, S3_SPI_ENDPOINT_PROTOCOL_DEFAULT);
 
         //
@@ -115,7 +120,7 @@ public S3NioSpiConfiguration(Map<String, ?> overrides) {
             key -> Optional.ofNullable(System.getProperty(key)).ifPresent(val -> put(key, val))
         );
 
-        overrides.keySet().forEach(key -> put(key, String.valueOf(overrides.get(key))));
+        overrides.keySet().forEach(key -> put(key, overrides.get(key)));
     }
 
     /**
@@ -272,7 +277,27 @@ public S3NioSpiConfiguration withCredentials(AwsCredentials credentials) {
         }
         return this;
     }
-
+
+    /**
+     * Fluently sets the value of {@code forcePathStyle} and adds 
+     * {@code S3_SPI_FORCE_PATH_STYLE_PROPERTY} to the map unless the given
+     * value is null. If null, {@code S3_SPI_FORCE_PATH_STYLE_PROPERTY} is
+     * removed from the map.
+     *
+     * @param forcePathStyle the new value; can be null
+     *
+     * @return this instance
+     */
+    public S3NioSpiConfiguration withForcePathStyle(Boolean forcePathStyle) {
+        if (forcePathStyle == null) {
+            remove(S3_SPI_FORCE_PATH_STYLE_PROPERTY);
+        } else {
+            put(S3_SPI_FORCE_PATH_STYLE_PROPERTY, forcePathStyle); 
+        }
+
+        return this;
+    }  
+
     /**
      * Get the value of the Maximum Fragment Size
      * @return the configured value or the default if not overridden
@@ -363,6 +388,10 @@ public String getRegion() {
     public String getBucketName() {
         return bucketName;
     }
+
+    public boolean getForcePathStyle() {
+        return (boolean)getOrDefault(S3_SPI_FORCE_PATH_STYLE_PROPERTY, false);
+    }
 
     /**
      * Generates an environment variable name from a property name. E.g 'some.property' becomes 'SOME_PROPERTY'

src/main/java/software/amazon/nio/spi/s3x/S3XFileSystemProvider.java

@@ -17,7 +17,11 @@
 package software.amazon.nio.spi.s3x;
 
 import java.net.URI;
+import java.util.HashMap;
+import java.util.Map;
+import software.amazon.nio.spi.s3.S3FileSystem;
 import software.amazon.nio.spi.s3.S3FileSystemProvider;
+import static software.amazon.nio.spi.s3.config.S3NioSpiConfiguration.S3_SPI_FORCE_PATH_STYLE_PROPERTY;
 import software.amazon.nio.spi.s3.util.S3FileSystemInfo;
 import software.amazon.nio.spi.s3x.util.S3XFileSystemInfo;
 
@@ -27,6 +31,17 @@
 public class S3XFileSystemProvider extends S3FileSystemProvider {
 
     public static final String SCHEME = "s3x";
+
+    @Override
+    public S3FileSystem newFileSystem(final URI uri, Map<String, ?> env) {
+        Map<String, Object> newEnv = new HashMap<>();
+
+        newEnv.putAll(env);
+        newEnv.putIfAbsent(S3_SPI_FORCE_PATH_STYLE_PROPERTY, true);
+
+        return super.newFileSystem(uri, newEnv);
+    }
+
 
     /**
      * Returns the URI scheme that identifies this provider.

src/test/java/software/amazon/nio/spi/s3/FakeAsyncS3ClientBuilder.java

@@ -37,62 +37,64 @@ public class FakeAsyncS3ClientBuilder implements S3CrtAsyncClientBuilder {
 
     @Override
     public S3CrtAsyncClientBuilder credentialsProvider(AwsCredentialsProvider acp) {
-        return BUILDER.credentialsProvider(credentialsProvider = acp);
+        BUILDER.credentialsProvider(credentialsProvider = acp); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder region(Region r) {
-        return BUILDER.region(region = r);
+        BUILDER.region(region = r); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder minimumPartSizeInBytes(Long l) {
-        return BUILDER.minimumPartSizeInBytes(minimumPartSizeInBytes = l);
+        BUILDER.minimumPartSizeInBytes(minimumPartSizeInBytes = l); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder targetThroughputInGbps(Double d) {
-        return BUILDER.targetThroughputInGbps(targetThroughputInGbps = d);
+        BUILDER.targetThroughputInGbps(targetThroughputInGbps = d); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder maxConcurrency(Integer i) {
-        return BUILDER.maxConcurrency(maxConcurrency = i);
+        BUILDER.maxConcurrency(maxConcurrency = i); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder endpointOverride(URI u) {
-       return BUILDER.endpointOverride(endpointOverride = u);
+        BUILDER.endpointOverride(endpointOverride = u); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder checksumValidationEnabled(Boolean b) {
-        return BUILDER.checksumValidationEnabled(checksumValidationEnabled = b);
+        BUILDER.checksumValidationEnabled(checksumValidationEnabled = b);
+        return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder initialReadBufferSizeInBytes(Long l) {
-        return BUILDER.initialReadBufferSizeInBytes(initialReadBufferSizeInBytes = l);
+        BUILDER.initialReadBufferSizeInBytes(initialReadBufferSizeInBytes = l);
+        return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder httpConfiguration(S3CrtHttpConfiguration c) {
-        return BUILDER.httpConfiguration(httpConfiguration = c);
+        BUILDER.httpConfiguration(httpConfiguration = c); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder retryConfiguration(S3CrtRetryConfiguration c) {
-        return BUILDER.retryConfiguration(retryConfiguration = c);
+        BUILDER.retryConfiguration(retryConfiguration = c);  return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder accelerate(Boolean b) {
-        return BUILDER.accelerate(accelerate = b);
+        BUILDER.accelerate(accelerate = b); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder forcePathStyle(Boolean b) {
-        return BUILDER.forcePathStyle(forcePathStyle = b);
+        BUILDER.forcePathStyle(forcePathStyle = b); return this;
     }
 
     @Override
@@ -102,11 +104,11 @@ public S3AsyncClient build() {
 
     @Override
     public S3CrtAsyncClientBuilder crossRegionAccessEnabled(Boolean b) {
-        return BUILDER.crossRegionAccessEnabled(b);
+        BUILDER.crossRegionAccessEnabled(b); return this;
     }
 
     @Override
     public S3CrtAsyncClientBuilder thresholdInBytes(Long l) {
-        return BUILDER.thresholdInBytes(l);
+        BUILDER.thresholdInBytes(l); return this;
     }
 }

src/test/java/software/amazon/nio/spi/s3/S3FileSystemProviderTest.java

@@ -36,7 +36,6 @@
 import java.nio.file.FileSystem;
 import java.nio.file.FileSystemAlreadyExistsException;
 import java.nio.file.FileSystemNotFoundException;
-import java.nio.file.FileSystems;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -48,7 +47,6 @@
 import java.nio.file.attribute.FileTime;
 import java.time.Instant;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -66,7 +64,6 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import software.amazon.awssdk.services.s3.S3AsyncClient;
-import static software.amazon.nio.spi.s3.config.S3NioSpiConfiguration.AWS_REGION_PROPERTY;
 
 @SuppressWarnings("unchecked")
 @ExtendWith(MockitoExtension.class)
@@ -527,4 +524,15 @@ public void setAttribute() {
         S3Path foo = fs.getPath("/foo");
         assertThrows(UnsupportedOperationException.class, () -> provider.setAttribute(foo, "x", "y"));
     }
+
+
+    @Test
+    public void defaultForcePathStyle() throws Exception {
+        final FakeAsyncS3ClientBuilder BUILDER = new FakeAsyncS3ClientBuilder();
+
+        fs.clientProvider().asyncClientBuilder(BUILDER);
+        fs.client(); fs.close();
+
+        assertNull(BUILDER.forcePathStyle);
+    }
 }

src/test/java/software/amazon/nio/spi/s3/config/S3NioSpiConfigurationTest.java

@@ -9,6 +9,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Properties;
+import static org.assertj.core.api.BDDAssertions.entry;
 
 import static org.assertj.core.api.BDDAssertions.then;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -49,6 +50,7 @@ public void constructors() {
         then(config.getBucketName()).isNull();
         then(config.getRegion()).isNull();
         then(config.getCredentials()).isNull();
+        then(config.getForcePathStyle()).isFalse();
     }
 
     @Test
@@ -234,4 +236,22 @@ public void withAndGetBucketName() {
             then(x).hasMessage("Bucket name should not contain uppercase characters");
         }
     }
+
+    @Test
+    public void withAndGetForcePathStyle() {
+        then(config).doesNotContainKey(S3_SPI_FORCE_PATH_STYLE_PROPERTY);
+        then(config.withForcePathStyle(true)).isSameAs(config);
+        then(config).contains(entry(S3_SPI_FORCE_PATH_STYLE_PROPERTY, true));
+        then(config.getForcePathStyle()).isTrue();
+        then(config.withForcePathStyle(false).getForcePathStyle()).isFalse();
+
+        Map<String, Object> map = new HashMap<>(); config = new S3NioSpiConfiguration(map);
+        then(config.getForcePathStyle()).isFalse();
+        map.put(S3_SPI_FORCE_PATH_STYLE_PROPERTY, true); config = new S3NioSpiConfiguration(map);
+        then(config .getForcePathStyle()).isTrue();
+        map.remove(S3_SPI_FORCE_PATH_STYLE_PROPERTY); // same S3NioSpiConfiguration on purpose
+        then(config.getForcePathStyle()).isTrue();
+        then(config.withForcePathStyle(null).getForcePathStyle()).isFalse();
+        then(config).doesNotContainKey(S3_SPI_FORCE_PATH_STYLE_PROPERTY);
+    }
 }