pact-foundation / pact-reference

Reference implementations for the pact specifications
https://pact.io
MIT License
91 stars 46 forks source link

Key generation in Pact V4 interactions is unstable across multiple runs #264

Closed mefellows closed 1 year ago

mefellows commented 1 year ago

Issue originally reported here: https://github.com/pact-foundation/pact-go/issues/230

Repro here: https://github.com/pact-foundation/pact-reference/blob/repro/pact-go-issue-230/rust/pact_consumer/tests/tests.rs#L24

Run the test several times, you should observe the key for the only interaction change between 7a7816368c8f4344 and e659df3a695ccd82.

file rust/pact_consumer/target/pact_dir/Consumer-Alice Service.json:

{
  "consumer": {
    "name": "Consumer"
  },
  "interactions": [
    {
      "description": "a retrieve Mallory request",
      "key": "7a7816368c8f4344",
      "pending": false,
      "providerStates": [
        {
          "name": "there is some good mallory"
        }
      ],
      "request": {
        "headers": {
          "content-type": [
            "application/json"
          ]
        },
        "method": "GET",
        "path": "/mallory"
      },
      "response": {
        "body": {
          "content": "That is some good Mallory.",
          "contentType": "*/*",
          "encoded": false
        },
        "headers": {
          "Content-Type": [
            "text/plain"
          ]
        },
        "status": 200
      },
      "transport": "http",
      "type": "Synchronous/HTTP"
    }
  ],
  "metadata": {
    "pactRust": {
      "consumer": "0.10.4",
      "mockserver": "0.9.8",
      "models": "1.0.4"
    },
    "pactSpecification": {
      "version": "4.0"
    }
  },
  "provider": {
    "name": "Alice Service"
  }
}

and

{
  "consumer": {
    "name": "Consumer"
  },
  "interactions": [
    {
      "description": "a retrieve Mallory request",
      "key": "e659df3a695ccd82",
      "pending": false,
      "providerStates": [
        {
          "name": "there is some good mallory"
        }
      ],
      "request": {
        "headers": {
          "content-type": [
            "application/json"
          ]
        },
        "method": "GET",
        "path": "/mallory"
      },
      "response": {
        "body": {
          "content": "That is some good Mallory.",
          "contentType": "*/*",
          "encoded": false
        },
        "headers": {
          "Content-Type": [
            "text/plain"
          ]
        },
        "status": 200
      },
      "transport": "http",
      "type": "Synchronous/HTTP"
    }
  ],
  "metadata": {
    "pactRust": {
      "consumer": "0.10.4",
      "mockserver": "0.9.8",
      "models": "1.0.4"
    },
    "pactSpecification": {
      "version": "4.0"
    }
  },
  "provider": {
    "name": "Alice Service"
  }
}

As per the original ticket, I suspect it's something to do with the request part of the interaction.

github-actions[bot] commented 1 year ago

👋 Hi! The 'smartbear-supported' label has just been added to this issue, which will create an internal tracking ticket in PactFlow's Jira (PACT-926). We will use this to prioritise and assign a team member to this task. All activity will be public on this ticket. For now, sit tight and we'll update this ticket once we have more information on the next steps.

See our documentation for more information.

rholshausen commented 1 year ago

I was not able to reproduce this, the key is always 375d0fac416d80f2 when I run that test.

This may be an MacOS specific issue. I will test on Windows to see what the behavior is there.

rholshausen commented 1 year ago

Running on Windows, the key is the same as on Linux.

rholshausen commented 1 year ago

Ok, managed to replicate the issue, it's when the header is added to the request (which is a bit silly on a GET)

mefellows commented 1 year ago

Is it just the content-type header or any header (e.g. accept?)

In the Go example I had, it's a POST request (I'll test locally now). I could also reproduce the problem (in Go at least) with a request body also.

rholshausen commented 1 year ago

I haven't diagnosed the issue yet, but I'm willing to bet it is due to ordering of keys in the header hashmap.

mefellows commented 1 year ago

Ahh yeah, that makes sense. I thought I recalled seeing a procedure that ordered the headers before hashing, so perhaps there is a bug in that. That could also explain why the body does it, because it would implicitly set a content-type header too.

FYI here is a quick recording of me doing it on a POST.

https://user-images.githubusercontent.com/53900/231636671-090ff76e-1f40-4163-9b74-fe6f0de3ef24.mov

rholshausen commented 1 year ago

Oh, lookie here image

mefellows commented 1 year ago

That's interesting. I'm not seeing that change locally, only the key.

Are you suggesting that because the test has content-type rather than Content-Type one is taking precedence over the other?

rholshausen commented 1 year ago

I don't know why it is happening, but that diff shows the point at which the key is calculated, and it is showing how those two key values are being generated. I.e. the key generation is deterministic. My suspicion is when the mock server writes the Pact file, the merge is causing this.

I'm going to update the hash function to always generate the same key regardless of the case of the header keys, which will fix the issue. But there is still a question as to why it is happening in the first place.

mefellows commented 1 year ago

Ah, ok that makes some form of sense.

rholshausen commented 1 year ago

I've gone through all the hash functions on all the models, hopefully it should now not change if the JSON doesn't change. I've also updated it so things like different case HTTP methods will still generate the same key.

mefellows commented 1 year ago

Awesome, thanks. I've just pulled this locally, compiled and tested with go - looks like the issue is resolved.

I did notice a bunch of dbg! statements still hanging around though:

    id: None,
    key: None,
    description: "some interaction",
    provider_states: [],
    request: HttpRequest {
        method: "POST",
        path: "/products",
        query: None,
        headers: Some(
            {
                "content-type": [
                    "application/json",
                ],
            },
        ),
        body: Missing,
        matching_rules: MatchingRules {
            rules: {
                PATH: MatchingRuleCategory {
                    name: PATH,
                    rules: {},
                },
                HEADER: MatchingRuleCategory {
                    name: HEADER,
                    rules: {},
                },
            },
        },
        generators: Generators {
            categories: {},
        },
    },
    response: HttpResponse {
        status: 200,
        headers: None,
        body: Missing,
        matching_rules: MatchingRules {
            rules: {},
        },
        generators: Generators {
            categories: {},
        },
    },
    comments: {},
    pending: false,
    plugin_config: {},
    interaction_markup: InteractionMarkup {
        markup: "",
        markup_type: "",
    },
    transport: Some(
        "http",
    ),
}
rholshausen commented 1 year ago

I did notice a bunch of dbg! statements still hanging around though:

Gah! It burns us, it burns us!

adamrodger commented 1 year ago

@uglyog @mefellows This is still broken for me in FFI 0.4.4 on this pact file:

{
  "consumer": {
    "name": "PactExtensionsTests-Consumer-V4"
  },
  "interactions": [
    {
      "description": "a sample request",
      "key": "e037db1f569db957",
      "pending": false,
      "providerStates": [
        {
          "name": "a provider state"
        },
        {
          "name": "another provider state"
        },
        {
          "name": "a provider state with params",
          "params": {
            "baz": "bash",
            "foo": "bar"
          }
        }
      ],
      "request": {
        "body": {
          "content": {
            "bool": true,
            "children": [
              {
                "bool": false,
                "int": 7,
                "string": "bar"
              }
            ],
            "int": 42,
            "string": "foo"
          },
          "contentType": "application/json",
          "encoded": false
        },
        "headers": {
          "Content-Type": [
            "application/json"
          ],
          "X-Request": [
            "request1",
            "request2"
          ]
        },
        "matchingRules": {
          "body": {
            "$.bool": {
              "combine": "AND",
              "matchers": [
                {
                  "match": "type"
                }
              ]
            },
            "$.children": {
              "combine": "AND",
              "matchers": [
                {
                  "match": "type",
                  "min": 1
                }
              ]
            },
            "$.children[*].bool": {
              "combine": "AND",
              "matchers": [
                {
                  "match": "type"
                }
              ]
            },
            "$.children[*].int": {
              "combine": "AND",
              "matchers": [
                {
                  "match": "type"
                }
              ]
            },
            "$.children[*].string": {
              "combine": "AND",
              "matchers": [
                {
                  "match": "type"
                }
              ]
            },
            "$.int": {
              "combine": "AND",
              "matchers": [
                {
                  "match": "type"
                }
              ]
            },
            "$.string": {
              "combine": "AND",
              "matchers": [
                {
                  "match": "type"
                }
              ]
            }
          },
          "header": {},
          "query": {}
        },
        "method": "POST",
        "path": "/things",
        "query": {
          "param": [
            "value1",
            "value2"
          ]
        }
      },
      "response": {
        "body": {
          "content": {
            "bool": true,
            "children": [
              {
                "bool": false,
                "int": 7,
                "string": "bar"
              }
            ],
            "int": 42,
            "string": "foo"
          },
          "contentType": "application/json",
          "encoded": false
        },
        "headers": {
          "Content-Type": [
            "application/json"
          ],
          "X-Response": [
            "response1",
            "response2"
          ]
        },
        "status": 201
      },
      "type": "Synchronous/HTTP"
    }
  ],
  "metadata": {
    "pactRust": {
      "ffi": "0.4.4",
      "models": "1.0.13"
    },
    "pactSpecification": {
      "version": "4.0"
    }
  },
  "provider": {
    "name": "PactExtensionsTests-Provider"
  }
}

The key flips between e037db1f569db957 and 917ca8ce1f6b5279 on subsequent runs

adamrodger commented 1 year ago

All of the data is hard-coded in that tests btw. The JSON of the interactions itself doesn't change, just the key.

mefellows commented 1 year ago

Looks like a few more games of whack-a-mole are going to be required here.

adamrodger commented 1 year ago

I've got a message interaction doing it as well (just in case we thought it was HTTP only):

{
  "consumer": {
    "name": "PactExtensionsTests-MessageConsumer-V4"
  },
  "interactions": [
    {
      "contents": {
        "content": {
          "bool": false,
          "int": 1,
          "string": "a description"
        },
        "contentType": "application/json",
        "encoded": false
      },
      "description": "a sample request",
      "key": "fdb9aa53df5e5d7",
      "metadata": {
        "queueId": "1234"
      },
      "pending": false,
      "providerStates": [
        {
          "name": "a provider state"
        },
        {
          "name": "another provider state"
        },
        {
          "name": "a provider state with params",
          "params": {
            "baz": "bash",
            "foo": "bar"
          }
        }
      ],
      "type": "Asynchronous/Messages"
    }
  ],
  "metadata": {
    "framework": {
      "language": "C#"
    },
    "pactRust": {
      "ffi": "0.4.4",
      "models": "1.0.13"
    },
    "pactSpecification": {
      "version": "4.0"
    }
  },
  "provider": {
    "name": "PactExtensionsTests-MessageProvider"
  }
}
mefellows commented 1 year ago

Thanks! What jumps out to me there are the multiple provider states, perhaps the internal array representation is messing with it 🤷 . Or

adamrodger commented 1 year ago

Yeah and one of the provider states has multiple values also - that seems suspicious straight away

rholshausen commented 1 year ago

I'm going to remove the key generation, it is causing too may issues. This was meant to be a user supplied key (See https://github.com/pact-foundation/pact-specification/issues/72) and the idea was to auto-calculate it if it was not supplied by a user.

nsfrias commented 1 year ago

I also experiencing this issue right now (Pact-go CLI v2.0.0-beta.18).

I am happy to test any fix with my tests, when one is available.

It would also be great if Pact-go can be updated to pull the ffi version with the fix.

mefellows commented 1 year ago

Thanks @nsfrias. I'll push out an intermediate version which resolved the example I had above, if that helps. In the meantime, Ron has proposed to revert that change which would eventually fully resolve it.

nsfrias commented 1 year ago

I don't know what is the use case behind the key value. That said from my perspective reverting the change would be the best.

mefellows commented 1 year ago

The next FFI release should have the auto-generated keys removed. Once this is out we can address upstream issues (e.g. https://github.com/pact-foundation/pact-go/issues/230)

mefellows commented 1 year ago

Closing as the fix has been applied, however there is a related issue #278 that needs addressing for plugins.