Do not set up the content type with it is already correctly set (#5131)

Co-authored-by: Simon Dumas <[email protected]>
BlueBrain · Sep 4, 2024 · f3d3e34 · f3d3e34
1 parent 823d072
commit f3d3e34
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 48 deletions.
diff --git a/...ala/ch/epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/s3/CopyResult.scala b/...ala/ch/epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/s3/CopyResult.scala
diff --git a/...epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/s3/S3OperationResult.scala b/...epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/s3/S3OperationResult.scala
@@ -0,0 +1,11 @@
+package ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3
+
+sealed trait S3OperationResult extends Product with Serializable
+
+object S3OperationResult {
+
+ final case object Success extends S3OperationResult
+
+ final case object AlreadyExists extends S3OperationResult
+
+}
diff --git a/...bluebrain/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClient.scala b/...bluebrain/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClient.scala
@@ -3,7 +3,7 @@ package ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.cli
 import akka.http.scaladsl.model.ContentType
 import cats.effect.{IO, Resource}
 import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.StoragesConfig.S3StorageConfig
-import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, CopyResult, HeadObject, PutObjectRequest}
+import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, HeadObject, PutObjectRequest, S3OperationResult}
 import fs2.Stream
 import io.laserdisc.pure.s3.tagless.{Interpreter, S3AsyncClientOp}
 import software.amazon.awssdk.auth.credentials.{AwsBasicCredentials, AwsCredentialsProvider, DefaultCredentialsProvider, StaticCredentialsProvider}
@@ -35,22 +35,22 @@ trait S3StorageClient {
  destinationBucket: String,
  destinationKey: String,
  options: CopyOptions
- ): IO[CopyResult]
+ ): IO[S3OperationResult]
 
  def copyObjectMultiPart(
  sourceBucket: String,
  sourceKey: String,
  destinationBucket: String,
  destinationKey: String,
  options: CopyOptions
- ): IO[CopyResult]
+ ): IO[S3OperationResult]
 
  def uploadFile(
  put: PutObjectRequest,
  fileData: Stream[IO, ByteBuffer]
  ): IO[Unit]
 
- def updateContentType(bucket: String, key: String, contentType: ContentType): IO[Unit]
+ def updateContentType(bucket: String, key: String, contentType: ContentType): IO[S3OperationResult]
 
  def objectExists(bucket: String, key: String): IO[Boolean]
  def bucketExists(bucket: String): IO[Boolean]

diff --git a/...n/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClientDisabled.scala b/...n/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClientDisabled.scala
@@ -2,7 +2,7 @@ package ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.cli
 
 import akka.http.scaladsl.model.ContentType
 import cats.effect.IO
-import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, CopyResult, HeadObject, PutObjectRequest}
+import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, HeadObject, PutObjectRequest, S3OperationResult}
 import ch.epfl.bluebrain.nexus.delta.sdk.error.ServiceError.FeatureDisabled
 import fs2.Stream
 import software.amazon.awssdk.services.s3.model._
@@ -27,7 +27,7 @@ private[client] object S3StorageClientDisabled extends S3StorageClient {
  destinationBucket: String,
  destinationKey: String,
  options: CopyOptions
- ): IO[CopyResult] = raiseDisabledErr
+ ): IO[S3OperationResult] = raiseDisabledErr
 
  override def objectExists(bucket: String, key: String): IO[Boolean] = raiseDisabledErr
 
@@ -36,7 +36,8 @@ private[client] object S3StorageClientDisabled extends S3StorageClient {
  data: Stream[IO, ByteBuffer]
  ): IO[Unit] = raiseDisabledErr
 
- override def updateContentType(bucket: String, key: String, contentType: ContentType): IO[Unit] = raiseDisabledErr
+ override def updateContentType(bucket: String, key: String, contentType: ContentType): IO[S3OperationResult] =
+ raiseDisabledErr
 
  override def bucketExists(bucket: String): IO[Boolean] = raiseDisabledErr
 
@@ -46,7 +47,7 @@ private[client] object S3StorageClientDisabled extends S3StorageClient {
  destinationBucket: String,
  destinationKey: String,
  options: CopyOptions
- ): IO[CopyResult] = raiseDisabledErr
+ ): IO[S3OperationResult] = raiseDisabledErr
 
  override def readFileMultipart(bucket: String, fileKey: String): Stream[IO, Byte] = throw disabledErr
 }
diff --git a/...brain/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClientImpl.scala b/...brain/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClientImpl.scala
@@ -62,7 +62,7 @@ final private[client] class S3StorageClientImpl(client: S3AsyncClientOp[IO]) ext
  destinationBucket: String,
  destinationKey: String,
  options: CopyOptions
- ): IO[CopyResult] =
+ ): IO[S3OperationResult] =
  approveCopy(destinationBucket, destinationKey, options.overwriteTarget).flatMap { approved =>
  if (approved) {
  val requestBuilder = CopyObjectRequest
@@ -77,8 +77,8 @@ final private[client] class S3StorageClientImpl(client: S3AsyncClientOp[IO]) ext
  .contentType(contentType.value)
  .metadataDirective(MetadataDirective.REPLACE)
  }
- client.copyObject(requestWithOptions.build()).as(CopyResult.Success)
- } else IO.pure(CopyResult.AlreadyExists)
+ client.copyObject(requestWithOptions.build()).as(S3OperationResult.Success)
+ } else IO.pure(S3OperationResult.AlreadyExists)
  }
 
  def copyObjectMultiPart(
@@ -87,13 +87,13 @@ final private[client] class S3StorageClientImpl(client: S3AsyncClientOp[IO]) ext
  destinationBucket: String,
  destinationKey: String,
  options: CopyOptions
- ): IO[CopyResult] =
+ ): IO[S3OperationResult] =
  approveCopy(destinationBucket, destinationKey, options.overwriteTarget).flatMap { approved =>
  if (approved) {
  copyObjectMultiPart(sourceBucket, sourceKey, destinationBucket, destinationKey, options.newContentType).as(
- CopyResult.Success
+ S3OperationResult.Success
  )
- } else IO.pure(CopyResult.AlreadyExists)
+ } else IO.pure(S3OperationResult.AlreadyExists)
  }
 
  private def copyObjectMultiPart(
@@ -183,19 +183,21 @@ final private[client] class S3StorageClientImpl(client: S3AsyncClientOp[IO]) ext
  .compile
  .drain
 
- override def updateContentType(bucket: String, key: String, contentType: ContentType): IO[Unit] = {
- val requestBuilder = CopyObjectRequest
- .builder()
- .sourceBucket(bucket)
- .sourceKey(key)
- .destinationBucket(bucket)
- .destinationKey(key)
- .checksumAlgorithm(checksumAlgorithm)
- .contentType(contentType.value)
- .metadataDirective(MetadataDirective.REPLACE)
-
- client.copyObject(requestBuilder.build()).void
- }
+ override def updateContentType(bucket: String, key: String, contentType: ContentType): IO[S3OperationResult] =
+ headObject(bucket, key).flatMap {
+ case head if head.contentType.contains(contentType) => IO.pure(S3OperationResult.AlreadyExists)
+ case _ =>
+ val requestBuilder = CopyObjectRequest
+ .builder()
+ .sourceBucket(bucket)
+ .sourceKey(key)
+ .destinationBucket(bucket)
+ .destinationKey(key)
+ .checksumAlgorithm(checksumAlgorithm)
+ .contentType(contentType.value)
+ .metadataDirective(MetadataDirective.REPLACE)
+ client.copyObject(requestBuilder.build()).as(S3OperationResult.Success)
+ }
 
  override def bucketExists(bucket: String): IO[Boolean] = {
  listObjectsV2(bucket)

diff --git a/...rain/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClientSuite.scala b/...rain/nexus/delta/plugins/storage/storages/operations/s3/client/S3StorageClientSuite.scala
@@ -3,7 +3,7 @@ package ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.cli
 import akka.http.scaladsl.model.ContentTypes
 import cats.effect.IO
 import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.StoragesConfig.S3StorageConfig
-import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, CopyResult, LocalStackS3StorageClient, S3Helpers}
+import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, LocalStackS3StorageClient, S3Helpers, S3OperationResult}
 import ch.epfl.bluebrain.nexus.testkit.mu.NexusSuite
 import io.laserdisc.pure.s3.tagless.S3AsyncClientOp
 import munit.AnyFixture
@@ -33,7 +33,7 @@ class S3StorageClientSuite extends NexusSuite with LocalStackS3StorageClient.Fix
  result <- s3StorageClient.copyObject(bucket, key, bucket, newKey, options)
  head <- s3StorageClient.headObject(bucket, newKey)
  } yield {
- assertEquals(result, CopyResult.Success)
+ assertEquals(result, S3OperationResult.Success)
  assertEquals(head.fileSize, contentLength)
  assertEquals(head.contentType, Some(expectedContentType))
  }
@@ -50,7 +50,7 @@ class S3StorageClientSuite extends NexusSuite with LocalStackS3StorageClient.Fix
  result <- s3StorageClient.copyObject(bucket, key, bucket, newKey, options)
  head <- s3StorageClient.headObject(bucket, newKey)
  } yield {
- assertEquals(result, CopyResult.Success)
+ assertEquals(result, S3OperationResult.Success)
  assertEquals(head.fileSize, contentLength)
  assertEquals(head.contentType, Some(contentType))
  }
@@ -67,7 +67,7 @@ class S3StorageClientSuite extends NexusSuite with LocalStackS3StorageClient.Fix
  head <- s3StorageClient.headObject(bucket, existingTargetKey)
  } yield {
  val clue = "The file should not have been overwritten"
- assertEquals(result, CopyResult.AlreadyExists)
+ assertEquals(result, S3OperationResult.AlreadyExists)
  assertEquals(head.fileSize, anotherContentLength, clue)
  assertEquals(head.contentType, Some(expectedContentType), clue)
  }
@@ -84,7 +84,7 @@ class S3StorageClientSuite extends NexusSuite with LocalStackS3StorageClient.Fix
  head <- s3StorageClient.headObject(bucket, existingTargetKey)
  } yield {
  val clue = "The file should have been overwritten"
- assertEquals(result, CopyResult.Success)
+ assertEquals(result, S3OperationResult.Success)
  assertEquals(head.fileSize, contentLength, clue)
  assertEquals(head.contentType, Some(contentType), clue)
  }
@@ -96,13 +96,29 @@ class S3StorageClientSuite extends NexusSuite with LocalStackS3StorageClient.Fix
  givenAnS3Bucket { bucket =>
  givenAFileInABucket(bucket, fileContents) { key =>
  for {
- _  <- s3StorageClient.updateContentType(bucket, key, contentType)
- head <- s3StorageClient.headObject(bucket, key)
+ result <- s3StorageClient.updateContentType(bucket, key, contentType)
+ head  <- s3StorageClient.headObject(bucket, key)
  } yield {
+ assertEquals(result, S3OperationResult.Success)
  assertEquals(head.contentType, Some(contentType))
  }
  }
  }
  }
 
+ test("Do not update the content type of an existing object if it is already set to this value") {
+ val originalContentType = ContentTypes.`text/plain(UTF-8)`
+ givenAnS3Bucket { bucket =>
+ givenAFileInABucket(bucket, fileContents) { key =>
+ for {
+ result <- s3StorageClient.updateContentType(bucket, key, originalContentType)
+ head <- s3StorageClient.headObject(bucket, key)
+ } yield {
+ assertEquals(result, S3OperationResult.AlreadyExists)
+ assertEquals(head.contentType, Some(originalContentType))
+ }
+ }
+ }
+ }
+
 }
diff --git a/ship/src/main/scala/ch/epfl/bluebrain/nexus/ship/files/FileCopier.scala b/ship/src/main/scala/ch/epfl/bluebrain/nexus/ship/files/FileCopier.scala
@@ -8,7 +8,7 @@ import ch.epfl.bluebrain.nexus.delta.kernel.RetryStrategy.logError
 import ch.epfl.bluebrain.nexus.delta.kernel.{Logger, RetryStrategy}
 import ch.epfl.bluebrain.nexus.delta.kernel.utils.UrlUtils
 import ch.epfl.bluebrain.nexus.delta.plugins.storage.files.model.FileAttributes
-import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, CopyResult, S3LocationGenerator}
+import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.{CopyOptions, S3LocationGenerator, S3OperationResult}
 import ch.epfl.bluebrain.nexus.delta.plugins.storage.storages.operations.s3.client.S3StorageClient
 import ch.epfl.bluebrain.nexus.delta.sourcing.model.ProjectRef
 import ch.epfl.bluebrain.nexus.ship.config.FileProcessingConfig
@@ -74,8 +74,8 @@ object FileCopier {
  } else
  s3StorageClient.copyObject(importBucket, originKey, targetBucket, targetKey, copyOptions)
  }.flatMap {
- case CopyResult.Success => IO.unit
- case CopyResult.AlreadyExists =>
+ case S3OperationResult.Success => IO.unit
+ case S3OperationResult.AlreadyExists =>
  IO.whenA(forceContentType) {
  attributes.mediaType.traverse { mediaType =>
  logger.info(s"Patching to content type $mediaType for file $patchedFileName") >>