CropSpec metadata can handle optional non String values

cropper/app/lib/CropSpecMetadata.scala

@@ -0,0 +1,57 @@
+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)
+
+    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
+
+    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))
+  }
+
+}

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         =

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)
+    }
+  }
+
+}