pact-foundation / pact-reference

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

pactffi_pact_handle_write_file when used with plugins does not write 'transport' key to pact #458

Open YOU54F opened 4 months ago

YOU54F commented 4 months ago

Expected - Calling pactffi_pact_handle_write_file will result in the same pact file as pactffi_write_file

Actual - Calling pactffi_pact_handle_write_file will result in a pact file, with a missing transport key in the plugin interaction

Reproducer - https://github.com/pact-foundation/pact-reference/compare/master...YOU54F:pact-reference:bug/pactffi_pact_handle_write_file_no_transport

The test skips sending a consumer request, and checking mismatches, and simply aims to serialise the file after starting the mock server. Sending a valid consumer request does not change the outcome

Uncomment pactffi_pact_handle_write_file and comment out pactffi_write_pact_file linw to see an error

#[test_log::test]
fn protobuf_consumer_test() {
  let consumer_name = CString::new("protobuf-consumer").unwrap();
  let provider_name = CString::new("protobuf-provider").unwrap();
  let pact_handle = pactffi_new_pact(consumer_name.as_ptr(), provider_name.as_ptr());
  pactffi_with_specification(pact_handle, PactSpecification::V4);
  let description = CString::new("a request to a plugin").unwrap();
  let interaction = pactffi_new_sync_message_interaction(pact_handle, description.as_ptr());
  let plugin_name = CString::new("protobuf").unwrap();
  pactffi_using_plugin(pact_handle, plugin_name.as_ptr(),null());

  let content_type = CString::new("application/grpc").unwrap();
  let proto_path: PathBuf = std::env::current_dir().unwrap().join("../../proto/area_calculator.proto");
  let json = json!({
    "pact:proto":  proto_path,
    "pact:proto-service": "Calculator/calculateOne",
     "pact:content-type": "application/protobuf",
     "request": {
       "rectangle": {
         "length": "matching(number, 3)",
         "width": "matching(number, 4)"
       }
     },
     "response": {
       "value": ["matching(number, 12)"]
     }
   });
  let body = CString::new(json.to_string()).unwrap();
  let address = CString::new("0.0.0.0").unwrap();
  let transport = CString::new("grpc").unwrap();

  pactffi_interaction_contents(interaction.clone(), InteractionPart::Request, content_type.as_ptr(), body.as_ptr());

  let port = pactffi_create_mock_server_for_transport(pact_handle.clone(), address.as_ptr(), 0, transport.as_ptr(), null());

  expect!(port).to(be_greater_than(0));

  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_write_pact_file(port, file_path.as_ptr(), true);
  // Uncomment pactffi_pact_handle_write_file and comment out pactffi_write_pact_file to see an error
  // pactffi_pact_handle_write_file(pact_handle, file_path.as_ptr(), true);
  pactffi_cleanup_mock_server(port);
  pactffi_cleanup_plugins(pact_handle);

  let contents = fs::read_to_string(tmp_path + "/protobuf-consumer-protobuf-provider.json").unwrap();
  let json: Value = serde_json::from_str(&contents).unwrap();

  assert_eq!(json.get("interactions").unwrap()[0]["transport"],"grpc");
}

relevant code

JP-Ellis commented 4 months ago

From the perspective of the FFI, I would first question whether we need to both function at all? Or should we deprecate one in favour of the other?

Having said that, if we do decide to keep both, I agree they should probably produce the same result.

Do you know which one is the more correct output? I'm guessing that pactffi_pact_handle_write_file is incorrect since, as you put it, it is missing the transport key?

YOU54F commented 4 months ago

I hadn't seen pactffi_pact_handle_write_file before, but it was used in pact-net and I have been looking at the work to incorporate plugins into pact-net.

I think it might be because its a message pact handle I get back from pactffi_new_sync_message_interaction and pactffi_pact_handle_write_file requires a pact handle.

`pact_ffi_write_pact looks correct to me (in the fact the transport key is written which allows plugins to be used on the verification side)

I don't think pactffi_with_specification is suitable for the message pact handle and aligns with some of the message handle confusion you and me have had recently

JP-Ellis commented 3 months ago

Yeah, this definitely seems to link with #440.

From the recent work in Pact Python, I can confirm that it is possible to use message interactions without using MessagePactHandle and MessageHandle (using, instead PactHandle and InteractionHandle respectively).

rholshausen commented 3 months ago

Found the root cause of the problem.

pactffi_write_pact_file calls pact_mock_server::write_pact_file which retrieves the plugin mock server Pact and then writes it out. The plugin mock server knows all about the transport being used and can set that field.

pactffi_pact_handle_write_file writes out the Pact stored with the handle. It is generic, and knows nothing about plugins and transports.