Pact Merging Creates Duplicate Interactions

[adamrodger]

FFI Version: 0.4.16
Specification Version: 4

I have a test in PactNet which is very simple, like this:

this.pact
    .UponReceiving("a request for an order with an unknown ID")
        .WithRequest(HttpMethod.Get, "/api/orders/404")
        .WithHeader("Accept", "application/json")
    .WillRespond()
        .WithStatus(HttpStatusCode.NotFound);

When I run the test, the mock server ends up being invoked with overwrite:false via the pactffi_pact_handle_write_file method.

Expected: The first time the test is executed then the new file will be written then every subsequent execution should just end up producing the same interaction definition (and thus merging it into the file effectively as a no-op).

Actual: Every time I run the test the same interaction is written as an exact duplicate, so my Pact file contains exactly the same thing three times in a row:

{
  "description": "a request for an order with an unknown ID",
  "pending": false,
  "request": {
    "headers": {
      "Accept": [
        "application/json"
      ]
    },
    "method": "GET",
    "path": "/api/orders/404"
  },
  "response": {
    "status": 404
  },
  "type": "Synchronous/HTTP"
},
{
  "description": "a request for an order with an unknown ID",
  "pending": false,
  "request": {
    "headers": {
      "Accept": [
        "application/json"
      ]
    },
    "method": "GET",
    "path": "/api/orders/404"
  },
  "response": {
    "status": 404
  },
  "type": "Synchronous/HTTP"
},
{
  "description": "a request for an order with an unknown ID",
  "pending": false,
  "request": {
    "headers": {
      "Accept": [
        "application/json"
      ]
    },
    "method": "GET",
    "path": "/api/orders/404"
  },
  "response": {
    "status": 404
  },

[adamrodger]

I think the problem must be something like here where it tries to match interactions based on their key:

https://github.com/pact-foundation/pact-reference/blob/master/rust/pact_models/src/v4/pact.rs#L365

My interactions don't explicitly have keys (I've no way of setting them via FFI), but I can only assume those keys are being auto-generated internally and then don't match.

[adamrodger]

I've recreated the problem at least as far back as FFI 0.4.0. I'll keep going back as far as I can until I can't reproduce

[adamrodger]

I've also recreated in FFI 0.3.13, but the difference there is that the Pact file outputs a unique key for each interaction (hence why they're not matching). I remember a change being made to prevent these keys being generated, so that sounds a likely candidate for the the problem.

In 0.3.13 the Pact file looks like:

    {
      "description": "a request for an order with an unknown ID",
      "key": "fcc034cf861cdb47", // NOTE: Auto-generated key
      "pending": false,
      "request": {
        "headers": {
          "Accept": [
            "application/json"
          ]
        },
        "method": "GET",
        "path": "/api/orders/404"
      },
      "response": {
        "status": 404
      },
      "type": "Synchronous/HTTP"
    },
    {
      "description": "a request for an order with an unknown ID",
      "key": "585430b9552bc4e0", // NOTE: Different key
      "pending": false,
      "request": {
        "headers": {
          "Accept": [
            "application/json"
          ]
        },
        "method": "GET",
        "path": "/api/orders/404"
      },
      "response": {
        "status": 404
      },
      "type": "Synchronous/HTTP"
    },

[adamrodger]

#264 was the issue I was thinking of where auto-generation of keys was removed. I'm not sure if there are still problems in here though, or whether it's the fallback logic that matches interactions without keys that's problematic. Either way it looks like it should be matching those interactions but it doesn't.

[adamrodger]

Yeah I can't see the problem just from looking at the code and the Pact output.

The key field is missing from the messages so those both must be None, which means it falls back to other matching logic. That logic checks the equality of the type field, which are both Synchronous/HTTP, then checks the equality of the provider states, which are both empty, then checks the equality of the descriptions, which are both a request for an order with an unknown ID.

I don't get how those wouldn't be considered equal just going off the code, so either this bit of code isn't the problem or those fields don't have the values I think they do. That would probably need some inspection in the debugger to work out.

[github-actions]

🤖 Great news! We've labeled this issue as smartbear-supported and created a tracking ticket in PactFlow's Jira (PACT-1835). We'll keep work public and post updates here. Meanwhile, feel free to check out our docs. Thanks for your patience!

[rholshausen]

pactffi_pact_handle_write_file is not the problem, it merges the Pact correctly.

[adamrodger]

The attached commits suggest that the interactions need to have a unique description, but the merging behaviour compares the description, provider states and interaction type as the unique identifier of an interaction instead of just the description. Is this a new restriction?

Also, to be clear I was writing out the pact file each time instead of having multiple interactions with the same description within the same FFI session. I just ran the same single test a few times and each test run added another instance of the same interaction to the file, but they were entirely independent runs.

[rholshausen]

While it is true that the merging behavior compares the description and provider states as the unique identifier, the provider states are not set yet so can't be used.

[adamrodger]

Do we know if this was fixed? I'm getting a lot of requests to release PactNet 5.0 fully and this is a blocking issue for the full release.

[YOU54F]

I don’t believe so, i think the general advice from my time using the pact project has been to delete pacts before executing your tests to avoid stale interactions.

merging might mean you update your test and the interaction contains some data from a previous test.

I think this issue is because of not deleting the pact before running his pact tests. If we can detect when we first run, it would probably be good to offer an ability to auto-delete any pacts in the pact folder, rather than having to tell people to do it anyway.

It was best practise Tim and I wanted!

pact-foundation/jest-pact#102

I wonder if its a client lib concern rather than worrying at the ffi barrier, but it might be that it is something we can do in the core.

I just worry that we will miss the chance if we want it to be highly parallelised because we don't want to delete pacts that have been created mid run.

i think some kind of warning about existing pact data would be good. i noted in the e2e example in pact net multiple invocations without deleting end up with the same interaction duplicated, which doesn’t seem right.

so in lieu of having provider states set if the unique key of desc and states maybe we just need to consider desc if no states set.

ill get this on my radar to look into for you asap. thanks for the investigation, i think this might date back to inception with the rust core, i’m certainly happy to check that and find a nice path forward for all parties :)

[YOU54F]

I've taken a look at this today and think I have a fix in place that is able to perform multiple runs of pact-net locally, without creating duplicate interactions in the pact file that is committed to the repo. Just testing in CI e2e, and need to create a repro locally in pact-ref.

I think the issue is two-fold

1. we can't consider provider states in your example, as they may not exist (and are optional) so should be considered as such, here
2. the match on type doesn't seem to work, where there are multiple different interaction types in a pact file. here

changing the ordering to check

1. matching description
2. matching provider states, if pact to be merged has provider states
3. match on pact interaction type

YOU54F#7

I'll look to clean it up tomorrow and get it in for review

There is a seperate issue blocking a clean passing run in pact-net with the latest version of the pact_ffi

• libpact_ffi 0.4.20 pact_ffi_mock_server_logs are empty, when testing in pact-net
  • failing run with 0.4.20
  • passing run with 0.4.19
  • passing run with 0.4.20
    • by changing expection to empty logs

failing step (link)

[xUnit.net 00:00:01.54]     PactNet.Tests.Drivers.FfiIntegrationTests.HttpInteraction_v3_CreatesPactFile [FAIL]
[xUnit.net 00:00:01.54]       Did not expect logs to be empty.
[xUnit.net 00:00:01.54]       Stack Trace:
[xUnit.net 00:00:01.54]            at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
[xUnit.net 00:00:01.54]            at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
[xUnit.net 00:00:01.54]            at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message)
[xUnit.net 00:00:01.54]            at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
      Request starting HTTP/1.1 POST http://localhost:5000/provider-states - application/json 75
[xUnit.net 00:00:01.55]            at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
[xUnit.net 00:00:01.55]            at FluentAssertions.Execution.AssertionScope.FailWith(String message)
dbug: Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware[0]
      Wildcard detected, all requests with hosts will be allowed.
[xUnit.net 00:00:01.55]            at FluentAssertions.Primitives.StringAssertions`1.NotBeEmpty(String because, Object[] becauseArgs)
[xUnit.net 00:00:01.55]         /home/runner/work/pact-net/pact-net/tests/PactNet.Tests/Drivers/FfiIntegrationTests.cs(73,0): at PactNet.Tests.Drivers.FfiIntegrationTests.HttpInteraction_v3_CreatesPactFile()

Cause?

Unknown at the moment, but this is the delta between 0.4.19 & 0.4.20

libpact_ffi-v0.4.19...libpact_ffi-v0.4.20