Use JsValue Not String For DCAR

[JamieB-gu]

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.

Paired with @rtyley

Footnotes

1. https://www.playframework.com/documentation/3.0.x/api/scala/play/api/libs/ws/WSBodyWritables$.html#writeableOf_String:play.api.libs.ws.BodyWritable[String] ↩

2. https://www.playframework.com/documentation/3.0.x/api/scala/play/api/libs/ws/WSBodyWritables$.html#writeableOf_JsValue:play.api.libs.ws.BodyWritable[play.api.libs.json.JsValue] ↩

3. https://www.playframework.com/documentation/3.0.x/api/scala/play/api/http/Writeable$.html#writeableOf_JsValue:play.api.http.Writeable[play.api.libs.json.JsValue] ↩

[rtyley]

Lovely stuff! I see the tests have failed, but from a quick look at the logs, maybe the only failing test was DotcomRenderingServiceTest:

[info] DotcomRenderingServiceTest:
[info] post
[info] - should return a 404 for DCR 415 errors *** FAILED ***
[info]   java.lang.IllegalArgumentException: Can't find a private method named: post
[info]   at org.scalatest.PrivateMethodTester$Invoker.invokePrivate(PrivateMethodTester.scala:247)
[info]   at renderers.DotcomRenderingServiceTest.$anonfun$new$1(DotcomRenderingServiceTest.scala:52)

...this is because the test is mocking the post() method, which has been changed to take a JsValue payload, rather that a String:

frontend/common/test/renderers/DotcomRenderingServiceTest.scala

Lines 38 to 55 in 55da434

	val payload = "payload"
	when(wsMock.url(any[String])).thenReturn(wsRequestMock)
	when(wsRequestMock.withRequestTimeout(any())).thenReturn(wsRequestMock)
	when(wsRequestMock.addHttpHeaders(any())).thenReturn(wsRequestMock)
	when(wsRequestMock.post(payload)).thenReturn(Future.successful(wsResponseMock))
	when(wsResponseMock.status).thenReturn(415)
	whenReady(
	dotcomRenderingService invokePrivate post(
	wsMock,
	payload,
	"https://endpoint.com",
	CacheTime.Default,
	Configuration.rendering.timeout,
	request,
	),
	) { result =>
	result.header.status should be(404)

Incidentally, it was quite hard to search the logs for the failure. Using something like https://github.com/test-summary/action or https://github.com/EnricoMi/publish-unit-test-result-action could make finding failing tests on Frontend much friendlier?

[github-actions]

Deploy build 3445 of dotcom:frontend-all to CODE

All deployment options

• Deploy build 3445 of dotcom:frontend-all to CODE
• Deploy parts of build 3445 to CODE by previewing it first
• What's on CODE right now?

---

From guardian/actions-riff-raff.

[SiAdcock]

Thanks @JamieB-gu and @rtyley this sounds good to me. I'm not fully clued up on the full impact of this change, and it's possible future travellers will want to know, so I was hoping you could answer for the record:

1. Are we likely to observe any benefits as a result of this change, other than the apparent reduction in complexity?
2. Do you know why we went down the JsValue -> String -> BodyWritable path in the first place?
3. Are we making any trade-offs by doing this?

Thanks in advance 🙏

[JamieB-gu]

Thanks @JamieB-gu and @rtyley this sounds good to me. I'm not fully clued up on the full impact of this change, and it's possible future travellers will want to know, so I was hoping you could answer for the record:

1. Are we likely to observe any benefits as a result of this change, other than the apparent reduction in complexity?
2. Do you know why we went down the JsValue -> String -> BodyWritable path in the first place?
3. Are we making any trade-offs by doing this?

Thanks in advance 🙏

Thanks @SiAdcock! To answer the questions:

1. Yes we hope so, we believe it may result in a performance improvement for two reasons. The first is that we're no longer doing this extra transformation step, so it may result in lower CPU usage. The other is that we're no longer creating extra Strings in memory, which will hopefully reduce memory usage and garbage collection.
2. Deferring to @rtyley below.
3. Deferring to @rtyley below.

[rtyley]

2. Do you know why we went down the JsValue -> String -> BodyWritable path in the first place?

The Playframework hides away the existence BodyWritable, using an implicit transformation to transform the object (whether a JsValue or aString) into bytes that can be transmitted over the HTTP connection. So most developers wouldn't have been aware of the BodyWritable part! So the question becomes: why did JsValue -> String occur? I think that's probably not an area that many developers were aware that a performance optimisation could occur - the code originally occurred in #20528 in October 2018, when the method was originally called, accurately, toJsonString. It was probably copied and pasted a lot since then!

3. Are we making any trade-offs by doing this?

Can't think of any negatives - the code is more strongly-typed, and the copying operations look to be more efficient because they're not doing unnecessary array-copying!

[prout-bot]

Seen on ADMIN-PROD (merged by @JamieB-gu 12 minutes and 4 seconds ago)

• Check your changes on www.theguardian.com ✔️
• Keep an eye on the deploy dashboard 📉

[prout-bot]

Seen on FRONTS-PROD (merged by @JamieB-gu 13 minutes and 9 seconds ago)

• Check your changes on www.theguardian.com ✔️
• Keep an eye on the deploy dashboard 📉

[rtyley]

I've taken a quick look at the graphs after this got merged at 1:07pm - I was wondering if garbage collection might have noticeably improved due to these changes, and unfortunately there's been no obvious overall change, at least not yet. Still, I think the change is still justified in terms of code clarity!