Implementing fluent initialization/setting of S3NioSpiConfiguration

[stefanofornari]

It also cleans up a bit the code using proper type for properties S3_SPI_READ_MAX_FRAGMENT_SIZE_PROPERTY and S3_SPI_READ_MAX_FRAGMENT_NUMBER_PROPERTY

Issue #
#112

Description of changes:
See #112 for a description of the functionality introduced.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[markjschreiber]

Possibly you can do this with a stream and collector or reuse String.join() with the sub array? https://docs.oracle.com/javase/8/docs/api/java/lang/String.html#join-java.lang.CharSequence-java.lang.CharSequence...-

[stefanofornari]

Looking at the JDK code I found there is a StringJoin that is supposed to do help in joining strings :) I would not complicate more the implementation as being a utility method I want to keep the implementation as most efficient as possible.

[markjschreiber]

I recommend using String.replaceAll("\p{javaWhitespace}", "") to replace anything that is equivalent to java.lang.Character.isWhitespace(). This will cover all kinds of form feed, line feed as well, etc.

[markjschreiber]

Perhaps you could even use String.matches(regex)?

[stefanofornari]

uhhh... mine was a pretty bad implementation actually... I am replacing with a much more efficient version (with a simple loop... less is more...)

[markjschreiber]

I think we are getting there. A couple of changes still required.

[stefanofornari]

I should have resolved all comments, but see below. There is an odd issue with gradle... I do not know why it is complaining about the version of a class in the logback package....

java.lang.UnsupportedClassVersionError: ch/qos/logback/classic/spi/LogbackServiceProvider has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0
[...]

I do not have the problem with maven and I am not a gradle expert. Can you give me some help?

Using access key and secret access key are but one way to create credentials in AWS. There are also situations in which you would need to provide additional information such as when using temporary credentials. The AWS docs list the full range of environment variables which can be used. I would therefore remove this version of withCredentials and use only withCredentials(AwsCredentials) and store that object in the map if needed.

If desired, you could retain this version of the method as a convenience method but use it to build an AwsBasicCredentials object and then use that in `withCredentials(credentials). An inversion of what you have here.

I have implemented it as suggested for your review. However I am doubtful: this configuration class is mostly designed to glue envvar/syspro and simple configuration properties. WRT credentials the original implementation did not provide special support credentials because it delegated it to the underlying AWS library. To support 3rd party services we need instead to add a key/secret mechanism. The doubt is that support for different authentication means, usually AWS specific, can still be achieved without the need of withCredentials(AwsCredentials) just not setting any credentials.
In short: consider to get rid of it, I am fine either ways.

[stefanofornari]

@markjschreiber any chances we can sort out this one? it would be very helpful to start to work on s3x provider

[markjschreiber]

@markjschreiber any chances we can sort out this one? it would be very helpful to start to work on s3x provider

Almost. I think that if you want to retain the withCredentials(string, string) method then this should construct an AwsBasicCredentials instance and pass that the withCredentials(AwsCredentials) and not the other way around. That way people with more complex credentials can use the withCredentials(AwsCredentials) version.

Apart from that it looks good although one of the unit tests seems to be failing right now.

[stefanofornari]

@markjschreiber it should be ready to go.

[markjschreiber]

Looks good. Thanks for the PR!