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

Using pactffi_with_pact_metadata with duplicate namespace, overwrites existing namespace contents #466

Closed
YOU54F opened this issue Oct 2, 2024 · 3 comments

Comments

@YOU54F
Copy link
Member

YOU54F commented Oct 2, 2024

Writing a pact with the following metadata

      pactffi_with_pact_metadata(pact, 'namespace1', 'var_1', 'value_1')
      pactffi_with_pact_metadata(pact, 'namespace1', 'var_2', 'value_2')
      pactffi_with_pact_metadata(pact, 'namespace2', 'var_1', 'value_1')
      pactffi_with_pact_metadata(pact, 'namespace2', 'var_2', 'value_2')
      pactffi_with_pact_metadata(pact, 'pactRust', 'var_1', 'var_1')
      pactffi_with_pact_metadata(pact, 'pactRust', 'var_2', 'var_2')

Results in the following metadata.

  "metadata": {
    "namespace1": {
      "var_2": "value_2"
    },
    "namespace2": {
      "var_2": "value_2"
    },
    "pactRust": {
      "mockserver": "1.2.9",
      "models": "1.2.3",
      "var_2": "var_2"
    },
    "pactSpecification": {
      "version": "3.0.0"
    }
  1. namespace1 only has the last entry {"var_2": "value_2"}, missing {"var_1": "value_1"}
  2. namespace2 only has the last entry {"var_2": "value_2"}, missing {"var_1": "value_1"}
  3. pactRust only has the last entry {"var_2": "value_2"}, missing {"var_1": "value_1"}, existing pactRust entries not removed. (models /mockserver)

Other notes.

  • pactSpecification namespace appears to be protected and cannot write to it
  • pactRust library versions are not overwritten, so only newly added keys bar the latest are removed, in a single pact execution

Source location for function

pact.with_pact(&|_, inner| {
let namespace = convert_cstr("namespace", namespace).unwrap_or_default();
let name = convert_cstr("name", name).unwrap_or_default();
let value = convert_cstr("value", value).unwrap_or_default();
if !namespace.is_empty() {
inner.pact.metadata.insert(namespace.to_string(), json!({ name: value }));
} else {
warn!("no namespace provided for metadata {:?} => {:?}. Ignoring", name, value);
}
!inner.mock_server_started
}).unwrap_or(false)
}

Should we do the following

  1. Fix so all added metadata is retained, unless a duplicate call is made which overwrites an existing namespace & key
  2. Create a v2 suffixed function that can accept json, rather than a using having to make multiple calls. This pattern is used in the message metadata v2 functions, but I believe this was mainly done to accept matching rules in the integration json format but are not relevant for generic top level Pact file metadata
@JP-Ellis
Copy link
Contributor

JP-Ellis commented Oct 3, 2024

I do think the current functions needs to be fixed. Adding to entries to a namespace should result in these two entries being in the namespace. This is a bug which I highly doubt anyone is relying on.

Now for accepting JSON, this really should be added. It can also fallack to handling the string as-is, namely:

  1. Try and deserialise the value into a serde_json::Value:
    2. If that succeeds, proceed with the given value in the code as before
    3. If that fails, proceed with the original string as the value

This would be a breaking change to the current behaviour for strings which were handled as strings, but can be deserialized into a JSON value:

Input Str/Current Fixed
"null" null
"1" 1
r#"{"foo": "bar"}# {"foo": "bar"}

Given this function is only for Pact metadata, I do wonder whether incorporating a breaking change into the current function is justified? Or we just be safe and create a _v2 function?

@rholshausen
Copy link
Contributor

I'll update it to return an error if you try to write to pactSpecification or pactRust. Them be mine, and you can't touch.

@YOU54F
Copy link
Member Author

YOU54F commented Nov 27, 2024

Cheers Ron!

@YOU54F YOU54F closed this as completed Nov 27, 2024
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