Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CAPI & FAPI clients to latest versions (also remove hack required for Lightbox) #25139

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

ioannakok
Copy link
Contributor

@ioannakok ioannakok commented Jun 22, 2022

What does this change?

The latest version of the CAPI client (v19) includes guardian/content-api-scala-client#359, which we can use to replace the pagination code based on play-iteratees (introduced in July 2014 with #5199), currently blocking the Scala 2.13 upgrade for Frontend.

The updated CAPI client is binary-incompatible with previous versions of the CAPI client, meaning we also need to update the FAPI client - as it also uses CAPI, and needs to be compiled against the same CAPI version. A similar PR, updating FAPI & CAPI in Ophan is at https://github.com/guardian/ophan/pull/4719, and has been successfully deployed. Like there, we're adding a FaciaClientTest that will break at build time rather than runtime if our versions of the FAPI & CAPI clients are incompatible.

The updates to the CAPI client also support previous/next search functionality, as used by the Lightbox, meaning that the small ContentApiNavQuery hack for the Lightbox (introduced here: #20462) can now be removed.

Screenshots

The next/previous functionality in lightboxes (eg on this article) from #20462 continues to work:

Screen.Recording.2022-06-30.at.11.48.37.mov

Checklist

Does this change update the version of CAPI we're using?

Tested

  • Locally
  • On CODE (optional)

@ioannakok
Copy link
Contributor Author

Deployment to CODE succeeded almost everywhere except for facia-press. We should release a beta version of facia-client: https://logs.gutools.co.uk/s/dotcom/goto/02ce2d00-f24e-11ec-9ed0-71515ea686c5

@rtyley
Copy link
Member

rtyley commented Jun 22, 2022

I've rolled back the Frontend CODE environment to main - the deploy went fine.

On our branch, without updating facia-scala-client (ie without using a facia-scala-client release incorporating guardian/facia-scala-client#271), we see this java.lang.NoSuchMethodError exception on facia-press for com.gu.contentapi.client.Decoder$.searchQueryBase():

java.lang.NoSuchMethodError: 'com.gu.contentapi.client.Decoder com.gu.contentapi.client.Decoder$.searchQueryBase()'
	at com.gu.facia.api.contentapi.ContentApi$.$anonfun$getHydrateResponse$1(ContentApi.scala:37)
	at scala.concurrent.Future$.$anonfun$traverse$1(Future.scala:850)
	at scala.collection.LinearSeqOptimized.foldLeft(LinearSeqOptimized.scala:126)
	at scala.collection.LinearSeqOptimized.foldLeft$(LinearSeqOptimized.scala:122)
	at scala.collection.immutable.List.foldLeft(List.scala:91)
	at scala.concurrent.Future$.traverse(Future.scala:850)
	at com.gu.facia.api.contentapi.ContentApi$.getHydrateResponse(ContentApi.scala:37)
	at com.gu.facia.api.FAPI$.$anonfun$getLiveContentForCollection$1(FAPI.scala:75)
	at com.gu.facia.api.Response.$anonfun$flatMap$1(Response.scala:13)
	at scala.concurrent.Future.$anonfun$flatMap$1(Future.scala:307)
	at scala.concurrent.impl.Promise.$anonfun$transformWith$1(Promise.scala:41)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
	at akka.dispatch.BatchingExecutor$AbstractBatch.processBatch(BatchingExecutor.scala:63)
	at akka.dispatch.BatchingExecutor$BlockableBatch.$anonfun$run$1(BatchingExecutor.scala:100)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
	at scala.concurrent.BlockContext$.withBlockContext(BlockContext.scala:85)
	at akka.dispatch.BatchingExecutor$BlockableBatch.run(BatchingExecutor.scala:100)
	at akka.dispatch.TaskInvocation.run(AbstractDispatcher.scala:49)
	at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(ForkJoinExecutorConfigurator.scala:48)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)

This is the code in facia-scala-client:

Future.traverse(searchQueries)(client.getResponse(_))

@rtyley rtyley changed the title Bump capi-client to 18.0.4-beta.0 - remove hack required for Lightbox Bump capi-client to 18.0.4-beta.1 - remove hack required for Lightbox Jun 22, 2022
rtyley added a commit that referenced this pull request Jun 29, 2022
We're adding a test that will break at build time rather than runtime if
our versions of the FAPI & CAPI clients are incompatible.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
rtyley added a commit that referenced this pull request Jun 30, 2022
We're adding a test that will break at build time rather than runtime if
our versions of the FAPI & CAPI clients are incompatible.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
rtyley added a commit that referenced this pull request Jun 30, 2022
We're adding a test that will break at build time rather than runtime if
our versions of the FAPI & CAPI clients are incompatible.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
rtyley added a commit that referenced this pull request Jun 30, 2022
We're adding a test that will break at build time rather than runtime if
our versions of the FAPI & CAPI clients are incompatible.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
@rtyley rtyley force-pushed the try-out-new-scala-client branch 2 times, most recently from 8c43ce2 to 71b241d Compare June 30, 2022 10:03
rtyley added a commit that referenced this pull request Jun 30, 2022
We're adding a test that will break at build time rather than runtime if
our versions of the FAPI & CAPI clients are incompatible.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
@rtyley
Copy link
Member

rtyley commented Jun 30, 2022

Deploying build 43804 (71b241d - which has the FAPI client updated to use the same CAPI client with guardian/facia-scala-client#271) to CODE has completed - happily, there are no further java.lang.NoSuchMethodError errors showing up in the logs:

image

The lightbox functionality from #20462 is also working:

Screen.Recording.2022-06-30.at.11.48.37.mov

@rtyley rtyley changed the title Bump capi-client to 18.0.4-beta.1 - remove hack required for Lightbox Update CAPI & FAPI clients to latest versions (also remove hack required for Lightbox) Jun 30, 2022
rtyley pushed a commit that referenced this pull request Jun 30, 2022
The latest version of the CAPI client (v19) includes
guardian/content-api-scala-client#359,
which we can use to replace the pagination code based on `play-iteratees`
(https://github.com/playframework/play-iteratees - introduced in July 2014
with #5199), currently blocking the
Scala 2.13 upgrade for Frontend (see #24817).

The updated CAPI client is binary-incompatible with previous versions of
the CAPI client, meaning we also need to update the FAPI client as it also
uses CAPI.

The updates to the CAPI client also support `previous`/`next` search
functionality, as used by the Lightbox, meaning that the small
`ContentApiNavQuery` hack for the Lightbox (introduced here:
#20462) can now be removed.

A similar PR updating FAPI & CAPI in Ophan is at
guardian/ophan#4719.

Co-authored-by: Roberto Tyley <roberto.tyley@guardian.co.uk>

We're adding a test that will break at build time rather than runtime if
our versions of the FAPI & CAPI clients are incompatible.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
The latest version of the CAPI client (v19) includes
guardian/content-api-scala-client#359,
which we can use to replace the pagination code based on `play-iteratees`
(https://github.com/playframework/play-iteratees - introduced in July 2014
with #5199), currently blocking the
Scala 2.13 upgrade for Frontend (see #24817).

The updated CAPI client is binary-incompatible with previous versions of
the CAPI client, meaning we also need to update the FAPI client as it also
uses CAPI. A similar PR updating FAPI & CAPI in Ophan is at
guardian/ophan#4719. Like there, we're adding a
test that will break at build time rather than runtime if our versions of
the FAPI & CAPI clients are incompatible.

The updates to the CAPI client also support `previous`/`next` search
functionality, as used by the Lightbox, meaning that the small
`ContentApiNavQuery` hack for the Lightbox (introduced here:
#20462) can now be removed.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529

Co-authored-by: Roberto Tyley <roberto.tyley@guardian.co.uk>
@ioannakok ioannakok marked this pull request as ready for review July 1, 2022 09:24
@ioannakok ioannakok requested a review from a team as a code owner July 1, 2022 09:24
Copy link
Contributor

@jfsoul jfsoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@ioannakok ioannakok merged commit 5da1c29 into main Jul 1, 2022
@ioannakok ioannakok deleted the try-out-new-scala-client branch July 1, 2022 14:14
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @ioannakok 18 minutes and 23 seconds ago)

@rtyley
Copy link
Member

rtyley commented Jul 1, 2022

Deployed on PROD, and the lightbox still works!

Screen.Recording.2022-07-01.at.16.39.39.mov

@rtyley rtyley mentioned this pull request Jul 27, 2022
2 tasks
ioannakok pushed a commit that referenced this pull request Aug 15, 2022
This test, introduced with #25139, is intended to fail
if incompatible versions of CAPI & FAPI are used. However, it also failed when we switched
to Scala 2.13 - there seems to be some difference in the way Future.traverse behaves between
the two versions of Scala - the `null` values returned by the Mockito mock didn't cause a
problem in Scala 2.12, but threw a `NullPointerException` in Scala 2.13 (in `Future.zipwith`).

Easiest fix is to use a stub, that returns non-null objects, rather than a mock. The requests
made by this stub ContentApiClient will never succeed, but we've verified that the calls
do indeed only fail with an exception when the versions of CAPI & FAPI are incompatible.
@ioannakok ioannakok mentioned this pull request Aug 26, 2022
17 tasks
@rtyley rtyley added the Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 Seen-on-PROD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants