Skip to content

Commit

Permalink
Merge pull request #4347 from eelpie/cropspec-metadata-none
Browse files Browse the repository at this point in the history
CropSpec metadata can handle optional non String values
  • Loading branch information
andrew-nowak authored Dec 4, 2024
2 parents 5e2003b + 886f267 commit 041aab5
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 42 deletions.
50 changes: 50 additions & 0 deletions cropper/app/lib/CropSpecMetadata.scala
Original file line number Diff line number Diff line change
@@ -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))
}

}
52 changes: 10 additions & 42 deletions cropper/app/lib/CropStore.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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]()) {
Expand All @@ -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 =
Expand Down
52 changes: 52 additions & 0 deletions cropper/test/lib/CropSpecMetadataTest.scala
Original file line number Diff line number Diff line change
@@ -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)
}
}

}

0 comments on commit 041aab5

Please sign in to comment.