-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Header is parsed incorrectly #1058
Comments
I agree, this isn't the right behaviour. |
Possibly downgrading to 10.4.0 will allow you to work around this - as it looks like this commit is the culprit: 9c7939e (cc @mefellows) |
Yes, it's a bug thanks for raising it. Will need a bit of thinking, because that change was introduced to fix an upstream issue. I |
See also #1031 |
Each header has different semantics, so maybe the solution is in specific matchers for different headers. |
Yes, that might be the way about it. |
I've tested it with |
It might be that the core is doing the split also. Out of interest, have you tried the 10.3.x branch? @rholshausen FYI |
Currently, the matching assumes all headers follow the RFC multi-value format, unless they specifically don't (like Date has a comma in that should not be split). Maybe we need to change it to assume headers are single values only, unless they are known multi-value headers. But then if someone creates a new custom header and expects it to be parsed as a multiple value one, it won't work. Maybe configuration to say a header is multi-value or not. We can also wait for everyone to move over to HTTP2, where this problem goes away because the number of values are explicit in the protocol, and not in the encoded value. |
I’m on mobile so it’s not easy to check, but I thought the current RFC just says header values are a string? I think a specific type of matcher is probably the way to go here |
If my reading is correct, all of these As a general rule, others (including unspecified headers) I believe are essentially a quoted string, which shouldn't be split. This paragraph I think is the key:
|
(AH! I forgot to hit send on it, just an FYI)
I'm sure we'll be there very soon. Remember how quickly we all moved to IPv6?
I've actually updated the Pact JS types to allow a header to be specified as a key with one or more values: export declare type TemplateHeaders = {
[header: string]:
| string
| MatchersV3.Matcher<string>
| (MatchersV3.Matcher<string> | string)[];
}; This would make it explicit in the test itself (and means the various components of the headers may also be matched). When we call |
@mefellows Unfortunately, it is not working as well for 10.0.x, 10.1.x, 10.2.x, 10.3.x. |
Hi Marcin, thanks for clarifying. I suspect that's related to the previous bug that was addressed in the related change above. I'll have to work with the team to come back to you on an ETA. |
We are really keen on work more with Pact, but this is a blocker for us to utilize the tool. Thanks for response, waiting then for more information 🙇♂️ |
👋 Thanks, this ticket has been added to the PactFlow team's backlog as PACT-714 |
I've just added a label "smartbear supported" to the ticket, which will create an internal tracking issue in PactFlow's Jira (see that comment above). We will use this to prioritise and assign a team member to this task. All activity will be public on this ticket. For now, sit tight and we'll update this ticket once we have more information on the next steps. |
@rholshausen does pact-foundation/pact-reference#259 fix the underlying issue here? I think I just need to remove any comma splitting present in Pact JS and defer to the core for logic around multi valued headers, is that right? |
No, it still follows the header grammar as documented in the RFC, but if you pass an array as the value, it should just use it as is. But I haven't tested this. |
We we have any update on this one? |
See upstream issue here: pact-foundation/pact-reference#300 |
Hello @mefellows OS: e.g. Mac OSX 10.15.7 Pact V3 interaction request:
logs:
|
It looks like the JSON object is being iterated on, and each key in the JSON structure is being converted into an index The repro is here: https://github.com/pact-foundation/pact-js/blob/repro/pact-js-issue-1058/examples/mocha/test/get-dogs.spec.js#L51
I think this is an upstream issue, but I'll create a repro in the core to prove it first. Re-opening. |
@mefellows I don't think so. The JSON string is evidently split by comma, because "curly bracket" is included in logged string
It's interesting that in my case even the real "x-info" request header (not header in interaction) is converted to array with empty string. But the real axios request contains correct "x-info" value. In your case there is splitted JSON string again.
|
I've also shown it's not a problem in the rust core's standard interface, so it is either:
I still think it's likely to be (2), as we don't do much with the headers. Still looking. |
Confirmed it's an upstream issue (see link above) and https://github.com/pact-foundation/pact-js-core/compare/repro/pact-js-issue-1058?expand=1. |
Hi @mefellows , is this fixed now? |
It doesn't look to be based on the upstream issue, but you could run a quick test of my repro (bump to latest version of Pact JS) to test that out. |
Hello @mefellows. I just tested the latest version of |
I'm guessing this might be place where it's happening. |
Software versions
Please provide at least OS and version of pact-js
"@pact-foundation/pact": "10.4.1",
v16.13.1
Issue Checklist
Please confirm the following:
Expected behaviour
Header should be parsed as a string, without default behaviour of splitting it by comma for every possible HTTP header.
Actual behaviour
No matter what value is passed in a HTTP Header and what is its name it is always split if it has a comma in it.
Steps to reproduce
Given the following code:
This test fails as pact splits the header by comma and tries to compare a part of invalid JSON between what was sent and what was received.
When the header is a string that does not contain any commas it works.
Relevant log files
The text was updated successfully, but these errors were encountered: