fix(ffi): shared processing of body

[JP-Ellis]

The logic of processing the body within the pactffi_with_body was not consistent across the various interaction types. This refactors the logic out into a separate process_body function.

Note that as a result of this change, the behaviour of the FFI may change.

[JP-Ellis]

Fixed the issue with the failing tests. Reason being that the Content-Type header was not being set if header were None. The new behaviour is:

• If headers is None, create a HashMap and populate it
• If the headers is Some(hm), then insert the new Content-Type header only if it does not exist already. This means that if someone has explicitly set the content type header, this function will not override it.

Before merging, I just want to update the docstring to make sure it lines up with the behaviour.

[JP-Ellis]

Docstring has been updated. @rholshausen, I'll leave you do a last check before merging 👍

[YOU54F]

I believe this change has possibly caused a regression in the pact-php compat and test suite, see mentioned PR.

Changes also consumed in following PR's.

• pact-go
• pact-js-core

There are xml tests in pact-js but they are only consumable on release of pact-js-core (unless locally tested)

[JP-Ellis]

Hmm, I was worried this change may have unintended repercussions, though was relieved to see the test suite to pass... Evidently an edge case was not covered.

I'll have a look at the PR that uncovered this unintended change, though given neither Pact Go or Pact JS picked up this issue, I'm wondering whether Tien may have unintentionally baked in the broken behaviour into a test?

Once I've figured out exactly the extent of the change, I'll update the changelog to highlight this change in behaviour in case it impacts anyone else in the future.

EDIT: So I have had a quick look at the way the tests/examples are implemented, and it appears that the XML description is built from scratch, and then encoded into JSON before being passed through the FFI. The logic implemented should detect the JSON content and parse it as before though. But let me dig into that a bit more later.

[YOU54F]

Ahh so I don't believe pact-js-core or pact-go covered it.

I think the detect content type from string is saying its application/json and it isn't being transformed into xml

I think its this bit of code

https://github.com/pact-foundation/pact-reference/pull/456/files#diff-d2ed99251aebcb3c325d1e2650a83cec3b1a8d470d9b524784b0797073b47fbdL1751-L1758

And I believe that we need to check if its actually xml, when intermediate json is detected

https://github.com/pact-foundation/pact-reference/pull/456/files#diff-d2ed99251aebcb3c325d1e2650a83cec3b1a8d470d9b524784b0797073b47fbdR1696

[JP-Ellis]

The logic should be fine.

There's two content types set at the start of that function:

1. detected_type which is taken from the body string, and
2. content_type which is taken from the function argument, with a series of fallbacks.

pact-reference/rust/pact_ffi/src/mock_server/handles.rs

Lines 1682 to 1687 in 111a18c

	let detected_type = detect_content_type_from_string(body);
	let content_type = content_type
	.clone()
	.or_else(|| content_type_hint.clone())
	.or_else(|| detected_type.clone())
	.or_else(|| Some(TEXT.clone()));

The main matching is done on the content_type:

pact-reference/rust/pact_ffi/src/mock_server/handles.rs

Line 1694 in 111a18c

match content_type {

Then if that is XML, it checks if the body is JSON, and fallbacks to raw body parsing:

pact-reference/rust/pact_ffi/src/mock_server/handles.rs

Lines 1707 to 1729 in 111a18c

	Some(ct) if ct.is_xml() => {
	// The XML payload may contain one of two cases:
	// 1. A raw XML payload
	// 2. A JSON payload describing the XML payload, including any
	// embedded generators and matching rules.
	match detected_type {
	Some(detected_ct) if detected_ct.is_json() => {
	trace!("Processing JSON description for XML body");
	let category = matching_rules.add_category("body");
	OptionalBody::Present(
	Bytes::from(
	process_xml(body.to_string(), category, generators).unwrap_or_default(),
	),
	Some(ct), // Note to use the provided content type, not the detected one
	None,
	)
	}
	_ => {
	trace!("Raw XML body left as is");
	OptionalBody::from(body)
	}
	}
	}

I'm also confused as to why the XML test within the FFI test suite passed, by the PHP one did not, as the one in this repo also uses the JSON body:

pact-reference/rust/pact_ffi/tests/tests.rs

Lines 478 to 535 in 111a18c

	fn http_xml_consumer_feature_test() {
	let consumer_name = CString::new("http-consumer").unwrap();
	let provider_name = CString::new("http-provider").unwrap();
	let pact_handle = pactffi_new_pact(consumer_name.as_ptr(), provider_name.as_ptr());
	let description = CString::new("request_with_matchers").unwrap();
	let interaction = pactffi_new_interaction(pact_handle.clone(), description.as_ptr());
	let accept = CString::new("Accept").unwrap();
	let content_type = CString::new("Content-Type").unwrap();
	let response_body_with_matchers = CString::new(r#"{"version":"1.0","charset":"UTF-8","root":{"name":"ns1:projects","children":[{"pact:matcher:type":"type","value":{"name":"ns1:project","children":[{"name":"ns1:tasks","children":[{"pact:matcher:type":"type","value":{"name":"ns1:task","children":[],"attributes":{"id":{"pact:matcher:type":"integer","value":1},"name":{"pact:matcher:type":"type","value":"Task 1"},"done":{"pact:matcher:type":"type","value":true}}},"examples":5}],"attributes":{}}],"attributes":{"id":{"pact:matcher:type":"integer","value":1},"type":"activity","name":{"pact:matcher:type":"type","value":"Project 1"}}},"examples":2}],"attributes":{"id":"1234","xmlns:ns1":"http://some.namespace/and/more/stuff"}}}"#).unwrap();
	let address = CString::new("127.0.0.1:0").unwrap();
	let description = CString::new("a request to test the FFI interface").unwrap();
	let method = CString::new("GET").unwrap();
	let path = CString::new("/xml").unwrap();
	let header = CString::new("application/xml").unwrap();
	let tmp = TempDir::new().unwrap();
	let tmp_path = tmp.path().to_string_lossy().to_string();
	let file_path = CString::new(tmp_path.as_str()).unwrap();
	pactffi_upon_receiving(interaction.clone(), description.as_ptr());
	pactffi_with_request(interaction.clone(), method.as_ptr(), path.as_ptr());
	pactffi_with_header(interaction.clone(), InteractionPart::Request, accept.as_ptr(), 0, header.as_ptr());
	// will respond with...
	pactffi_with_header(interaction.clone(), InteractionPart::Response, content_type.as_ptr(), 0, header.as_ptr());
	pactffi_with_body(interaction.clone(), InteractionPart::Response, header.as_ptr(), response_body_with_matchers.as_ptr());
	pactffi_response_status(interaction.clone(), 200);
	let port = pactffi_create_mock_server_for_pact(pact_handle.clone(), address.as_ptr(), false);
	expect!(port).to(be_greater_than(0));
	// Mock server has started, we can't now modify the pact
	expect!(pactffi_upon_receiving(interaction.clone(), description.as_ptr())).to(be_false());
	let client = Client::default();
	let result = client.get(format!("http://127.0.0.1:{}/xml", port).as_str())
	.header("Accept", "application/xml")
	.send();
	match result {
	Ok(res) => {
	expect!(res.status()).to(be_eq(200));
	expect!(res.headers().get("Content-Type").unwrap()).to(be_eq("application/xml"));
	expect!(res.text().unwrap_or_default()).to(be_equal_to("<?xml version='1.0'?><ns1:projects id='1234' xmlns:ns1='http://some.namespace/and/more/stuff'><ns1:project id='1' name='Project 1' type='activity'><ns1:tasks><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/></ns1:tasks></ns1:project><ns1:project id='1' name='Project 1' type='activity'><ns1:tasks><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/><ns1:task done='true' id='1' name='Task 1'/></ns1:tasks></ns1:project></ns1:projects>"));
	},
	Err(_) => {
	panic!("expected 200 response but request failed");
	}
	};
	let mismatches = unsafe {
	CStr::from_ptr(pactffi_mock_server_mismatches(port)).to_string_lossy().into_owned()
	};
	pactffi_write_pact_file(port, file_path.as_ptr(), true);
	pactffi_cleanup_mock_server(port);
	expect!(mismatches).to(be_equal_to("[]"));
	}

I would need to enable trace-level logging and run the Pact PHP tests to properly understand that is going on.