diff --git a/cropper/app/lib/CropSpecMetadata.scala b/cropper/app/lib/CropSpecMetadata.scala new file mode 100644 index 0000000000..78e6c0a707 --- /dev/null +++ b/cropper/app/lib/CropSpecMetadata.scala @@ -0,0 +1,50 @@ +package lib + +import com.gu.mediaservice.lib.formatting.printDateTime +import com.gu.mediaservice.model.{Bounds, Crop, CropSpec, Dimensions, ExportType} + +trait CropSpecMetadata { + + def metadataForCrop(crop: Crop, dimensions: Dimensions): Map[String, String] = { + val CropSpec(sourceUri, Bounds(x, y, w, h), r, t) = crop.specification + val metadata = Map("source" -> sourceUri, + "bounds-x" -> x, + "bounds-y" -> y, + "bounds-width" -> w, + "bounds-height" -> h, + "type" -> t.name, + "author" -> crop.author, + "date" -> crop.date.map(printDateTime), + "width" -> dimensions.width, + "height" -> dimensions.height, + "aspect-ratio" -> r) + + metadata.collect { + case (key, Some(value)) => key -> value.toString + case (key, value) if value != None => key -> value.toString + } + } + + def cropSpecFromMetadata(userMetadata: Map[String, String]): Option[CropSpec] = { + for { + source <- userMetadata.get("source") + // we've moved to kebab-case as localstack doesn't like `_` + // fallback to reading old values for older crops + // see https://github.com/localstack/localstack/issues/459 + x <- getOrElseOrNone(userMetadata, "bounds-x", "bounds_x").map(_.toInt) + y <- getOrElseOrNone(userMetadata, "bounds-y", "bounds_y").map(_.toInt) + w <- getOrElseOrNone(userMetadata, "bounds-width", "bounds_w").map(_.toInt) + h <- getOrElseOrNone(userMetadata, "bounds-height", "bounds_h").map(_.toInt) + ratio = getOrElseOrNone(userMetadata, "aspect-ratio", "aspect_ratio") + exportType = userMetadata.get("type").map(ExportType.valueOf).getOrElse(ExportType.default) + } yield { + CropSpec(source, Bounds(x, y, w, h), ratio, exportType) + } + } + + private def getOrElseOrNone(theMap: Map[String, String], preferredKey: String, fallbackKey: String): Option[String] = { + // Return the `preferredKey` value in `theMap` or the `fallbackKey` or `None` + theMap.get(preferredKey).orElse(theMap.get(fallbackKey)) + } + +} diff --git a/cropper/app/lib/CropStore.scala b/cropper/app/lib/CropStore.scala index 37e36ef1b8..6026b2710e 100644 --- a/cropper/app/lib/CropStore.scala +++ b/cropper/app/lib/CropStore.scala @@ -7,34 +7,15 @@ import com.gu.mediaservice.lib.S3ImageStorage import com.gu.mediaservice.lib.logging.LogMarker import com.gu.mediaservice.model._ -import scala.collection.compat._ - -class CropStore(config: CropperConfig) extends S3ImageStorage(config) { +class CropStore(config: CropperConfig) extends S3ImageStorage(config) with CropSpecMetadata { import com.gu.mediaservice.lib.formatting._ def getSecureCropUri(uri: URI): Option[URL] = config.imgPublishingSecureHost.map(new URI("https", _, uri.getPath, uri.getFragment).toURL) - def storeCropSizing(file: File, filename: String, mimeType: MimeType, crop: Crop, dimensions: Dimensions)(implicit logMarker: LogMarker) : Future[Asset] = { - val CropSpec(sourceUri, Bounds(x, y, w, h), r, t) = crop.specification - val metadata = Map("source" -> sourceUri, - "bounds-x" -> x, - "bounds-y" -> y, - "bounds-width" -> w, - "bounds-height" -> h, - "type" -> t.name, - "author" -> crop.author, - "date" -> crop.date.map(printDateTime), - "width" -> dimensions.width, - "height" -> dimensions.height - ) ++ r.map("aspect-ratio" -> _) - - val filteredMetadata = metadata.collect { - case (key, Some(value)) => key -> value - case (key, value) => key -> value - }.view.mapValues(_.toString).toMap - - storeImage(config.imgPublishingBucket, filename, file, Some(mimeType), filteredMetadata, overwrite = true) map { s3Object => + def storeCropSizing(file: File, filename: String, mimeType: MimeType, crop: Crop, dimensions: Dimensions)(implicit logMarker: LogMarker): Future[Asset] = { + val metadata = metadataForCrop(crop, dimensions) + storeImage(config.imgPublishingBucket, filename, file, Some(mimeType), metadata, overwrite = true) map { s3Object => Asset( translateImgHost(s3Object.uri), Some(s3Object.size), @@ -45,11 +26,6 @@ class CropStore(config: CropperConfig) extends S3ImageStorage(config) { } } - private def getOrElseOrNone(theMap: Map[String, String], preferredKey: String, fallbackKey: String): Option[String] = { - // Return the `preferredKey` value in `theMap` or the `fallbackKey` or `None` - theMap.get(preferredKey).orElse(theMap.get(fallbackKey)) - } - def listCrops(id: String): Future[List[Crop]] = { list(config.imgPublishingBucket, id).map { crops => crops.foldLeft(Map[String, Crop]()) { @@ -60,25 +36,17 @@ class CropStore(config: CropperConfig) extends S3ImageStorage(config) { val objectMetadata = s3Object.metadata.objectMetadata val updatedCrop = for { - // Note: if any is missing, the entry won't be registered - source <- userMetadata.get("source") - - // we've moved to kebab-case as localstack doesn't like `_` - // fallback to reading old values for older crops - // see https://github.com/localstack/localstack/issues/459 - x <- getOrElseOrNone(userMetadata, "bounds-x", "bounds_x").map(_.toInt) - y <- getOrElseOrNone(userMetadata, "bounds-y", "bounds_y").map(_.toInt) - w <- getOrElseOrNone(userMetadata, "bounds-width", "bounds_w").map(_.toInt) - h <- getOrElseOrNone(userMetadata, "bounds-height", "bounds_h").map(_.toInt) - width <- userMetadata.get("width").map(_.toInt) + cropSource <- cropSpecFromMetadata(userMetadata) + x = cropSource.bounds.x + y = cropSource.bounds.y + w = cropSource.bounds.width + h = cropSource.bounds.height + width <- userMetadata.get("width").map(_.toInt) height <- userMetadata.get("height").map(_.toInt) cid = s"$id-$x-$y-$w-$h" - ratio = getOrElseOrNone(userMetadata, "aspect-ratio", "aspect_ratio") author = userMetadata.get("author") date = userMetadata.get("date").flatMap(parseDateTime) - exportType = userMetadata.get("type").map(ExportType.valueOf).getOrElse(ExportType.default) - cropSource = CropSpec(source, Bounds(x, y, w, h), ratio, exportType) dimensions = Dimensions(width, height) sizing = diff --git a/cropper/test/lib/CropSpecMetadataTest.scala b/cropper/test/lib/CropSpecMetadataTest.scala new file mode 100644 index 0000000000..bfd3c8c8d1 --- /dev/null +++ b/cropper/test/lib/CropSpecMetadataTest.scala @@ -0,0 +1,52 @@ +package lib + +import com.gu.mediaservice.model._ +import org.joda.time.DateTime +import org.scalatest.funspec.AnyFunSpec +import org.scalatest.matchers.should.Matchers + +class CropSpecMetadataTest extends AnyFunSpec with Matchers with CropSpecMetadata { + + private val cropSpec = CropSpec( + uri = "/test", + bounds = Bounds(1, 2, 3, 4), aspectRatio = Some("16:9"), `type` = ExportType.default + ) + private val crop = Crop( + id = Some("123"), + specification = cropSpec, + author = Some("Tony McCrae"), + date = Some(DateTime.now), + master = None, + assets = List.empty, + ) + private val dimensions = Dimensions(640, 480) + + describe("metadata for crop spec") { + it("should serialize crop spec to key value pairs") { + val metadata = metadataForCrop(crop, dimensions) + + metadata("width") shouldBe "640" + metadata("height") shouldBe "480" + metadata.get("aspect-ratio") shouldBe Some("16:9") + metadata.get("author") shouldBe Some("Tony McCrae") + } + + it("should handle empty optional fields") { + val withEmptyField = cropSpec.copy(aspectRatio = None) + + val metadata = metadataForCrop(crop.copy(specification = withEmptyField, author = None), dimensions) + + metadata.get("aspect-ratio") shouldBe None + metadata.get("author") shouldBe None + } + + it("should round trip metadata back to crop spec") { + val metadata = metadataForCrop(crop, dimensions) + + val roundTripped = cropSpecFromMetadata(metadata) + + roundTripped shouldBe Some(cropSpec) + } + } + +}