search

pact-foundation / pact-reference

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

fix(ffi): shared processing of body #456

Closed JP-Ellis closed 4 months ago

JP-Ellis commented 4 months ago

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 commented 4 months ago

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:

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

JP-Ellis commented 4 months ago

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

YOU54F commented 4 months ago

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.

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

JP-Ellis commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

https://github.com/pact-foundation/pact-reference/blob/111a18cd4ef93b0a9337f5a3c31d22b5949fa6a3/rust/pact_ffi/src/mock_server/handles.rs#L1682-L1687

The main matching is done on the content_type:

https://github.com/pact-foundation/pact-reference/blob/111a18cd4ef93b0a9337f5a3c31d22b5949fa6a3/rust/pact_ffi/src/mock_server/handles.rs#L1694

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

https://github.com/pact-foundation/pact-reference/blob/111a18cd4ef93b0a9337f5a3c31d22b5949fa6a3/rust/pact_ffi/src/mock_server/handles.rs#L1707-L1729

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:

https://github.com/pact-foundation/pact-reference/blob/111a18cd4ef93b0a9337f5a3c31d22b5949fa6a3/rust/pact_ffi/tests/tests.rs#L478-L535

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