Support support-fetching-then-parsing-with-key

# Conflicts:
#	core/src/main/scala/com/gu/etagcaching/Loading.scala
#	core/src/main/scala/com/gu/etagcaching/fetching/Fetching.scala

Author: rtyley

core/src/main/scala/com/gu/etagcaching/Loading.scala

@@ -8,11 +8,14 @@ import scala.concurrent.{ExecutionContext, Future}
 /**
  * `Loading` represents the two sequential steps of getting something useful from a remote resource, specifically:
  *
- *  - Fetching
+ *  - [[Fetching]]
  *  - Parsing
  *
- * Our [[Fetching]] interface requires supporting conditional-fetching based on `ETag`s, which means that
- * we can short-cut the Parsing step if the resource hasn't changed.
+ * The [[Fetching]] interface supports conditional-fetching based on `ETag`s, allowing us to short-cut the Parsing step
+ * if the resource hasn't changed.
+ *
+ * The easiest way to get a [[Loading]] instance is with a [[Fetching]] instance - just call
+ * [[Fetching.thenParsing]] to create a [[Loading]] instance.
  *
  * @tparam K The 'key' or resource identifier type - for instance, a URL or S3 Object Id.
  * @tparam V The 'value' for the key - a parsed representation of whatever was in the resource data.
@@ -34,17 +37,6 @@ trait Loading[K, V] {
 }
 
 object Loading {
-  def by[K, Response, V](fetching: Fetching[K, Response])(parse: Response => V)(implicit parsingEC: ExecutionContext): Loading[K, V] = new Loading[K, V] {
-    def fetchAndParse(key: K): Future[MissingOrETagged[V]] =
-      fetching.fetch(key).map(_.map(parse))
-
-    def fetchThenParseIfNecessary(key: K, oldV: ETaggedData[V]): Future[MissingOrETagged[V]] =
-      fetching.fetchOnlyIfETagChanged(key, oldV.eTag).map {
-        case None => oldV // we got HTTP 304 'NOT MODIFIED': there's no new data - old data is still valid
-        case Some(freshResponse) => freshResponse.map(parse)
-      }
-  }
-
   /**
    * Represents an update event for a given key.
    */

core/src/main/scala/com/gu/etagcaching/fetching/Fetching.scala

@@ -37,7 +37,31 @@ trait Fetching[K, Response] {
 
   def mapResponse[Response2](f: Response => Response2)(implicit ec: ExecutionContext): Fetching[K, Response2] = ResponseMapper(this)(f)
 
-  def thenParsing[V](parse: Response => V)(implicit parsingEC: ExecutionContext): Loading[K, V] = Loading.by(this)(parse)
+  /**
+   * Transforms this [[Fetching]] instance into a full [[Loading]] instance by saying how to parse
+   * the fetched response.
+   *
+   * If you happen to need the `key` to parse the response, use [[thenParsingWithKey]].
+   */
+  def thenParsing[V](parse: Response => V)(implicit parsingEC: ExecutionContext): Loading[K, V] = thenParsingWithKey((_, response) => parse(response))
+
+  /**
+   * Transforms this [[Fetching]] instance into a full [[Loading]] instance by saying how to parse
+   * the fetched response - in this particular case, the parsing method is passed the `key`
+   * as well as the fetched response.
+   *
+   * If you don't need the `key` to parse the response, just use [[thenParsing]].
+   */
+  def thenParsingWithKey[V](parse: (K, Response) => V)(implicit parsingEC: ExecutionContext): Loading[K, V] = new Loading[K, V] {
+    def fetchAndParse(key: K): Future[MissingOrETagged[V]] =
+      fetch(key).map(_.map(parse(key, _)))
+
+    def fetchThenParseIfNecessary(key: K, oldV: ETaggedData[V]): Future[MissingOrETagged[V]] =
+      fetchOnlyIfETagChanged(key, oldV.eTag).map {
+        case None => oldV // we got HTTP 304 'NOT MODIFIED': there's no new data - old data is still valid
+        case Some(freshResponse) => freshResponse.map(parse(key, _))
+      }
+  }
 }
 
 object Fetching {

core/src/test/scala/com/gu/etagcaching/CountingParser.scala

@@ -0,0 +1,15 @@
+package com.gu.etagcaching
+
+import java.util.concurrent.atomic.AtomicLong
+
+class CountingParser[K,V](parser: K => V) extends (K => V) {
+
+  private val counter = new AtomicLong()
+
+  override def apply(key: K): V = {
+    counter.incrementAndGet()
+    parser(key)
+  }
+
+  def count(): Long = counter.get()
+}

core/src/test/scala/com/gu/etagcaching/LoadingTest.scala

@@ -2,17 +2,75 @@ package com.gu.etagcaching
 
 import com.gu.etagcaching.FreshnessPolicy.TolerateOldValueWhileRefreshing
 import com.gu.etagcaching.Loading.Update
-import com.gu.etagcaching.fetching.Fetching
-import org.scalatest.OptionValues
+import com.gu.etagcaching.fetching.{ETaggedData, Fetching}
 import org.scalatest.concurrent.{Eventually, ScalaFutures}
 import org.scalatest.flatspec.AnyFlatSpec
 import org.scalatest.matchers.should.Matchers
+import org.scalatest.{Inside, OptionValues}
 
+import java.time.DayOfWeek.{SATURDAY, SUNDAY}
+import java.time.temporal.ChronoUnit.DAYS
+import java.time.{DayOfWeek, ZoneId, ZonedDateTime}
+import java.util.Locale
+import java.util.Locale.UK
 import scala.collection.mutable
 import scala.concurrent.ExecutionContext.Implicits.global
 import scala.concurrent.duration._
 
-class LoadingTest extends AnyFlatSpec with Matchers with ScalaFutures with OptionValues with Eventually {
+class LoadingTest extends AnyFlatSpec with Matchers with ScalaFutures with OptionValues with Eventually with Inside {
+  val exampleFetchingDayLength: Fetching[ZonedDateTime, java.time.Duration] = TestFetching.withLookup { dateTime =>
+    val day = dateTime.truncatedTo(DAYS)
+    Some(java.time.Duration.between(day, day.plusDays(1)))
+  }
+  val momentInShortUKDay: ZonedDateTime = ZonedDateTime.of(2025, 3, 30, 21, 55, 56, 0, ZoneId.of("Europe/London"))
+  val momentInNormalUSDay: ZonedDateTime = momentInShortUKDay.withZoneSameInstant(ZoneId.of("America/Chicago"))
+
+  "Creating a Loading instance from a Fetching instance" should "work with 'thenParsing'" in {
+    val loading: Loading[ZonedDateTime, Long] = exampleFetchingDayLength.thenParsing(_.toHours)
+
+    loading.fetchAndParse(momentInShortUKDay).futureValue.toOption.value shouldBe 23
+  }
+
+  it should "work with 'thenParsingWithKey'" in {
+    val loading: Loading[ZonedDateTime, String] =
+      exampleFetchingDayLength.thenParsingWithKey((key, response) => s"${key.getZone.getId.split('/')(1)}: ${response.toHours} hours")
+
+    loading.fetchAndParse(momentInShortUKDay).futureValue.toOption.value shouldBe "London: 23 hours"
+    loading.fetchAndParse(momentInNormalUSDay).futureValue.toOption.value shouldBe "Chicago: 24 hours"
+  }
+
+  private def loadingFavouriteDayOfWeekInDifferentCountries() = new TestLoading[Locale, String, DayOfWeek](DayOfWeek.valueOf)
+
+  "Loading" should "not do any parsing if fetching found no change" in {
+    val sample = loadingFavouriteDayOfWeekInDifferentCountries()
+
+    sample.dataStore(UK) = "SATURDAY"
+    inside(sample.loading.fetchAndParse(UK).futureValue) {
+      case initialLoad: ETaggedData[DayOfWeek] =>
+        sample.countingParser.count() shouldBe 1
+        initialLoad.result shouldBe SATURDAY
+
+        sample.loading.fetchThenParseIfNecessary(UK, initialLoad).futureValue.toOption.value shouldBe SATURDAY
+        sample.countingParser.count() shouldBe 1
+    }
+  }
+
+  it should "do parsing when values change" in {
+    val sample = loadingFavouriteDayOfWeekInDifferentCountries()
+
+    sample.dataStore(UK) = "SATURDAY"
+    inside(sample.loading.fetchAndParse(UK).futureValue) {
+      case initialLoad: ETaggedData[DayOfWeek] =>
+        sample.countingParser.count() shouldBe 1
+        initialLoad.result shouldBe SATURDAY
+
+        sample.dataStore(UK) = "SUNDAY"
+        sample.loading.fetchThenParseIfNecessary(UK, initialLoad).futureValue.toOption.value shouldBe SUNDAY
+        sample.countingParser.count() shouldBe 2
+    }
+  }
+
+
   "onUpdate" should "give callbacks that allow logging updates" in {
     val updates: mutable.Buffer[Update[String, Int]] = mutable.Buffer.empty
 
@@ -43,3 +101,9 @@ class LoadingTest extends AnyFlatSpec with Matchers with ScalaFutures with Optio
     updates.toSeq shouldBe expectedUpdates // No updates if we're not requesting the key from the cache
   }
 }
+
+class TestLoading[K, Response, V](parser: Response => V) {
+  val dataStore = mutable.Map[K, Response]()
+  val countingParser = new CountingParser[Response, V](parser)
+  val loading: Loading[K, V] = TestFetching.withLookup(dataStore.get).thenParsing(countingParser)
+}

core/src/test/scala/com/gu/etagcaching/TestFetching.scala

@@ -1,6 +1,6 @@
 package com.gu.etagcaching
 
-import com.gu.etagcaching.fetching.{ETaggedData, Fetching, MissingOrETagged}
+import com.gu.etagcaching.fetching.{ETaggedData, Fetching, Missing, MissingOrETagged}
 
 import java.util.concurrent.atomic.AtomicInteger
 import scala.concurrent.{ExecutionContext, Future}
@@ -17,4 +17,25 @@ object TestFetching {
     override def fetchOnlyIfETagChanged(key: String, eTag: String): Future[Option[MissingOrETagged[Int]]] =
       fetch(key).map(Some(_))(ExecutionContext.parasitic)
   }
+
+  /**
+   * Create a test [[Fetching]] instance with a (possibly impure) `lookup` function.
+   *
+   * The [[Fetching]] instance will use the object hashcode to create the object's ETag.
+   *
+   * The `lookup` function can return different values for the same input if we want to simulate
+   * the value for a key changing over time.
+   */
+  def withLookup[K, V](lookup: K => Option[V]): Fetching[K, V] = new Fetching[K, V] {
+    override def fetch(key: K): Future[MissingOrETagged[V]] = Future.successful {
+      lookup(key).fold[MissingOrETagged[V]](Missing) { value =>
+        ETaggedData(value.hashCode().toString, value)
+      }
+    }
+    override def fetchOnlyIfETagChanged(key: K, ETag: String): Future[Option[MissingOrETagged[V]]] =
+      fetch(key).map {
+        case ETaggedData(ETag, _) => None
+        case other => Some(other)
+      }(ExecutionContext.parasitic)
+  }
 }