Skip to content

Commit

Permalink
Use JsValue Not String For DCAR (#27641)
Browse files Browse the repository at this point in the history
Frontend posts the data for articles, fronts, tags pages and so on to DCAR as JSON, using `WSClient`. At the moment the following steps are taken to transform from an instance of a case class in frontend to the format that `WSClient` uses (`BodyWritable`):

```
case class instance -> JsValue -> String -> BodyWritable
```

The final transformation here, `String -> BodyWritable`, is handled by `WSClient`, as it defines an implicit way to get from one type to the other. However, it also defines a way to do this for a `play-json` `JsValue`, so we do not need to convert to a `String` beforehand. We should be able to pass a `JsValue` to `WSClient` directly, removing one of the transformation steps:

```
case class instance -> JsValue -> BodyWritable
```

The way to achieve this is by modifying our methods that use `WSClient.post` to take a `JsValue` payload instead of a `String`. We are then able to update the `toJson` methods on each of our models to remove the `Json.stringify` step. However, aside from `RemoteRender` requests, these `toJson` methods are also used to provide for `JsonFormat` requests via the `renderJson` method, which at the moment requires a `String`. This method, though, makes use of Play's `Writeable`, which also includes an implicit transformation to get from a `play-json` `JsValue`. So we can update it to take a `JsValue` instead, and save the additional `String` transformation step here too.

Note that there is one exception where we're using `circe` instead of `play-json`, for crosswords, so in that case we're transforming from one to the other via `String`.
  • Loading branch information
JamieB-gu authored Dec 13, 2024
1 parent 0361ab8 commit 9c8a254
Show file tree
Hide file tree
Showing 14 changed files with 37 additions and 28 deletions.
3 changes: 2 additions & 1 deletion applications/app/controllers/CrosswordsController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import org.joda.time.{DateTime, LocalDate}
import pages.{CrosswordHtmlPage, IndexHtmlPage, PrintableCrosswordHtmlPage}
import play.api.data.Forms._
import play.api.data._
import play.api.libs.json.JsValue
import play.api.libs.ws.WSClient
import play.api.mvc._
import renderers.DotcomRenderingService
Expand Down Expand Up @@ -124,7 +125,7 @@ class CrosswordPageController(
}
private def getDCRJson(crosswordPage: CrosswordPageWithContent, pageType: PageType)(implicit
request: RequestHeader,
): String =
): JsValue =
DotcomRenderingDataModel.toJson(
DotcomRenderingDataModel.forCrossword(crosswordPage, request, pageType),
)
Expand Down
3 changes: 2 additions & 1 deletion applications/app/controllers/GalleryController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import contentapi.ContentApiClient
import model._
import model.dotcomrendering.{DotcomRenderingDataModel, PageType}
import pages.GalleryHtmlPage
import play.api.libs.json.JsValue
import play.api.libs.ws.WSClient
import play.api.mvc._
import play.twirl.api.Html
Expand Down Expand Up @@ -74,7 +75,7 @@ class GalleryController(

private def getDCRJson(galleryPage: GalleryPage, pageType: PageType, blocks: Blocks)(implicit
request: RequestHeader,
): String = {
): JsValue = {
DotcomRenderingDataModel.toJson(DotcomRenderingDataModel.forGallery(galleryPage, request, pageType, blocks))
}

Expand Down
2 changes: 1 addition & 1 deletion applications/app/controllers/ImageContentController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class ImageContentController(

private def getDCRJson(content: ImageContentPage, pageType: PageType, mainBlock: Option[Block])(implicit
request: RequestHeader,
): String = {
): JsValue = {
DotcomRenderingDataModel.toJson(DotcomRenderingDataModel.forImageContent(content, request, pageType, mainBlock))
}

Expand Down
4 changes: 2 additions & 2 deletions applications/app/controllers/MediaController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import implicits.{AppsFormat, JsonFormat}
import model._
import model.dotcomrendering.{DotcomRenderingDataModel, PageType}
import pages.ContentHtmlPage
import play.api.libs.json.{Format, JsObject, Json}
import play.api.libs.json.{Format, JsObject, Json, JsValue}
import play.api.libs.ws.WSClient
import play.api.mvc._
import renderers.DotcomRenderingService
Expand Down Expand Up @@ -96,7 +96,7 @@ class MediaController(

private def getDCRJson(content: MediaPage, pageType: PageType, blocks: Blocks)(implicit
request: RequestHeader,
): String = {
): JsValue = {
DotcomRenderingDataModel.toJson(DotcomRenderingDataModel.forMedia(content, request, pageType, blocks))
}

Expand Down
4 changes: 2 additions & 2 deletions article/app/controllers/ArticleController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import model.Cached.{RevalidatableResult, WithoutRevalidationResult}
import model.dotcomrendering.{DotcomRenderingDataModel, PageType}
import model._
import pages.{ArticleEmailHtmlPage, ArticleHtmlPage}
import play.api.libs.json.Json
import play.api.libs.json.{Json, JsValue}
import play.api.libs.ws.WSClient
import play.api.mvc._
import renderers.DotcomRenderingService
Expand Down Expand Up @@ -91,7 +91,7 @@ class ArticleController(

/** Returns a JSON representation of the payload that's sent to DCR when rendering the Article.
*/
private def getDCRJson(article: ArticlePage, blocks: Blocks)(implicit request: RequestHeader): String = {
private def getDCRJson(article: ArticlePage, blocks: Blocks)(implicit request: RequestHeader): JsValue = {
val pageType: PageType = PageType(article, request, context)
val newsletter = newsletterService.getNewsletterForArticle(article)

Expand Down
7 changes: 5 additions & 2 deletions common/app/common/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import model.CacheTime.RecentlyUpdated
import model.Cached.RevalidatableResult
import model.{ApplicationContext, Cached, NoCache}
import play.api.Logger
import play.api.libs.json.{JsObject, JsString}
import play.api.libs.json.{JsObject, JsString, JsValue}
import play.api.mvc.{RequestHeader, Result}
import play.twirl.api.Html
import model.ApplicationContext
Expand Down Expand Up @@ -162,7 +162,10 @@ object `package`
JsonComponent(page, json)
}

def renderJson(json: String, page: model.Page)(implicit request: RequestHeader, context: ApplicationContext): Result =
def renderJson(json: JsValue, page: model.Page)(implicit
request: RequestHeader,
context: ApplicationContext,
): Result =
Cached(page) {
RevalidatableResult.Ok(json)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ object DotcomBlocksRenderingDataModel {
}
}

def toJson(model: DotcomBlocksRenderingDataModel): String = {
def toJson(model: DotcomBlocksRenderingDataModel): JsValue = {
val jsValue = Json.toJson(model)
Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
DotcomRenderingUtils.withoutNull(jsValue)
}

def apply(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ object DotcomFrontsRenderingDataModel {
)
}

def toJson(model: DotcomFrontsRenderingDataModel): String = {
def toJson(model: DotcomFrontsRenderingDataModel): JsValue = {
val jsValue = Json.toJson(model)
Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
DotcomRenderingUtils.withoutNull(jsValue)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ object DotcomNewslettersPageRenderingDataModel {
)
}

def toJson(model: DotcomNewslettersPageRenderingDataModel): String = {
def toJson(model: DotcomNewslettersPageRenderingDataModel): JsValue = {
val jsValue = Json.toJson(model)
Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
DotcomRenderingUtils.withoutNull(jsValue)
}

private def convertNewsletterResponseToData(response: NewsletterResponseV2): NewsletterData = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,9 @@ object DotcomRenderingDataModel {
}
}

def toJson(model: DotcomRenderingDataModel): String = {
def toJson(model: DotcomRenderingDataModel): JsValue = {
val jsValue = Json.toJson(model)
Json.stringify(withoutNull(jsValue))
withoutNull(jsValue)
}

def forInteractive(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ object DotcomTagPagesRenderingDataModel {
)
}

def toJson(model: DotcomTagPagesRenderingDataModel): String = {
def toJson(model: DotcomTagPagesRenderingDataModel): JsValue = {
val jsValue = Json.toJson(model)
Json.stringify(DotcomRenderingUtils.withoutNull(jsValue))
DotcomRenderingUtils.withoutNull(jsValue)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package model.dotcomrendering.pageElements

import com.gu.contentapi.client.model.v1.Crossword
import com.gu.contentapi.json.CirceEncoders._
import io.circe.JsonObject
import io.circe.syntax._
import implicits.Dates.CapiRichDateTime
import model.dotcomrendering.DotcomRenderingUtils
import play.api.libs.json.{JsObject, Json, JsValue}

case class EditionsCrosswordRenderingDataModel(
crosswords: Iterable[Crossword],
Expand All @@ -25,9 +26,10 @@ object EditionsCrosswordRenderingDataModel {
}
}))

def toJson(model: EditionsCrosswordRenderingDataModel): String = {
JsonObject(
"crosswords" -> model.crosswords.asJson.deepDropNullValues,
).asJson.noSpaces
}
def toJson(model: EditionsCrosswordRenderingDataModel): JsValue =
DotcomRenderingUtils.withoutNull(
Json.obj(
"crosswords" -> Json.parse(model.crosswords.asJson.toString()),
),
)
}
5 changes: 3 additions & 2 deletions common/app/renderers/DotcomRenderingService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import model.{
RelatedContentItem,
SimplePage,
}
import play.api.libs.json.JsValue
import play.api.libs.ws.{WSClient, WSResponse}
import play.api.mvc.Results.{InternalServerError, NotFound}
import play.api.mvc.{RequestHeader, Result}
Expand Down Expand Up @@ -56,7 +57,7 @@ class DotcomRenderingService extends GuLogging with ResultWithPreconnectPreload

private[this] def postWithoutHandler(
ws: WSClient,
payload: String,
payload: JsValue,
endpoint: String,
timeout: Duration = Configuration.rendering.timeout,
)(implicit request: RequestHeader): Future[WSResponse] = {
Expand Down Expand Up @@ -93,7 +94,7 @@ class DotcomRenderingService extends GuLogging with ResultWithPreconnectPreload

private[this] def post(
ws: WSClient,
payload: String,
payload: JsValue,
endpoint: String,
cacheTime: CacheTime,
timeout: Duration = Configuration.rendering.timeout,
Expand Down
3 changes: 2 additions & 1 deletion common/test/renderers/DotcomRenderingServiceTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import org.scalatest.{BeforeAndAfterAll, DoNotDiscover, PrivateMethodTester}
import org.scalatest.matchers.should.Matchers
import test.{ConfiguredTestSuite, TestRequest, WithMaterializer, WithTestWsClient}
import play.api.libs.ws.{WSClient, WSRequest, WSResponse}
import play.api.libs.json.{JsString}
import play.api.mvc.{Result}
import conf.Configuration
import org.mockito.Matchers.any
Expand Down Expand Up @@ -35,7 +36,7 @@ import org.scalatest.flatspec.AnyFlatSpec
private val wsRequestMock = mock[WSRequest]

"post" should "return a 404 for DCR 415 errors" in {
val payload = "payload"
val payload = JsString("payload")
when(wsMock.url(any[String])).thenReturn(wsRequestMock)
when(wsRequestMock.withRequestTimeout(any())).thenReturn(wsRequestMock)
when(wsRequestMock.addHttpHeaders(any())).thenReturn(wsRequestMock)
Expand Down

0 comments on commit 9c8a254

Please sign in to comment.