pact-foundation / pact-reference

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

[FFI] `with_body` and `with_binary_file` confusion #336

Closed JP-Ellis closed 7 months ago

JP-Ellis commented 8 months ago

Summary

The FFI provides two functions to set the expected body of an interaction:

Both function require the body to be passed as a char* pointer to an array, but they differ in that (at least according to the documentation):

Based on the discussion in this Slack thread it would appear that with_binary_file can only be used with V3 (or later?) interactions.

This leaves unfortunately no way to specify an interaction with an arbitrary binary body for interactions using versions prior to V3 (at least, according to the documentation).

Proposed changes

Alternative 1

A new with_binary_body method

To avoid changing the behaviour of existing functions, the simplest change might just be to add a new with_binary_body function for V1 interactions. It would act in a similar way to with_body, albeit with little/no smarts in parsing the content.

Update with_binary_file and its documentation

The with_binary_file documentation should be updated to make it very clear that this function should not be used for interaction versions prior to V3.

Furthermore, I would suggest that the method be updated to do this check programmatically, and return a failure if with_binary_file is called with an incompatible version.

Alternative 2

Update with_binary_file behaviour based on interaction version

Instead of adding a new function, and having with_binary_file work only for interactions using v3 or later, the behaviour of with_binary_file could be adjusted based on the interaction version such that:

JP-Ellis commented 8 months ago

Tagging @rholshausen, @mefellows, @YOU54F and @tienvx for comments and feedback. This was discussed before, and I want to make sure the above captures the relevant part of this discussion.

rholshausen commented 8 months ago

I think option 1 is the way to go. Leave with_binary_file as is for now.

tienvx commented 8 months ago

About option 2, I created a PR #327 . I think it's not released yet, so you have to build it manually

rholshausen commented 7 months ago

0.4.10 released