From 9e2de5afb91f54d1564e663ef71be254e492e3fd Mon Sep 17 00:00:00 2001 From: Roberto Tyley <roberto.tyley@guardian.co.uk> Date: Tue, 24 May 2022 22:39:02 +0100 Subject: [PATCH] Support pagination even for queries that can not specify order-by Get rid of Aux[Q,R] Allow specifying direction to get next search query We're trying to support LightBox functionality that was introduced here: https://github.com/guardian/frontend/pull/20462 Co-authored-by: Roberto Tyley <roberto.tyley@guardian.co.uk> Support direction for all paginatable queries, and remove unused PaginatedApiResponse classes --- .../GuardianContentClientTest.scala | 46 ++++++- .../ContentApiClient.scala | 64 ++++------ .../com.gu.contentapi.client/Parameters.scala | 2 + .../model/Decoder.scala | 58 ++++----- .../model/PaginatedApiResponse.scala | 31 ----- .../model/Queries.scala | 119 +++++++++++++++--- .../model/package.scala | 9 +- .../ContentApiClientTest.scala | 44 +++++-- 8 files changed, 230 insertions(+), 143 deletions(-) delete mode 100644 client/src/main/scala/com.gu.contentapi.client/model/PaginatedApiResponse.scala diff --git a/client-default/src/test/scala/com.gu.contentapi.client/GuardianContentClientTest.scala b/client-default/src/test/scala/com.gu.contentapi.client/GuardianContentClientTest.scala index 3d119321..a06055e6 100644 --- a/client-default/src/test/scala/com.gu.contentapi.client/GuardianContentClientTest.scala +++ b/client-default/src/test/scala/com.gu.contentapi.client/GuardianContentClientTest.scala @@ -1,14 +1,14 @@ package com.gu.contentapi.client -import java.time.Instant - -import com.gu.contentapi.client.model.v1.{ContentType, ErrorResponse, SearchResponse} -import com.gu.contentapi.client.model.{ContentApiError, ItemQuery, SearchQuery} +import com.gu.contentapi.client.model.Direction.{Next, Previous} +import com.gu.contentapi.client.model.v1.{ContentType, ErrorResponse, SearchResponse, TagsResponse} +import com.gu.contentapi.client.model.{ContentApiError, FollowingSearchQuery, ItemQuery, SearchQuery} import com.gu.contentatom.thrift.{AtomData, AtomType} import org.scalatest._ import org.scalatest.concurrent.{IntegrationPatience, ScalaFutures} import org.scalatest.time.{Seconds, Span} +import java.time.Instant import scala.concurrent.ExecutionContext.Implicits.global object GuardianContentClientTest { @@ -60,6 +60,13 @@ class GuardianContentClientTest extends FlatSpec with Matchers with ScalaFutures content.futureValue.id should be (TestItemPath) } + it should "perform a given sections query" in { + val query = ContentApiClient.sections.q("business") + val results = for (response <- api.getResponse(query)) yield response.results + val fResults = results.futureValue + fResults.size should be >= 3 // UK business, US business, all da business + } + it should "perform a given atoms query" in { val query = ContentApiClient.atoms.types("explainer") val results = for (response <- api.getResponse(query)) yield response.results @@ -88,6 +95,24 @@ class GuardianContentClientTest extends FlatSpec with Matchers with ScalaFutures } } + it should "be able to find previous and next articles in a series" in { + // This kind of functionality is used eg. for the DotCom image lightbox forward-backward buttons: + // https://github.com/guardian/frontend/pull/20462 + val queryForNext = FollowingSearchQuery( + ContentApiClient.search.tag("commentisfree/series/guardian-comment-cartoon").orderBy("oldest").pageSize(1), + "commentisfree/picture/2022/may/15/nicola-jennings-boris-johnson-northern-ireland-cartoon", + Next + ) + + api.getResponse(queryForNext).futureValue.results.head.id shouldBe + "commentisfree/picture/2022/may/16/steve-bell-on-the-queens-platinum-jubilee-cartoon" + + val queryForPrevious = queryForNext.copy(direction = Previous) + + api.getResponse(queryForPrevious).futureValue.results.head.id shouldBe + "commentisfree/picture/2022/may/13/martin-rowson-whats-left-in-the-tories-box-of-tricks-cartoon" + } + it should "paginate through all results" in { val query = ContentApiClient.search .q("brexit") @@ -116,6 +141,19 @@ class GuardianContentClientTest extends FlatSpec with Matchers with ScalaFutures result.futureValue should be (42) } + it should "be able to paginate any query that supports paging, eg Tags!" in { + val query = ContentApiClient.tags + .tagType("newspaper-book") + .section("music") + .pageSize(5) + // https://content.guardianapis.com/tags?type=newspaper-book§ion=music&page-size=5 + // has 19 results as of May 2022 + + val result = api.paginateAccum(query)({ r: TagsResponse => r.results.length }, { (a: Int, b: Int) => a + b }) + + result.futureValue should be > 10 + } + it should "fold over the results" in { val query = ContentApiClient.search .q("brexit") diff --git a/client/src/main/scala/com.gu.contentapi.client/ContentApiClient.scala b/client/src/main/scala/com.gu.contentapi.client/ContentApiClient.scala index a07c6001..4d51cc79 100644 --- a/client/src/main/scala/com.gu.contentapi.client/ContentApiClient.scala +++ b/client/src/main/scala/com.gu.contentapi.client/ContentApiClient.scala @@ -2,14 +2,16 @@ package com.gu.contentapi.client import com.gu.contentapi.buildinfo.CapiBuildInfo import com.gu.contentapi.client.HttpRetry.withRetry +import com.gu.contentapi.client.model.Direction.Next import com.gu.contentapi.client.model.HttpResponse.isSuccessHttpResponse import com.gu.contentapi.client.model._ +import com.gu.contentapi.client.thrift.ThriftDeserializer import com.gu.contentatom.thrift.AtomType +import com.twitter.scrooge.{ThriftStruct, ThriftStructCodec} import scala.concurrent.{ExecutionContext, Future} trait ContentApiClient { - import Decoder._ /** Your API key */ def apiKey: String @@ -46,7 +48,7 @@ trait ContentApiClient { /** Streamlines the handling of a valid CAPI response */ - private def fetchResponse(contentApiQuery: ContentApiQuery)(implicit context: ExecutionContext): Future[Array[Byte]] = get(url(contentApiQuery), headers).flatMap { + private def fetchResponse(contentApiQuery: ContentApiQuery[_])(implicit ec: ExecutionContext): Future[Array[Byte]] = get(url(contentApiQuery), headers).flatMap { case response if isSuccessHttpResponse(response) => Future.successful(response) case response => Future.failed(ContentApiError(response)) }.map(_.body) @@ -59,7 +61,7 @@ trait ContentApiClient { } } - def url(contentApiQuery: ContentApiQuery): String = + def url(contentApiQuery: ContentApiQuery[_]): String = contentApiQuery.getUrl(targetUrl, parameters) /** Runs the query against the Content API. @@ -68,10 +70,10 @@ trait ContentApiClient { * @param query the query * @return a future resolving to an unmarshalled response */ - def getResponse[Q <: ContentApiQuery](query: Q)( - implicit - decoder: Decoder[Q], - context: ExecutionContext): Future[decoder.Response] = + def getResponse[R <: ThriftStruct](query: ContentApiQuery[R])( + implicit + decoder: Decoder[R], + context: ExecutionContext): Future[R] = fetchResponse(query) map decoder.decode /** Unfolds a query to its results, page by page @@ -82,15 +84,12 @@ trait ContentApiClient { * @param f a result-processing function * @return a future of a list of result-processed results */ - def paginate[Q <: PaginatedApiQuery[Q], R, M](query: Q)(f: R => M)( - implicit - decoder: Decoder.Aux[Q, R], - pager: PaginatedApiResponse[R], + def paginate[R <: ThriftStruct: Decoder, E, M](query: PaginatedApiQuery[R, E])(f: R => M)( + implicit context: ExecutionContext ): Future[List[M]] = unfoldM { r: R => - val nextQuery = pager.getNextId(r).map { id => getResponse(ContentApiClient.next(query, id)) } - (f(r), nextQuery) + (f(r), query.followingQueryGiven(r, Next).map(getResponse(_))) }(getResponse(query)) /** Unfolds a query by accumulating its results @@ -101,10 +100,8 @@ trait ContentApiClient { * @param f a result-processing function * @return a future of an accumulated value */ - def paginateAccum[Q <: PaginatedApiQuery[Q], R, M](query: Q)(f: R => M, g: (M, M) => M)( - implicit - decoder: Decoder.Aux[Q, R], - pager: PaginatedApiResponse[R], + def paginateAccum[R <: ThriftStruct: Decoder, E, M](query: PaginatedApiQuery[R, E])(f: R => M, g: (M, M) => M)( + implicit context: ExecutionContext ): Future[M] = paginate(query)(f).map { @@ -114,26 +111,22 @@ trait ContentApiClient { } /** Unfolds a query by accumulating its results - * - * @tparam Q the type of a Content API query with pagination parameters + * * @tparam R the type of response expected for `Q` * @param query the initial query * @param f a result-processing function * @return a future of an accumulated value */ - def paginateFold[Q <: PaginatedApiQuery[Q], R, M](query: Q)(m: M)(f: (R, M) => M)( - implicit - decoder: Decoder.Aux[Q, R], - decoderNext: Decoder.Aux[NextQuery[Q], R], - pager: PaginatedApiResponse[R], + def paginateFold[R <: ThriftStruct: Decoder, E, M](query: PaginatedApiQuery[R, E])(m: M)(f: (R, M) => M)( + implicit context: ExecutionContext ): Future[M] = { - def paginateFoldIn(nextQuery: Option[NextQuery[Q]])(m: M): Future[M] = { - val req = nextQuery.map(getResponse(_)).getOrElse(getResponse(query)) + def paginateFoldIn(currentQuery: Option[PaginatedApiQuery[R, E]])(m: M): Future[M] = { + val req = currentQuery.map(getResponse(_)).getOrElse(getResponse(query)) req.flatMap { r: R => - pager.getNextId(r) match { + query.followingQueryGiven(r, Next) match { case None => Future.successful(f(r, m)) - case Some(id) => paginateFoldIn(Some(ContentApiClient.next(query, id)))(f(r, m)) + case Some(nextQuery) => paginateFoldIn(Some(nextQuery))(f(r, m)) } } } @@ -169,19 +162,4 @@ trait ContentApiQueries { val restaurantReviews = RestaurantReviewsQuery() val filmReviews = FilmReviewsQuery() val videoStats = VideoStatsQuery() - def next[Q <: PaginatedApiQuery[Q]](q: Q, id: String) = NextQuery(normalize(q), id) - - private def normalize[Q <: PaginatedApiQuery[Q]]: Q => Q = - normalizePageSize andThen normalizeOrder - - private def normalizePageSize[Q <: PaginatedApiQuery[Q]]: Q => Q = - q => if (q.has("page-size")) q else q.pageSize(10) - - private def normalizeOrder[Q <: PaginatedApiQuery[Q]]: Q => Q = - q => if (q.has("order-by")) - q - else if (q.has("q")) - q.orderBy("relevance") - else - q.orderBy("newest") } \ No newline at end of file diff --git a/client/src/main/scala/com.gu.contentapi.client/Parameters.scala b/client/src/main/scala/com.gu.contentapi.client/Parameters.scala index e4600fc4..e6c5b5a7 100644 --- a/client/src/main/scala/com.gu.contentapi.client/Parameters.scala +++ b/client/src/main/scala/com.gu.contentapi.client/Parameters.scala @@ -40,11 +40,13 @@ trait Parameters[Owner <: Parameters[Owner]] { self: Owner => case class StringParameter(name: String, value: Option[String] = None) extends OwnedParameter { type Self = String def withValue(newValue: Option[String]) = copy(value = newValue) + def setIfUndefined(str: String) = if (owner.has(name)) owner else apply(str) } case class IntParameter(name: String, value: Option[Int] = None) extends OwnedParameter { type Self = Int def withValue(newValue: Option[Int]) = copy(value = newValue) + def setIfUndefined(v: Int) = if (owner.has(name)) owner else apply(v) } case class DateParameter(name: String, value: Option[Instant] = None) extends OwnedParameter { diff --git a/client/src/main/scala/com.gu.contentapi.client/model/Decoder.scala b/client/src/main/scala/com.gu.contentapi.client/model/Decoder.scala index bc2386bd..f2694feb 100644 --- a/client/src/main/scala/com.gu.contentapi.client/model/Decoder.scala +++ b/client/src/main/scala/com.gu.contentapi.client/model/Decoder.scala @@ -1,48 +1,34 @@ package com.gu.contentapi.client -import com.gu.contentapi.client.model._ import com.gu.contentapi.client.model.v1._ import com.gu.contentapi.client.thrift.ThriftDeserializer import com.twitter.scrooge.{ThriftStruct, ThriftStructCodec} -/** Typeclass witnessing how to unmarshall a Thrift stream of bytes - * into a concrete data type - * @tparam Query the query type - */ -trait Decoder[Query] { - /** the response type corresponding to `Query` */ - type Response <: ThriftStruct - /** the codec unmarshalling instances of `Response` */ - def codec: ThriftStructCodec[Response] - /** performs the unmarshalling - * @return a function taking an array of bytes into a `Response` - */ - def decode: Array[Byte] => Response = ThriftDeserializer.deserialize(_, codec) -} -private[client] object Decoder { +class Decoder[Response <: ThriftStruct](codec: ThriftStructCodec[Response]) { + def decode(data: Array[Byte]): Response = ThriftDeserializer.deserialize(data, codec) +} - type Aux[Q, R] = Decoder[Q] { type Response = R } +trait PaginationDecoder[Response, Element] { + val pageSize: Response => Int + val elements: Response => collection.Seq[Element] +} - private def apply[Q, R <: ThriftStruct](c: ThriftStructCodec[R]) = new Decoder[Q] { - type Response = R - def codec = c - } +object Decoder { + type PageableResponseDecoder[Response <: ThriftStruct, Element] = Decoder[Response] with PaginationDecoder[Response, Element] - private def atomsDecoder[Query] = apply[Query, AtomsResponse](AtomsResponse) + def pageableResponseDecoder[R <: ThriftStruct, E](c: ThriftStructCodec[R])(ps: R => Int, el: R => collection.Seq[E]): PageableResponseDecoder[R, E] = + new Decoder[R](c) with PaginationDecoder[R, E] { + val pageSize: R => Int = ps + val elements: R => collection.Seq[E] = el + } - implicit val itemQuery = apply[ItemQuery, ItemResponse](ItemResponse) - implicit val tagsQuery = apply[TagsQuery, TagsResponse](TagsResponse) - implicit val sectionsQuery = apply[SectionsQuery, SectionsResponse](SectionsResponse) - implicit val editionsQuery = apply[EditionsQuery, EditionsResponse](EditionsResponse) - implicit val videoStatsQuery = apply[VideoStatsQuery, VideoStatsResponse](VideoStatsResponse) - implicit val atomsQuery = atomsDecoder[AtomsQuery] - implicit val recipesQuery = atomsDecoder[RecipesQuery] - implicit val reviewsQuery = atomsDecoder[ReviewsQuery] - implicit val gameReviewsQuery = atomsDecoder[GameReviewsQuery] - implicit val restaurantReviewsQuery = atomsDecoder[RestaurantReviewsQuery] - implicit val filmReviewsQuery = atomsDecoder[FilmReviewsQuery] - implicit def searchQueryBase[T <: SearchQueryBase[T]] = apply[T, SearchResponse](SearchResponse) - implicit def nextQuery[Q <: PaginatedApiQuery[Q]](implicit d: Decoder[Q]) = apply[NextQuery[Q], d.Response](d.codec) - implicit def atomsUsageQuery = apply[AtomUsageQuery, AtomUsageResponse](AtomUsageResponse) + implicit val itemDecoder: Decoder[ItemResponse] = new Decoder(ItemResponse) + implicit val tagsDecoder: PageableResponseDecoder[TagsResponse, Tag] = pageableResponseDecoder(TagsResponse)(_.pageSize, _.results) + implicit val sectionsQuery: Decoder[SectionsResponse] = new Decoder(SectionsResponse) + implicit val editionsDecoder: Decoder[EditionsResponse] = new Decoder(EditionsResponse) + implicit val videoStatsDecoder: Decoder[VideoStatsResponse] = new Decoder(VideoStatsResponse) + implicit val atomsDecoder: Decoder[AtomsResponse] = new Decoder(AtomsResponse) + implicit val searchDecoder: PageableResponseDecoder[SearchResponse, Content] = pageableResponseDecoder(SearchResponse)(_.pageSize, _.results) + implicit val atomUsageDecoder: Decoder[AtomUsageResponse] = new Decoder(AtomUsageResponse) } diff --git a/client/src/main/scala/com.gu.contentapi.client/model/PaginatedApiResponse.scala b/client/src/main/scala/com.gu.contentapi.client/model/PaginatedApiResponse.scala deleted file mode 100644 index 116877dc..00000000 --- a/client/src/main/scala/com.gu.contentapi.client/model/PaginatedApiResponse.scala +++ /dev/null @@ -1,31 +0,0 @@ -package com.gu.contentapi.client - -import com.gu.contentapi.client.model.v1._ -import com.gu.contentatom.thrift.Atom - -/** Typeclass witnessing how to extract the id for - * a paginated query - */ -trait PaginatedApiResponse[Response] { - /** the id for a next query, if any */ - def getNextId: Response => Option[String] -} - -/** Instances of [[PaginatedApiResponse]] for response types - * potentially containing pages of results - */ -object PaginatedApiResponse { - - implicit val searchResponse = new PaginatedApiResponse[SearchResponse] { - def getNextId = r => if (r.results.length < r.pageSize) None else r.results.lastOption.map(_.id) - } - - implicit val tagsResponse = new PaginatedApiResponse[TagsResponse] { - def getNextId = r => if (r.results.length < r.pageSize) None else r.results.lastOption.map(_.id) - } - - implicit val atomsResponse = new PaginatedApiResponse[AtomsResponse] { - def getNextId = r => if (r.results.length < r.pageSize) None else r.results.lastOption.map(_.id) - } - -} \ No newline at end of file diff --git a/client/src/main/scala/com.gu.contentapi.client/model/Queries.scala b/client/src/main/scala/com.gu.contentapi.client/model/Queries.scala index e7ad2be8..6aee8f8e 100644 --- a/client/src/main/scala/com.gu.contentapi.client/model/Queries.scala +++ b/client/src/main/scala/com.gu.contentapi.client/model/Queries.scala @@ -1,10 +1,14 @@ package com.gu.contentapi.client.model +import com.gu.contentapi.client.Decoder.PageableResponseDecoder +import com.gu.contentapi.client.model.Direction.Next +import com.gu.contentapi.client.model.v1.{AtomUsageResponse, AtomsResponse, Content, EditionsResponse, ItemResponse, SearchResponse, SectionsResponse, Tag, TagsResponse, VideoStatsResponse} import com.gu.contentapi.client.utils.QueryStringParams import com.gu.contentapi.client.{Parameter, Parameters} import com.gu.contentatom.thrift.AtomType +import com.twitter.scrooge.ThriftStruct -sealed trait ContentApiQuery { +sealed trait ContentApiQuery[Response <: ThriftStruct] { def parameters: Map[String, String] def pathSegment: String @@ -18,11 +22,36 @@ sealed trait ContentApiQuery { } def getUrl(targetUrl: String, customParameters: Map[String, String] = Map.empty): String = - url(s"$targetUrl/${pathSegment}", parameters ++ customParameters) + url(s"$targetUrl/$pathSegment", parameters ++ customParameters) + +} + +abstract class PaginatedApiQuery[Response <: ThriftStruct, Element]( + implicit prd: PageableResponseDecoder[Response, Element] +) extends ContentApiQuery[Response] { + + /** + * Produce a version of this query that explicitly sets previously ''unset'' pagination/ordering parameters, + * matching how the Content API server decided to process the previous request. + * + * For instance, if the Content API decided to process https://content.guardianapis.com/search?q=brexit + * with pageSize:10 & orderBy:relevance, that will have been detailed in the CAPI response - and therefore we + * can copy those parameters into our following query so we don't change how we're ordering the results + * as we paginate through them! + */ + def setPaginationConsistentWith(response: Response): PaginatedApiQuery[Response, Element] + + def followingQueryGiven(response: Response, direction: Direction): Option[PaginatedApiQuery[Response, Element]] = + if (response.impliesNoFurtherResults) None else setPaginationConsistentWith(response).followingQueryGivenFull(response, direction) + + /** Construct a query for the subsequent results after this response. This method will only be called if the + * response was supplied a full page of results, meaning that there's the possibility of more results to fetch. + */ + protected def followingQueryGivenFull(response: Response, direction: Direction): Option[PaginatedApiQuery[Response, Element]] } trait SearchQueryBase[Self <: SearchQueryBase[Self]] - extends ContentApiQuery + extends PaginatedApiQuery[SearchResponse, Content] with ShowParameters[Self] with ShowReferencesParameters[Self] with OrderByParameter[Self] @@ -35,7 +64,7 @@ trait SearchQueryBase[Self <: SearchQueryBase[Self]] } case class ItemQuery(id: String, parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[ItemResponse] with EditionParameters[ItemQuery] with ShowParameters[ItemQuery] with ShowReferencesParameters[ItemQuery] @@ -58,26 +87,42 @@ case class ItemQuery(id: String, parameterHolder: Map[String, Parameter] = Map.e case class SearchQuery(parameterHolder: Map[String, Parameter] = Map.empty) extends SearchQueryBase[SearchQuery] { + def setPaginationConsistentWith(response: SearchResponse): PaginatedApiQuery[SearchResponse, Content] = + pageSize.setIfUndefined(response.pageSize).orderBy.setIfUndefined(response.orderBy) + def withParameters(parameterMap: Map[String, Parameter]): SearchQuery = copy(parameterMap) override def pathSegment: String = "search" + + protected override def followingQueryGivenFull(response: SearchResponse, direction: Direction) = for { + lastResultInResponse <- response.results.lastOption + } yield FollowingSearchQuery(this, lastResultInResponse.id, direction) + } case class TagsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends PaginatedApiQuery[TagsResponse, Tag] with ShowReferencesParameters[TagsQuery] with PaginationParameters[TagsQuery] with FilterParameters[TagsQuery] with FilterTagParameters[TagsQuery] with FilterSearchParameters[TagsQuery] { + def setPaginationConsistentWith(response: TagsResponse): PaginatedApiQuery[TagsResponse, Tag] = + pageSize.setIfUndefined(response.pageSize) + def withParameters(parameterMap: Map[String, Parameter]) = copy(parameterMap) override def pathSegment: String = "tags" + + protected override def followingQueryGivenFull(response: TagsResponse, direction: Direction): Option[TagsQuery] = { + val followingPage = response.currentPage + direction.delta + if (followingPage >= 1 && followingPage <= response.pages) Some(page(followingPage)) else None + } } case class SectionsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[SectionsResponse] with FilterSearchParameters[SectionsQuery] with FilterSectionParameters[SectionsQuery]{ @@ -87,7 +132,7 @@ case class SectionsQuery(parameterHolder: Map[String, Parameter] = Map.empty) } case class EditionsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[EditionsResponse] with FilterSearchParameters[EditionsQuery] { def withParameters(parameterMap: Map[String, Parameter]) = copy(parameterMap) @@ -99,7 +144,7 @@ case class VideoStatsQuery( edition: Option[String] = None, section: Option[String] = None, parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[VideoStatsResponse] with FilterSearchParameters[VideoStatsQuery] { def withParameters(parameterMap: Map[String, Parameter]) = copy(edition, section, parameterMap) @@ -108,7 +153,7 @@ case class VideoStatsQuery( } case class AtomsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[AtomsResponse] with AtomsParameters[AtomsQuery] with PaginationParameters[AtomsQuery] with UseDateParameter[AtomsQuery] @@ -121,16 +166,16 @@ case class AtomsQuery(parameterHolder: Map[String, Parameter] = Map.empty) } case class AtomUsageQuery(atomType: AtomType, atomId: String, parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[AtomUsageResponse] with PaginationParameters[AtomUsageQuery] { - + def withParameters(parameterMap: Map[String, Parameter]) = copy(parameterHolder = parameterMap) override def pathSegment: String = s"atom/${atomType.toString.toLowerCase}/$atomId/usage" } case class RecipesQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[AtomsResponse] with PaginationParameters[RecipesQuery] with RecipeParameters[RecipesQuery] { @@ -140,7 +185,7 @@ case class RecipesQuery(parameterHolder: Map[String, Parameter] = Map.empty) } case class ReviewsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[AtomsResponse] with PaginationParameters[ReviewsQuery] with ReviewSpecificParameters[ReviewsQuery] { @@ -150,7 +195,7 @@ case class ReviewsQuery(parameterHolder: Map[String, Parameter] = Map.empty) } case class GameReviewsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[AtomsResponse] with ReviewSpecificParameters[GameReviewsQuery] with PaginationParameters[GameReviewsQuery] with GameParameters[GameReviewsQuery] { @@ -161,7 +206,7 @@ case class GameReviewsQuery(parameterHolder: Map[String, Parameter] = Map.empty) } case class RestaurantReviewsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[AtomsResponse] with ReviewSpecificParameters[RestaurantReviewsQuery] with PaginationParameters[RestaurantReviewsQuery] with RestaurantParameters[RestaurantReviewsQuery] { @@ -172,7 +217,7 @@ case class RestaurantReviewsQuery(parameterHolder: Map[String, Parameter] = Map. } case class FilmReviewsQuery(parameterHolder: Map[String, Parameter] = Map.empty) - extends ContentApiQuery + extends ContentApiQuery[AtomsResponse] with ReviewSpecificParameters[FilmReviewsQuery] with PaginationParameters[FilmReviewsQuery] with FilmParameters[FilmReviewsQuery] { @@ -182,12 +227,46 @@ case class FilmReviewsQuery(parameterHolder: Map[String, Parameter] = Map.empty) override def pathSegment: String = "atoms/reviews/film" } -case class NextQuery[Q <: PaginatedApiQuery[Q]](originalQuery: Q, contentId: String) - extends ContentApiQuery { - +sealed trait Direction { + val pathSegment: String + val delta: Int + def guidingElementIn[T](elements: Iterable[T]): Option[T] +} + +object Direction { + object Next extends Direction { + override val pathSegment: String = "next" + override val delta: Int = 1 + override def guidingElementIn[T](elements: Iterable[T]): Option[T] = elements.lastOption + + } + object Previous extends Direction { + override val pathSegment: String = "prev" + override val delta: Int = -1 + override def guidingElementIn[T](elements: Iterable[T]): Option[T] = elements.headOption + } + + def forPathSegment(pathSegment: String): Direction = pathSegment match { + case Next.pathSegment => Next + case Previous.pathSegment => Previous + } +} + +case class FollowingSearchQuery( + originalQuery: PaginatedApiQuery[SearchResponse, Content], contentId: String, direction: Direction = Next +) extends PaginatedApiQuery[SearchResponse, Content] { + def parameters: Map[String, String] = originalQuery.parameters.filterKeys(not(isPaginationParameter)).toMap - override def pathSegment: String = s"""content/${contentId}/next""" + override def pathSegment: String = s"content/$contentId/${direction.pathSegment}" + + override def setPaginationConsistentWith(response: SearchResponse): PaginatedApiQuery[SearchResponse, Content] = + originalQuery.setPaginationConsistentWith(response) + + protected override def followingQueryGivenFull(response: SearchResponse, updatedDirection: Direction): Option[PaginatedApiQuery[SearchResponse, Content]] = for { + content <- updatedDirection.guidingElementIn(response.results) + } yield copy(contentId = content.id, direction = updatedDirection) + } trait EditionParameters[Owner <: Parameters[Owner]] extends Parameters[Owner] { this: Owner => diff --git a/client/src/main/scala/com.gu.contentapi.client/model/package.scala b/client/src/main/scala/com.gu.contentapi.client/model/package.scala index 260e68c8..d8921157 100644 --- a/client/src/main/scala/com.gu.contentapi.client/model/package.scala +++ b/client/src/main/scala/com.gu.contentapi.client/model/package.scala @@ -1,7 +1,14 @@ package com.gu.contentapi.client +import com.gu.contentapi.client.Decoder.PageableResponseDecoder +import com.twitter.scrooge.ThriftStruct + package object model { - type PaginatedApiQuery[A <: Parameters[A]] = ContentApiQuery with PaginationParameters[A] with OrderByParameter[A] + + implicit class RichPageableResponse[R <: ThriftStruct, E](response: R)(implicit prd: PageableResponseDecoder[R, E]) { + + val impliesNoFurtherResults: Boolean = prd.elements(response).size < prd.pageSize(response) + } private[model] def not[A](f: A => Boolean): A => Boolean = !f(_) diff --git a/client/src/test/scala/com.gu.contentapi.client/ContentApiClientTest.scala b/client/src/test/scala/com.gu.contentapi.client/ContentApiClientTest.scala index 90f78fb1..3a3eae94 100644 --- a/client/src/test/scala/com.gu.contentapi.client/ContentApiClientTest.scala +++ b/client/src/test/scala/com.gu.contentapi.client/ContentApiClientTest.scala @@ -1,14 +1,14 @@ package com.gu.contentapi.client -import java.time.Instant -import java.util.concurrent.TimeUnit - +import com.gu.contentapi.client.model.Direction.{Next, Previous} import com.gu.contentapi.client.model._ -import com.gu.contentapi.client.model.v1.SearchResponse +import com.gu.contentapi.client.model.v1.{Content, SearchResponse} import org.scalatest._ import org.scalatest.concurrent.ScalaFutures import org.scalatest.time.{Seconds, Span} +import java.time.Instant +import java.util.concurrent.TimeUnit import scala.concurrent.ExecutionContext.Implicits.global import scala.concurrent.duration.Duration import scala.concurrent.{ExecutionContext, Future} @@ -29,6 +29,19 @@ class ContentApiClientTest extends FlatSpec with Matchers with ScalaFutures with api.url(ContentApiClient.search) should include(s"api-key=${api.apiKey}") } + it should "generate the correct path segments for following a query" in { + val queryForNext = FollowingSearchQuery( + ContentApiClient.search, + "commentisfree/picture/2022/may/15/nicola-jennings-boris-johnson-northern-ireland-cartoon", + Next + ) + + api.url(queryForNext) should include(s"content/commentisfree/picture/2022/may/15/nicola-jennings-boris-johnson-northern-ireland-cartoon/next") + + val queryForPrevious = queryForNext.copy(direction = Previous) + api.url(queryForPrevious) should include(s"content/commentisfree/picture/2022/may/15/nicola-jennings-boris-johnson-northern-ireland-cartoon/prev") + } + it should "understand custom parameters" in { val now = Instant.now() val params = ContentApiClient.search @@ -46,18 +59,33 @@ class ContentApiClientTest extends FlatSpec with Matchers with ScalaFutures with behavior of "Paginated queries" + def stubContent(capiId: String): Content = Content(capiId, webTitle="", webUrl="", apiUrl="") + def stubContents(num: Int) = (1 to num).map(i => stubContent(s"blah-$i")) + def stubSearchResponse(pageSize: Int, orderBy: String, results: Seq[Content]): SearchResponse = SearchResponse( + status = "", userTier="", total = -1, startIndex = -1, currentPage = -1, pages= -1, orderBy = orderBy, + pageSize = pageSize, // Needed for deciding next query + results = results) + it should "produce next urls for 10 results ordered by relevance" in { val query = ContentApiClient.search.q("brexit") - val next = ContentApiClient.next(query, "hello") + val next = query.followingQueryGiven(stubSearchResponse( + pageSize = 10, + orderBy = "relevance", + stubContents(9) :+ stubContent("hello") + ), Next).value testPaginatedQuery("content/hello/next", 10, "relevance", Some("brexit"))(next) } it should "produce next urls for 20 results order by newest" in { val query = ContentApiClient.search.pageSize(20) - val next = ContentApiClient.next(query, "hello") + val next = query.followingQueryGiven(stubSearchResponse( + pageSize = 20, + orderBy = "newest", + stubContents(19) :+ stubContent("hello") + ), Next).value - testPaginatedQuery("content/hello/", 20, "newest")(next) + testPaginatedQuery("content/hello/next", 20, "newest")(next) } it should "recover gracefully from error" in { @@ -72,7 +100,7 @@ class ContentApiClientTest extends FlatSpec with Matchers with ScalaFutures with errorTest.futureValue } - def testPaginatedQuery(pt: String, page: Int, ob: String, q: Option[String] = None)(query: ContentApiQuery) = { + def testPaginatedQuery(pt: String, page: Int, ob: String, q: Option[String] = None)(query: ContentApiQuery[_]) = { val ps = query.parameters query.pathSegment should startWith (pt) ps.get("page-size") should be (Some(page.toString))