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 invalid scheme error with Addressable::Template #758

Merged
merged 3 commits into from Jun 3, 2018

Conversation

uiur
Copy link
Contributor

@uiur uiur commented May 19, 2018

Fix #489

problem

Addressable::Template#match expects a valid uri with scheme
sporkmonger/addressable#208 (comment)

It works with most cases without uri scheme, but it raises an error with some cases like 127.0.0.1:8080.

I've found that it causes the error when using webmock with System Spec and chrome driver.

solution

Always pass a uri with scheme to Addressable::Template#match.

The difference from #739 is

  • adding a spec
  • making failing specs pass

@bblimke
Copy link
Owner

bblimke commented May 19, 2018

@uiureo thank you for the PR.

Could you please explain why have you removed all these urls in https://github.com/bblimke/webmock/pull/758/files#diff-a5f6621858e36a69278262b16c576604 ?

These urls were there to guarantee that WebMock can match urls without scheme provided with urls with the default scheme, which is not the case anymore.
As I understand this change breaks backwards compatibility?

@uiur
Copy link
Contributor Author

uiur commented May 19, 2018

Ah I understand. This is mistake.
I will fix it in a way that it doesn't change WebMock::Util::URI.variations_of_uri_as_strings.

@uiur
Copy link
Contributor Author

uiur commented May 19, 2018

Added with_scheme: true option to WebMock::Util::URI.variations_of_uri_as_strings instead.

@bblimke How do you think?

@@ -47,7 +47,7 @@ def self.variations_of_uri_as_strings(uri_object)
uris = uris_with_inferred_port_and_without(uris)
end

if normalized_uri.scheme == "http"
if normalized_uri.scheme == "http" && !with_scheme
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the with_scheme flag here. The condition is "if not with scheme" and the line below compares returns values with scheme and without.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag means "returns only variations with scheme", but I agree it's confusing.

only_variations_with_scheme might be better?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes only_with_scheme would be more meaningful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated 1962400

@uiur
Copy link
Contributor Author

uiur commented Jun 2, 2018

@bblimke Could you review this again?

@bblimke
Copy link
Owner

bblimke commented Jun 3, 2018

@uiureo sorry. Looks good. Thank you!

@bblimke bblimke merged commit 20e8d38 into bblimke:master Jun 3, 2018
@bblimke
Copy link
Owner

bblimke commented Jun 3, 2018

Released as 3.4.2. Thank you!

@alain-andre
Copy link

Hi there, I'm having this issue and I'm wondering why I cannot see this fix in the latest code :

WebMock::Util::URI.variations_of_uri_as_strings(uri).any? { |u| template.match(u) }

It has been remove on commit

I do not understand why in the UT it adds the only_with_scheme of the WebMock::Util::URI.variations_of_uri_as_strings method but removes it from the request_pattern class.

Any idea ?

@bblimke
Copy link
Owner

bblimke commented Sep 11, 2020

@alain-andre it has been removed due to this issue #828

The problem was that the only_with_scheme solution is not complete.

Hopefully the issue will be resolved once #890 is finished. @guppy0356

@uiur uiur deleted the fix-489 branch September 13, 2020 09:58
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.

Invalid scheme format. (Addressable::URI::InvalidURIError)
3 participants