Skip to content
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.

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

feat: implement a presignGetObject method #500

Open
wants to merge 2 commits into
base: series/2.x
Choose a base branch
from

Conversation

mbaechler
Copy link
Contributor

We implemented a method to generate presigned links :

  /**
   * This method generates a `PresignedGetObjectRequest` containing an URL that allows the object to be downloaded
   * without any credentials. Signature happens locally, it does not issue any network call. It's not a pure
   * computation neither as it performs an IO by looking at the current time.
   *
   * See https://web.archive.org/web/20240621234636/https://docs.aws.amazon.com/AmazonS3/latest/userguide/ShareObjectPreSignedURL.html
   * for further details regarding signature with S3.
   */

It requires to instantiate a S3Presigner in the Live class but it doesn't leak to the user.

@mbaechler mbaechler requested a review from a team as a code owner June 26, 2024 14:18
"software.amazon.awssdk" % "sts" % awsVersion,
"dev.zio" %% "zio-test" % zioVersion % Test,
"dev.zio" %% "zio-test-sbt" % zioVersion % Test,
"com.dimafeng" %% "testcontainers-scala-minio" % "0.41.4" % Test,
Copy link
Member

@regis-leray regis-leray Jun 26, 2024

Choose a reason for hiding this comment

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

please remove test container, we already have a minio in place with docker compose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Regis,

Actually we find that handy to not have a launch containers by hand before running tests.
Could you elaborate on why you don't like it, please?

Copy link
Member

Choose a reason for hiding this comment

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

For multiple reason: slow down the execution, more code to add in unit-test, more librairies dependencies etc...
Prefer to stick to docker-compose, since it works pretty well.
thank you

* See https://web.archive.org/web/20240621234636/https://docs.aws.amazon.com/AmazonS3/latest/userguide/ShareObjectPreSignedURL.html
* for further details regarding signature with S3.
*/
override def presignGetObject(
Copy link
Member

Choose a reason for hiding this comment

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

i dont think this code should be in the client. since there is nothing related to this. you could move this into the extra utils function

 def presignGetObject[R](
    bucketName: String,
    key: String,
    signatureDuration: Duration
  ): ZIO[S3Presigner, S3Exception, Unit] =   ZIO.serviceWithZIO[S3Presigner]{ s3Presigner =>

   //should wrap this unasync call with ZIO
   s3Presigner.presignGetObject(
        GetObjectPresignRequest
          .builder()
          .signatureDuration(signatureDuration)
          .getObjectRequest(GetObjectRequest.builder().bucket(bucketName).key(key).build())
          .build()
}


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the reason why we chose to put it in the client is that the configuration required is very similiar to the S3 client one.
The other solution is to provide another service with it's own layer definition, but then we should probably provide a layer for the configuration to avoid asking the same configuration twice to the user.
What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants