-
Notifications
You must be signed in to change notification settings - Fork 218
Fix expected body when using generators #353
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
Fix expected body when using generators #353
Conversation
👋🏾 Sorry I have missed this on my radar, thanks for the great reproduction. I will make some time either this week or next week to review. |
firstly, apologies for causing you impact with the change, I knew this would potentially break someones workflow, but I wanted to ensure we could use generators in pact-ruby and didn't want to lose great work done by a contributor. I've taken a quick look but not ran any of the code locally and added a few suggestions, as I really appreciate you considering where the fix should go, and the wider impact already present with this bug.
I think this should go into That change would live in pact-support, the generator code is here https://github.com/pact-foundation/pact-support/blob/master/lib/pact/generators.rb with each individual generator here https://github.com/pact-foundation/pact-support/tree/master/lib/pact/generator if you are developing locally, you can use the Line 17 in 789596f
If the changes go into pact-support, they will be leveraged by the other pact ruby projects, to ensure the fix is available to all |
Alright thanks for the feedback. I'll have a look at that next week. |
Hey @karouf, Do we still want to take a look at this? I am currently prepping a v2 rewrite but the v1 code will remain in place for a while to support transitioning |
I'm currently trying to fit this in our plan for Q4. So I'll let you know shortly. |
I might be more included to let this through in pact-ruby, as I have a proposed v2 version of the gem, which will use the rust core, which has full support for v3/v4 pacts see #369 The v2 portion of the codebase does not share any reliance of satellite gems ( pact- support/message/mock-server ) and will ultimately be deprecated. I would rather your free time in q4 was spent upgrading to the new v2 version, rather than trying to upgrade v1 for not masses of gains. |
Since v1.66.0, with the introduction of generators, we started to see issues where instead of setting the JSON body of the expected request as a string, Pact started returning the same data but encoded as query parameters.
The consumer Pact is generated using v3 of the Pact spec with the JVM SDK, and specifically
stringType
fromPactDslJsonBody
. It produces a Pact file that contains something likeYou can find a reproduction case in a dedicated repository.
Just run
bundle exec rake pact:verify:foobar
to have a successful verification with v1.65.3.And run
FAIL=1 bundle exec rake pact:verify:foobar
to demonstrate the issue with v1.66.0.After investigating a bit, we narrowed it down to that commit.
In order to demonstrate the issue, I added this test.
My proposed fix is done in
Pact::Provider::Request::Replayable
because I'm not super familiar with the code base and I wasn't sure ifPact::Provider::Generators.apply_generators
should ever return anything else than a string or not. But if it's not the case, the fix could probably go inPact::Provider::Generators
as I suspect there is the same issue with the way path and maybe headers are generated as well.I'd be happy to fix it another way if you find it better and cover other cases if needed. Just let me know 😄