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

Make the ContentApiQuery type convariant in Response #429

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

fredex42
Copy link
Contributor

@fredex42 fredex42 commented Jun 28, 2024

What does this change?

Changes the Response type parameter from invarant to covariant.

This is to make pattern-matching in tests to work better - used in integration tests introduced with https://github.com/guardian/crier/pull/169.

In a nutshell, we needed to be able to use Mockito to verify two different calls to the library. If the calls changed order, the behaviour would still be correct (ordering cannot be guaranteed in this case) but the test would then fail if it tried to cast the SearchQuery to the ItemQuery or vice-versa.

With this update in place, we can make a correctly scoped match statement to handle both of these cases gracefully:

      verify(mockLiveContentApi, times(2)).getResponse(argThat((arg:ContentApiQuery[ThriftStruct])=> arg match {
        case searchQuery: SearchQuery=>
          searchQuery.channelId.contains("all") &&
            searchQuery.parameters.get("ids").contains(s"internal-code/composer/${notifications.head.value.composerId}") &&
            searchQuery.parameters.get("show-channels").contains("all")
        case itemQuery: ItemQuery=>
          itemQuery.channelId.contains("all") &&
            itemQuery.id=="lifeandstyle/article/2024/jun/27/bzbcxvzxvzxv"
        case other@_ =>
          fail(s"Unexpected CAPI call: ${other.getClass.getTypeName}")
      }))(any, any)

Without this update, the compiler objects since the type signature of ContentApiQuery[Response] requires specific response type and not an ancestor of this type. When changed to ContentApiQuery[+Response] we are able to successfully match against any possible value.

How to test

Integration tests linked above work.

Tested locally.

How can we measure success?

Able to continue on Crier

Have we considered potential risks?

n/a

@fredex42 fredex42 requested a review from a team as a code owner June 28, 2024 16:29
Copy link

Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 4f29429. ± Comparison against base commit c406c9e.

@gu-scala-library-release
Copy link
Contributor

@fredex42 has published a preview version of this PR with release workflow run #64, based on commit 4f29429:

31.0.3-PREVIEW.agcovariant-query-response.2024-06-28T1631.4f294296

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the ag/covariant-query-response branch, or use the GitHub CLI command:

gh workflow run release.yml --ref ag/covariant-query-response

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

Copy link
Member

@JustinPinner JustinPinner left a comment

Choose a reason for hiding this comment

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

I think this should be pretty harmless 👍

@fredex42 fredex42 merged commit 2fc15c3 into main Jul 2, 2024
9 checks passed
@fredex42 fredex42 deleted the ag/covariant-query-response branch July 2, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants