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

Pact verifier mangles JSON request body #894

Closed
4 of 5 tasks
iknowcss opened this issue Jul 11, 2022 · 6 comments · Fixed by #897
Closed
4 of 5 tasks

Pact verifier mangles JSON request body #894

iknowcss opened this issue Jul 11, 2022 · 6 comments · Fixed by #897
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@iknowcss
Copy link

Software versions

Please provide at least OS and version of pact-js

  • OS: Linux Mint 20.2 (x86_64)
  • Pact Node version: @pact-foundation/pact-node@10.17.2
  • Node Version: 14.17.6
  • Other Versions: @pact-foundation/pact@9.18.0 and 9.18.1

Issue Checklist

Please confirm the following:

  • I have upgraded to the latest
  • I have the read the FAQs in the Readme
  • I have triple checked, that there are no unhandled promises in my code
  • I have set my log level to debug and attached a log file showing the complete request/response cycle
  • For bonus points and virtual high fives, I have created a reproduceable git repository (see below) to illustrate the problem

Expected behaviour

Verifier makes a request to the provider with body:

{"order_line":{"qty_ordered":10,"unit_price":19.99,"description":"Added by system","discount_amount":0.5,"discount_rate":0}}

Actual behaviour

Verifier makes a request to the provider with body:

{"{\"order_line\":{\"qty_ordered\":10,\"unit_price\":19.99,\"description\":\"Added by system\",\"discount_amount\":0.5,\"discount_rate\":0}}":""}

Steps to reproduce

Use the following contract to run the pact verifier against a stub provider:

ExampleContract.zip

Run this command to start a stub provider which echos the request method, url, headers and body:

node -e "const http = require('http'); const server = http.createServer((req, res) => { console.log(req.method + ' ' + req.url); console.log(req.headers); console.log(''); req.on('data', (data) => {process.stdout.write(data);}); req.on('end', () => { console.log('\n');res.end('{}'); }); }); server.listen(1988, () => console.log('running'))"

In @pact-foundation/pact@9.17.3 the last interaction request looks like this:

PATCH /v1/orders/cl1q49gs30005qy695yk54tpp/order_lines/cl1q49gs20004qy69alpp7lln
{
  'content-type': 'application/x-www-form-urlencoded',
  'content-length': '124',
  host: 'localhost:36605',
  authorization: 'Bearer [truncated]',
  'x-forwarded-for': '127.0.0.1',
  cookie: '',
  connection: 'close'
}

{"order_line":{"qty_ordered":10,"unit_price":19.99,"description":"Added by system","discount_amount":0.5,"discount_rate":0}}

...but @pact-foundation/pact@9.18.0 and 19.18.1 the last interaction request looks like this:

PATCH /v1/orders/cl1q49gs30005qy695yk54tpp/order_lines/cl1q49gs20004qy69alpp7lln
{
  'content-type': 'application/x-www-form-urlencoded',
  'content-length': '145',
  host: 'localhost:37719',
  authorization: 'Bearer [truncated]',
  'x-forwarded-for': '127.0.0.1',
  cookie: '',
  connection: 'close'
}

{"{\"order_line\":{\"qty_ordered\":10,\"unit_price\":19.99,\"description\":\"Added by system\",\"discount_amount\":0.5,\"discount_rate\":0}}":""}

Relevant log files

debug.log

@iknowcss iknowcss added the bug Indicates an unexpected problem or unintended behavior label Jul 11, 2022
@mefellows mefellows transferred this issue from pact-foundation/pact-js-core Jul 16, 2022
@mefellows
Copy link
Member

Thanks for the great bug report @iknowcss.

I had a quick play in the hotel, and it looks like this line is causing the issue in your case. This seems to mess up the middleware that intercepts the body to allow a requestFilter to modify the body (see #873).

In your case, it looks like specifying an explicit Content-Type header of application/json resolves it.

I'll look into why this specific pathway comes up in the code, but at least we have some pointers now.

@iknowcss
Copy link
Author

@mefellows thanks for looking into it! Yeah I noticed the content type is wrong at some point in my investigation. I will change that on my end and confirm it fixes the problem.

This is a bit of a weird one I guess because my consumer request is technically wrong (should set the correct header) but my tests broke after a minor upgrade. Maybe you could make the case that the previous version had a bug and the latest version fixes it 🤷‍♂️

Anyway, thanks again for the help and I will report back soon.

@mefellows
Copy link
Member

No worries! I had a chat with a few maintainers this evening to see if I was missing anything, but basically it just happens to be the default (in the Ruby based implementation, which is the 9.x.x line) that it uses the application/x-www-form-urlencoded content type if there is a body but no content type is specified.

The change we introduced is definitely backwards incompatible, but accidentally so in the way you allude to.

I have some ideas as to how I can address this - the primary one is detecting the lack of content type but the presence of what looks to be a single JSON body and warning the user. This would indicate incorrect consumer test case setup.

Do you have enough to resolve your immediate issue?

@iknowcss
Copy link
Author

@mefellows I believe so, I will let you know today once I confirm that adding the content type fixes my problem.

@iknowcss
Copy link
Author

@mefellows I can confirm that adding in the Content-Type to the matchers on the consumer side and then running the verifier does solve the problem.

@mefellows
Copy link
Member

Great, thanks for confirming @iknowcss. I'll get a small patch in the next few days to warn about this situation. Thanks for bringing it to our attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants