pact-foundation / pact-reference

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

String Provider State Params Serialised As Numbers #263

Closed adamrodger closed 1 year ago

adamrodger commented 1 year ago

See: https://github.com/pact-foundation/pact-net/issues/449

If a provider state param just happens to contain only numbers then it's serialised to the pact file as a number instead of a string.

You'd expect the pact file to contain:

"providerStates": [
  {
    "name": "state with param",
    "params": {
      "issue": "449" // <-- note that this is a string
    }
  }
]

but it contains

"providerStates": [
  {
    "name": "state with param",
    "params": {
      "issue": 449 // <-- note that this is a number instead of a string
    }
  }
]
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-937). 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

If you are using the function pactffi_given_with_param you need to pass through the value in JSON form. If it is a string value, it must be wrapped in double quotes, i.e.

pactffi_given_with_param(interaction, "state with param", name: "issue", value: "\"449\"")
adamrodger commented 1 year ago

How come we only have to do that for numbers? Other strings that get passed in don't have quotes around them and thus would be invalid JSON. I'd have to check what Boolean values do but could have a similar problem.

What happens if a regular string has quotes around it? Would those be stripped or would they cause parse errors? That seems pretty unexpected.

rholshausen commented 1 year ago

What happens if a regular string has quotes around it?

If it can't parse it as JSON, it falls back to using the raw string. The problem here is it can parse it as JSON, which is needed in case you actually want to pass numbers and booleans in.

adamrodger commented 1 year ago

So if a user passes "foo" or foo then they'll both parse to exactly the same thing, even though the user supplied two different inputs?

That feels like a bit of a foot gun.

Does that mean all FFI consumers need to escape all possible JSON values from user input, just in case it happens to parse as valid JSON when that wasn't intended? That would be a non-trivial task to implement everywhere.

What if they pass something like ["foo"]? Will that parse as an array of strings instead of the literal supplied value?

rholshausen commented 1 year ago

This is not meant to be the format the user provides, that should be an idiomatic value for the language being used. For instance, with Ruby, Python or Groovy the user would be able to pass a map of values, and it should be converted to JSON behind the scenes.

adamrodger commented 1 year ago

I'd question if this is really closed as completed. I think it's clear that users could get really weird unexpected behaviour with this.

YOU54F commented 1 month ago

I'd question if this is really closed as completed. I think it's clear that users could get really weird unexpected behaviour with this.

It would be best to prove or disprove this with tests, and then we have something to work with, as well as some documentation for users.

As to Ron's comment about using json and a commit to update the docstring for usage here

I've implemented that in your issue repro over in pact-net (thank you!), and using the documented suggestion by Ron (parsing as json) works as per your test case.

delta passing action run

Edge cases are to be expected but happy to unearth them.