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

Mismatch with header 'Accept' #1031

Closed
4 of 5 tasks
danymarques opened this issue Dec 22, 2022 · 4 comments
Closed
4 of 5 tasks

Mismatch with header 'Accept' #1031

danymarques opened this issue Dec 22, 2022 · 4 comments
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@danymarques
Copy link

Software versions

  • OS: Mac OSX 13.0.1
  • Consumer Pact library: Pact JS v10.4.0
  • Node Version: 14.21.1

Issue Checklist

Please confirm the following:

  • I have upgraded to the latest
  • I have read the FAQs in the Readme
  • I have triple checked, that there are no unhandled promises in my code and have read the section on intermittent test failures
  • 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 reproducible git repository (see below) to illustrate the problem

Expected behavior

When giving multiple arguments to a header, the tests should work.

Actual behavior

When we give multiple arguments to a header, eg:

Accept: 'application/problem+json, application/json, text/plain, */*',

the tests fail with the following error:

1.0 Mismatch with header 'Accept': Expected header 'Accept' to have value 'application/problem+jsonapplication/jsontext/plain*/*' but was 'application/problem+json'

which means that the headers are not handled correctly.

Steps to reproduce

  • git clone git@github.com:danymarques/pact-error-demo.git
  • npm ci
  • npm run test
@danymarques danymarques added the bug Indicates an unexpected problem or unintended behavior label Dec 22, 2022
@mefellows
Copy link
Member

Thanks for this. I think it's a duplicate of #964. I've just pushed up a fix that should fix it, at least as far as we can in Pact JS.

You can now either specify headers as a comma delimited list (as you have in your repro) or explicitly as an [] of items, each of which can also have matching rules if desired.

I've noticed a small bug with the way it reports mismatches if the request header doesn't match, but you can see what's happening in the debug logs when enabled. I'll raise an upstream issue for that.

@danymarques
Copy link
Author

Hi @mefellows ,
I compiled your branch and tested it in the project and it works perfectly.
When are you planning to release it?
Cheers

@GeorgeTailor
Copy link

@mefellows afair HTTP header does not have any constraints to be either a string or a comma-separated string or an array, for example, we use a JSON in a header with multiple properties, thus the object has commas. When it is sent as a string, some splitting magic happens and probably pact tries to treat broken parts of json as a separate array entries, while in reality it is a single string, but with commas.

@mefellows
Copy link
Member

You're right, it is a bug - thanks for raising it. I'll need to review the RFC and the current core implementation to think of the best way to handle this.

mefellows added a commit that referenced this issue Jul 8, 2023
Reverts the core change in 9c7939e
Relates to #1031
Fixes #1058
mefellows added a commit that referenced this issue Jul 8, 2023
Reverts the core change in 9c7939e
Relates to #1031
Fixes #1058

chore: allow v4 tests to run without env var for contributors

chore: update debug message for headers

chore: update mocha test to use correct multi-valued header syntax
mefellows added a commit that referenced this issue Jul 8, 2023
Reverts the core change in 9c7939e
Relates to #1031
Fixes #1058

chore: allow v4 tests to run without env var for contributors

chore: update debug message for headers

chore: update mocha test to use correct multi-valued header syntax
mefellows added a commit that referenced this issue Jul 8, 2023
Reverts the core change in 9c7939e
Relates to #1031
Fixes #1058

chore: update debug message for headers
chore: update mocha test to use correct multi-valued header syntax
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

No branches or pull requests

3 participants