From 32e4b11ee2d2f17f46c84c674dfd4df758759b96 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 31 Aug 2024 13:53:46 +0100 Subject: [PATCH 1/3] Refactor; extract the mapping of Crops to metadata and back. To allow testing. --- cropper/app/lib/CropSpecMetadata.scala | 52 ++++++++++++++++++++++++++ cropper/app/lib/CropStore.scala | 52 +++++--------------------- 2 files changed, 62 insertions(+), 42 deletions(-) create mode 100644 cropper/app/lib/CropSpecMetadata.scala diff --git a/cropper/app/lib/CropSpecMetadata.scala b/cropper/app/lib/CropSpecMetadata.scala new file mode 100644 index 0000000000..4baa9cd3fc --- /dev/null +++ b/cropper/app/lib/CropSpecMetadata.scala @@ -0,0 +1,52 @@ +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, + ) ++ r.map("aspect-ratio" -> _) + + val flattenedMetadata = metadata.collect { + case (key, Some(value)) => key -> value + case (key, value) => key -> value + }.view.mapValues(_.toString).toMap + + flattenedMetadata + } + + 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 = From 14fa02b07b32719589d5ba50152a1f4fef918e8a Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 31 Aug 2024 19:46:47 +0100 Subject: [PATCH 2/3] Initial testing for crop spec metadata; CropSpec and Crop fields seem to handle None inputs to metadata differently! Not sure if this is intentional. --- cropper/test/lib/CropSpecMetadataTest.scala | 52 +++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 cropper/test/lib/CropSpecMetadataTest.scala diff --git a/cropper/test/lib/CropSpecMetadataTest.scala b/cropper/test/lib/CropSpecMetadataTest.scala new file mode 100644 index 0000000000..421bc31441 --- /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 Some("None") // TODO this does not look intentional! + } + + it("should round trip metadata back to crop spec") { + val metadata = metadataForCrop(crop, dimensions) + + val roundTripped = cropSpecFromMetadata(metadata) + + roundTripped shouldBe Some(cropSpec) + } + } + +} From d294f9eb579844e7b0036a7a101b2dd11bc69df0 Mon Sep 17 00:00:00 2001 From: Tony McCrae Date: Sat, 31 Aug 2024 19:56:17 +0100 Subject: [PATCH 3/3] Fixes literal 'None' encoding of some empty Crop fields. Treat all fields the same way then explicitly filter out None fields to protect ourselves from any misunderstandings about how collect treats values of type Option. --- cropper/app/lib/CropSpecMetadata.scala | 9 +++++++-- cropper/test/lib/CropSpecMetadataTest.scala | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/cropper/app/lib/CropSpecMetadata.scala b/cropper/app/lib/CropSpecMetadata.scala index 4baa9cd3fc..f6875a5816 100644 --- a/cropper/app/lib/CropSpecMetadata.scala +++ b/cropper/app/lib/CropSpecMetadata.scala @@ -17,9 +17,14 @@ trait CropSpecMetadata { "date" -> crop.date.map(printDateTime), "width" -> dimensions.width, "height" -> dimensions.height, - ) ++ r.map("aspect-ratio" -> _) + "aspect-ratio" -> r) - val flattenedMetadata = metadata.collect { + val nonEmptyMetadata = metadata.filter { + case (_, None) => false + case _ => true + } + + val flattenedMetadata = nonEmptyMetadata.collect { case (key, Some(value)) => key -> value case (key, value) => key -> value }.view.mapValues(_.toString).toMap diff --git a/cropper/test/lib/CropSpecMetadataTest.scala b/cropper/test/lib/CropSpecMetadataTest.scala index 421bc31441..bfd3c8c8d1 100644 --- a/cropper/test/lib/CropSpecMetadataTest.scala +++ b/cropper/test/lib/CropSpecMetadataTest.scala @@ -37,7 +37,7 @@ class CropSpecMetadataTest extends AnyFunSpec with Matchers with CropSpecMetadat val metadata = metadataForCrop(crop.copy(specification = withEmptyField, author = None), dimensions) metadata.get("aspect-ratio") shouldBe None - metadata.get("author") shouldBe Some("None") // TODO this does not look intentional! + metadata.get("author") shouldBe None } it("should round trip metadata back to crop spec") {