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

Fix issue where a URI with a duplicate of the same param does not allow a match for the same URI #602

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kcdragon
Copy link
Contributor

See issue #515

A couple things to note about this fix.

  • I wrote a Cucumber feature test because this was the best way for me to troubleshoot this issue. However, I don't think this is the most appropriate type of test. It should probably be an RSpec test. I tried to write a test for this feature in one of the RSpec files (specifically spec/lib/vcr/library_hooks/webmock_spec.rb) but I was not able to get it working. Any ideas where this test should go? If someone could point out a similar existing test, that would be great.
  • The Cucumber feature test I wrote is basically a copy of "uri.feature". However, when I added my fix the faraday examples did not pass. I am not completely surprised by this because my fix targeted WebMock and these examples probably don't use that. My thought is that if I am on the right track, I can update this PR with the fix for faraday but I want to make sure my approach so far is a good one. I would hopefully write an RSpec unit test for these faraday examples (instead of cucumber).The faraday examples I am referring to are these:
| c.hook_into :faraday  | faraday (w/ net_http) |
| c.hook_into :faraday  | faraday (w/ typhoeus) |
  • I'm wondering if the change I made should be an optional one. We could expose a VCR config option called allow_duplicate_query_string_params that modified the WebMock query_values_notation if VCR config option was true (we set it to false by default). Let me know your thoughts on whether this should be a config option or always in place.

@berlintam
Copy link

Thanks so much for posting this fix! It really helped us with our project.

Any chance this can make its way into master?

@krainboltgreene
Copy link
Contributor

krainboltgreene commented May 10, 2019

@kcdragon I know this has taken a long time, but can you please update from master?

@kcdragon
Copy link
Contributor Author

@kcdragon I know this has taken a long time, but can you please update from master?

Hi @krainboltgreene
I updated the branch with master but there is a failing test. I'll try to look into this over the weekend and see if its a quick fix.

@olleolleolle
Copy link
Member

Hello, @kcdragon! Master changed, again, what does this PR look like, rebased, now? Thanks for your continued perseverance!

@kcdragon kcdragon force-pushed the fix-duplicate-param-in-uri branch from 725f983 to ff4eb59 Compare May 29, 2020 00:54
@kcdragon
Copy link
Contributor Author

kcdragon commented May 29, 2020

@olleolleolle I rebased with master and its passing locally for me now. The build is failing though. It looks like the build server is choosing version 1.8.11 of webmock and locally I've got 3.8.3. I took a look at the webmock code and it looks like the field I am making use of, query_values_notation, was added in 1.19.0 (see bblimke/webmock@f158a3c).

webmock isn't locked on anything in gemspec so I'm guessing you want to allow any version (including old versions) of this dependency? I see a couple options if we don't want to require a version greater than 1.19.0

  • Only set query_values_notation if it exists on ::WebMock::Config.instance
  • Add a config option to VCR that sets query_values_notation and then warn if it doesn't exist.

Let me know your thoughts.

@kcdragon
Copy link
Contributor Author

@olleolleolle I thought a little bit more about this and I'm now thinking it would be pretty reasonable to require webmock >= 1.19.0. I saw VCR now requires a Ruby >= 2.3 so it seems pretty unlikely to me that someone would have Ruby 2.3 (released late 2016) and a webmock from 2014. Let me know your thoughts.

@kcdragon kcdragon force-pushed the fix-duplicate-param-in-uri branch from ff4eb59 to 6282858 Compare June 4, 2020 01:41
@kcdragon kcdragon force-pushed the fix-duplicate-param-in-uri branch from 6282858 to 1007ab2 Compare February 18, 2021 04:43
@kcdragon kcdragon force-pushed the fix-duplicate-param-in-uri branch from 1007ab2 to 1023d98 Compare February 18, 2021 04:57
@kcdragon
Copy link
Contributor Author

@olleolleolle I'm coming back around to this now. I pushed the change I mentioned in #602 (comment). The build is now failing with

An error occurred while loading ./spec/lib/vcr/library_hooks/typhoeus_0.4_spec.rb.
Failure/Error: require 'webmock'
NoMethodError:
  undefined method `on_complete' for Typhoeus:Module
# ./vendor/bundle/ruby/2.4.0/gems/webmock-1.19.0/lib/webmock/http_lib_adapters/typhoeus_hydra_adapter.rb:50:in `remove_after_request_callback'

It looks like the version of Typhoeus being with Gemfile.typhoeus-0.4 doesn't have a method needed in the newer version of webmock. I'm guessing a patch for that method could be added here lib/vcr/library_hooks/typhoeus_0.4.rb but I'm wondering if it makes more sense to drop support for typhoeus 0.4. It looks like it was expected to be dropped anyway a couple major versions ago https://github.com/vcr/vcr/blob/master/lib/vcr/library_hooks/typhoeus_0.4.rb#L101.

Let me know your thoughts.

@olleolleolle
Copy link
Member

olleolleolle commented May 4, 2021

@kcdragon Thanks for the detailed report, and the details.

Yes, we should use a Typhoeus + webmock which is up-to-date. Dropping Ye Olde Versions is alright by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants