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

# Conflicts:
#	core/src/test/scala/com/gu/etagcaching/LoadingTest.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

@@ -15,6 +15,8 @@ import org.scalatest.{Inside, OptionValues}
 
 import java.time.DayOfWeek
 import java.time.DayOfWeek.{MONDAY, SATURDAY, THURSDAY}
+import java.time.format.DateTimeFormatter
+import java.time.temporal.ChronoField.DAY_OF_WEEK
 import java.util.Locale
 import java.util.Locale.{FRANCE, GERMANY, UK}
 import scala.collection.mutable
@@ -51,6 +53,17 @@ class LoadingTest extends AnyFlatSpec with Matchers with ScalaFutures with Optio
     loading.fetchAndParse(GERMANY).futureValue.toOption.value shouldBe MONDAY
   }
 
+  it should "be possible with 'thenParsingWithKey', if we need the key for the 'parsing' phase" in {
+    val fetching: Fetching[Locale, String] =
+      TestFetching.withStubDataStore(Map(FRANCE -> "jeudi", GERMANY -> "Montag"))
+
+    val loading: Loading[Locale, DayOfWeek] = fetching.thenParsingWithKey((localeKey, response) =>
+      DayOfWeek.of(DateTimeFormatter.ofPattern("EEEE", localeKey).parse(response).get(DAY_OF_WEEK)))
+
+    loading.fetchAndParse(FRANCE).futureValue.toOption.value shouldBe THURSDAY
+    loading.fetchAndParse(GERMANY).futureValue.toOption.value shouldBe MONDAY
+  }
+
   "fetchThenParseIfNecessary" should "*only* do parsing if fetching found a change in ETag value" in {
     val dataStore = mutable.Map(UK -> "SATURDAY")
     val testApparatus = new TestApparatus(dataStore)(parser = DayOfWeek.valueOf)