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

Inconsistent pact manifest generation when using gRPC responseMetadata #6

Open
ysatarov opened this issue Nov 21, 2024 · 10 comments
Open

Comments

@ysatarov
Copy link

ysatarov commented Nov 21, 2024

Hi!

We encountered a strange problem when using with gRPC and responseMetadata. The contents of the pact manifest in these cases depends on the order in which the tests are run.

screenshot

Below is a slightly modified example of area_calculator test for reproduction:
78a2a2a

There are two identical tests added, one of them at the beginning and the other at the end of spec-file. The response format in the final pact manifest differs for these two added specs when running specs in random order, which leads to an error publishing contract to pact broker with pact-cli:

Cannot change the content of the pact for SOME-CONSUMER version 2821c525 and provider SOME_PROVIDER, as race conditions will cause unreliable results for can-i-deploy. Each pact must be published with a unique consumer version number. Some Pact libraries generate random data when a concrete value for a type matcher is not specified, and this can cause the contract to mutate - ensure you have given example values for all type matchers. For more information see https://docs.pact.io/go/versioning

Tried protobuf-plugin v0.4 / v0.5.4, pact-ffi v0.4.22 (linux-x86_64 and macos-aarch64), ruby v3.1.2 / v3.3.6

@YOU54F
Copy link
Owner

YOU54F commented Nov 21, 2024

Hey @ysatarov.

First off nice work to you and the Kuper-Tech team for your work on sbmt-pact. I've been meaning to catch up for a while.

I took it for a spin recently and started testing it out against some of our existing ruby projects which use pact.

Needed a few tweaks as we don't use rails.

My delta is here. Kuper-Tech/sbmt-pact@master...YOU54F:sbmt-pact:feat/pact-ruby

Anyway back to your question in point.

The order of interactions should not matter. If you do re-order them to the same as a previous publish, does that rectify the publishing issue?

Is that the only delta between runs (interaction order)?

If so, that is determined by the pact-broker on publish.

Here is where the check starts in the pact_publish api resource

https://github.com/pact-foundation/pact_broker/blob/96532124f3a53a499276c69ff2df785b8377588e/lib/pact_broker/api/resources/publish_contracts.rb#L25

which calls into the contract_service

https://github.com/pact-foundation/pact_broker/blob/96532124f3a53a499276c69ff2df785b8377588e/lib/pact_broker/contracts/service.rb#L49-L64

the calculated pact_version will be important, as it will be used to find existing pacts, by that version number.

https://github.com/pact-foundation/pact_broker/blob/96532124f3a53a499276c69ff2df785b8377588e/lib/pact_broker/contracts/service.rb#L131-L142

The Pact Broker isn't aware of the V4 Pact specification, so it may not be able to determine that these pact files are the same.

Maybe that is enough of a cookie trail to take a peek at the broker and cast your Rubyist eyes across it

@ysatarov
Copy link
Author

My delta is here

Thanks! It may take a little bit of time, but we'll definitely look into your changes. Feel free to send a pull request if you'd like

Is that the only delta between runs (interaction order)?

Well, not exactly. Maybe it was the screenshot that added some confusion )
The point is the same test example using responseMetadata gives different serialization of response.contents in generated pact manifest.

If it runs first, response.contents would be

...
      "response": [
        {
          "contents": {
            "content": ""
          },
         ...
        }
      ],
...

And if we move the same test example into the end of the spec file and run, response.contents will contain 3 more fields:

...
      "response": [
        {
          "contents": {
            "content": "",
            "contentType": "application/protobuf;message=AreaResponse",
            "contentTypeHint": "BINARY",
            "encoded": "base64"
          },
         ...
        }
      ],
...

screenshot1

@YOU54F
Copy link
Owner

YOU54F commented Nov 21, 2024

hmm curious. If the middle test is not there, does the same occur? I wonder if the interactions are not fully cleared in between tests.

@ysatarov
Copy link
Author

If the middle test is not there, does the same occur?

if I run only that one specific test, it generates:

...
      "response": [
        {
          "contents": {
            "content": "",
            "contentType": "application/protobuf;message=AreaResponse",
            "contentTypeHint": "BINARY",
            "encoded": "base64"
          },
         ...
        }
      ],
...

@YOU54F
Copy link
Owner

YOU54F commented Nov 26, 2024

Ok, so we have for a most minimal reproduction

  • 2 exact tests, executed in the same run
    • same named consumer/provider pair
    • only differ by description
    • each test creates a new pact handle
  1. Moving the new_pact initializer out of the each indiv tests block so each test share a pact handle does not make a difference.
  2. Swapping the order of the tests, always results in the first interaction losing some fields, transforming the first interaction from
"response": [
  {
    "contents": {
      "content": ""
    },
    "metadata": {
      "contentType": "application/protobuf;message=AreaResponse",
      "grpc-message": "Not implemented",
      "grpc-status": "UNIMPLEMENTED"
    }
  }
],

to

      "response": [
        {
          "contents": {
            "content": "",
            "contentType": "application/protobuf;message=.area_calculator.AreaResponse",
            "contentTypeHint": "BINARY",
            "encoded": "base64"
          },
          "metadata": {
            "contentType": "application/protobuf;message=.area_calculator.AreaResponse",
            "grpc-message": "Not implemented",
            "grpc-status": "UNIMPLEMENTED"
          }
        }
      ],

a diff of

53,56c53
<             "content": "",
<             "contentType": "application/protobuf;message=.area_calculator.AreaResponse",
<             "contentTypeHint": "BINARY",
<             "encoded": "base64"
---
>             "content": ""
  1. If we name each provider differently, each pact is saved with the correct fully populated response[0].contents object
  2. If we run 1 test, with a pre-existing pact containing the other interaction, the other interaction will be modified.

This leads me to believe the issue is during the pact merge process in pact-reference, the rust core.

@YOU54F
Copy link
Owner

YOU54F commented Nov 26, 2024

I'll make a repro in pact-ref and make a bug report over there (or transfer this issue)

@YOU54F
Copy link
Owner

YOU54F commented Nov 26, 2024

Added a failing test in pact_reference project, via the pact_ffi interface which we use as consumers in the ruby wrapper lib (and others)

https://github.com/pact-foundation/pact-reference/compare/master...YOU54F:pact-reference:issue/pact-ruby-ffi-6?expand=1

Will transfer across now - edit: can't transfer as it is my personal account rather than under the pact-foundation org.

Will create a new issue

@YOU54F
Copy link
Owner

YOU54F commented Nov 26, 2024

I've raised that now against the core, linked issue ☝🏾

As a temporary workaround, you could

  1. name the providers differently in order to generate multiple pact files
  2. merge all the interactions into a single pact file
  3. update the provider name in the single pact file, to that of your actual provider

FYI - We have a maintainer meet every 2 weeks, the next one is tomorrow, some details in the below link.

Feel free to drop by if you'd like at any point and extend the invite to the Kuper-Tech Ruby Platform team :)

https://github.com/pact-foundation/roadmap?tab=readme-ov-file#-maintainer-sessions

@rholshausen
Copy link
Contributor

This is fixed in the plugin driver commit pact-foundation/pact-plugins@e06d8b3#diff-89470b102b7a795f90ba67ecca198b91793400c51b7ec35630063fda014498efR587

@YOU54F
Copy link
Owner

YOU54F commented Nov 29, 2024

great stuff, thanks Ron.

I’ll leave this ticket open until the ffi is updated with the changes, released and consumed in pact-ruby-ffi

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

No branches or pull requests

3 participants