S3: Remove duplicated region argument in client methods

[zaharidichev]

Currently we have:

final class S3Settings(val bufferType: BufferType,
                       val diskBufferPath: String,
                       val proxy: Option[Proxy],
                       val awsCredentials: AWSCredentials,
                       val s3Region: String,
                       val pathStyleAccess: Boolean)

But then we have methods such as:

  private[this] def s3Request(s3Location: S3Location,
                              region: String,
                              method: HttpMethod = HttpMethods.GET,
                              uriFn: (Uri => Uri) = identity)(implicit conf: S3Settings): HttpRequest

Whats the point of passing a region arg if we already have it configured into the implicit configuration. This is confusing. IMO, we need to omit the region argument and keep the one in S3Settings and use it.

[zaharidichev]

As a matter of fact I think it will be better to proceed with this before addressing #271

[johanandren]

Would be good to have one of the previous s3 committers comment on this as it will break the code of pretty much everyone who used the current s3 client API.

Ping @thereisnospoon, @kobmic, @Scalahansolo, @svezfaz, @2Beaucoup

[svezfaz]

I think cleaning up the API now might make sense. It seems like there is more than one wrong thing with it:

object S3Stream {

  def apply(credentials: AWSCredentials, region: String)(implicit system: ActorSystem, mat: Materializer): S3Stream =
    new S3Stream(credentials, region, S3Settings(system))
}

private[alpakka] final class S3Stream(credentials: AWSCredentials,
                                      region: String,
                                      val settings: S3Settings)(implicit system: ActorSystem, mat: Materializer) {
...
}

1. neither system nor materializer are actually needed to create a S3Stream. Only thing needed is an executionContext, and that could as well be passed to every method separately, instead of passing it at creation
2. as per @zaharidichev comment, we only really need the S3Settings object to create an S3Stream, having this hybrid approach makes for a confusing API
3. S3Settings.apply taking an ActorSystem does not seem good practice to me. If we want a default way to create S3Settings we could just have an empty apply() that performs ConfigFactory.load() for us

Moreover, issues 1 and 2 gets propagated to S3Client as well.

+1 to sort this up now :)

[zaharidichev]

@svezfaz Should we sort that stuff in this PR or create a new one ? Also, if anyone else can chip in with any comments, it will be greatly appreciated :)

[svezfaz]

my take is that it won't be lots of work, and at least we would break the API with a single pass, instead of fragmenting this. But it's up to you of course.

[zaharidichev]

Alright, will finish it later tonight and push then :)

[zaharidichev]

@svezfaz While we are on these pages, what do you think about this piece of code here:

private[alpakka] final class S3Stream(credentials: AWSCredentials,
                                      val settings: S3Settings)

S3Settings already contains AWSCredentials so again this does not seem to be very clean. I think its better to remove the ability to pass AWSCredentials. Everything that is configuration should be coming from S3Settings.

[kobmic]

@zaharidichev I like the possibility to pass AWSCredentials in S3Client (which then calls S3Stream). We always use IAM roles and never store AWS credentials in any config files. If I understand your proposal correctly, we would have to keep credentials in the config file?

[svezfaz]

@kobmic agreed, the contract for S3Client should probably stay. It makes sense for simpler usages to just bypass having specific config files. It will just need to be changed internally in how it creates the underlying S3Stream, e.g. it could load the reference config and override only credentials and region.

[zaharidichev]

@kobmic The more I think about it, it is clear that you are quite right. @svezfaz So what do you think is the best course of action in that case. Do we leave it like it is ? I did not quite get what you mean by "override". By the looks of it, we now are obliged to pass e separate credentials object on construction no matter what.

[svezfaz]

What I meant is, we could have S3Stream that can be only created passing a S3Settings, and keep S3Client simpler API requiring only credentials and region.
S3Client internals could look something like

final class S3Client(credentials: AWSCredentials, region: String) {

  val settings = S3Settings.apply(ConfigFactory.load())
    .copy(awsCredentials = credentials, s3Region = region)
  
  private[this] val impl = S3Stream(settings)
  ...
}

(this would require making S3Settings a case class.. which might be a good call anyway ;) )

[zaharidichev]

That makes sense. However what happens in the situation that is the opposite of the one that @kobmic describes. Imagine a client of the API actually does keep all the settings (including credentials and region) in the configuration file. Then he is forced to provide these when constructing the S3Client even though they will be loaded from the config file. Does not seem very intuitive to me.

object S3Client {
  val MinChunkSize = 5242880

  def apply(): S3Client = new S3Client(S3Settings.apply(ConfigFactory.load()))
  
  def apply(credentials: AWSCredentials, region: String): S3Client = {
    val settings = S3Settings.apply(ConfigFactory.load())
      .copy(awsCredentials = credentials, s3Region = region)
    new S3Client(s3Settings)
  }
}

final class S3Client(s3Settings: S3Settings) {
.......

This leaves it to the client of the API to choose whether to rely completely on the config file or override region and credentials.

[svezfaz]

@zaharidichev yes, that looks like what we need

[kobmic]

@zaharidichev looks good to me

[zaharidichev]

@svezfaz

neither system nor materializer are actually needed to create a S3Stream. Only thing needed is an executionContext, and that could as well be passed to every method separately, instead of passing it at creation

The method below however needs both the Actor System and the materializer, You guys thinks that its better to pass all that is needed implicitly on every call, where needed, instead of provideng them on construction ?

  private def signAndGet(request: HttpRequest): Future[HttpResponse] =
    for (req <- Signer.signedRequest(request, signingKey);
         res <- Http().singleRequest(req)) yield res

[svezfaz]

@zaharidichev you're right - I missed the fact that it's using Http().singleRequest in a few places. This indeed requires a materializer. I would pass it in at creation - we can keep the mat.executionContext calls wherever needed.

Where do you see an ActorSystem is needed?

[zaharidichev]

Hi there. Its needed in calls such as Http().singleRequest(req). Because...

object Http {
  def apply()(implicit system: ActorSystem): HttpExt = super.apply(system)
.....
}

While Materializer is need in Signer.signedRequest

[zaharidichev]

So we really have two options actually. One is to pass things at construction time, the way it is now and the other is to pas whatever is needed as implicits. I think we need to choose one of them and to stick to it.

[svezfaz]

completely missed that as well. sorry :/
I would just pass them at construction

[zaharidichev]

Think we should be all good now
#298

[zaharidichev]

@svezfaz does that look good as it is ?