Fix regression: Allow using Addressable::Template for URI host part

[valscion]

Ever since webmock v3.4.2, we've had a few failing specs that have blocked us from upgrading webmock. All of them are a variant of Addressable::URI::InvalidURIError error with the message Invalid character in host

The full error looks like this:

  1) Mailchimp::List#subsribe_to_list correctly generates the Mailchimp member ID (MD5 of lowercase address)
     Failure/Error: expect(api_request).to have_been_made
     
     Addressable::URI::InvalidURIError:
       Invalid character in host: '{data_center}.api.mailchimp.com'
     # ./lib/mailchimp/list.spec.rb:66:in `block (3 levels) in <top (required)>'

and the spec scenario looks like this:

describe NewsletterSubscriptionsController, type: :controller do
  it 'correctly generates the Mailchimp member ID (MD5 of lowercase address)' do
    api_pattern = Addressable::Template.new(
      'https://{data_center}.api.mailchimp.com/3.0/lists/{list_id}/members/{member_id}'
    )
    member_id = '8809a2bcd95c14cbb88859efbb6a3031' # MD5 of 'test@test.åäö.fi'
    expected_pattern = api_pattern.partial_expand(member_id: member_id)
    
    api_request = stub_request(:put, expected_pattern).with(
      body: {
        email_address: 'TEST@TEST.ÅÄÖ.FI',
        status: 'subscribed',
        ip_signup: 'test-remote-ip'
      }
    )
    
    post :create, params: { email: 'TEST@TEST.ÅÄÖ.FI' }
    expect(api_request).to have_been_made
  end
end

At first we thought that our spec was somehow broken. I looked into this more closely today, and I think it might be a bug in webmock, possibly caused by #758

I wasn't able to figure out another way to specify that we're not interested in the lowest sub-domain in the URL (the {data_center} part in my example). It seems like using a template right after https:// part should work, as per addressable docs, but it doesn't seem to work anymore.

This PR contains a failing test to show the error, and a commit that fixes the failure.

The change avoids going through Addressable::URI.heuristic_parse when the input is already an Addressable::URI instance. This avoid the error raised from heuristic_parse to be triggered.

[valscion]

Ok looks like #798 does not trigger a Travis build for some reason, so I created #799 that contains my new spec and the reverse of #758 to demonstrate the existence of a regression.

[valscion]

Hi there ☺️. Have you had a chance to look at this? I'm mainly asking if this added test case is something webmock would want to support but has accidentally broken?

This PR contains a failing test to show the error. I'm not sure how to proceed in fixing that, so any tips would be appreciated.

[bblimke]

@valscion I haven't had a chance to investigate that yet. If it's something that was supported and now is broken then I guess we should bring the support back. I can have a look when I get some time, unless someone finds the fix before me.

[mrageh]

@valscion have you found a solution to this problem, we've just upgraded webmock and been hit by this?

[valscion]

Nope, we're stuck in webmock 3.4.1

[valscion]

Rebased on top of current master to show that the spec is still failing as expected.

I might have some time today to look into this to see if I could fix it — otherwise, would appreciate pointers on how to go fixing this.

[valscion]

Seems like all specs pass when I merely remove the WebMock::Util::URI.heuristic_parse from being used in WebMock::URIAddressablePattern#matches? code path. Huh.

The spec added in #758 that caused this regression still passes with this change, so this PR should be good to go? Care to check it out @bblimke when you've got time? 💞

[bblimke]

@valscion I believe that with this change you are ignoring @pattern.pattern and soimply matching the passed uri with the pattern generated with the same uri, which obviously matches :) feel free to add a spec with webmock pattern that doesn't match your url (e.g different) domain and this code will still match.

[bblimke]

Perhaps we should silently rescue InvalidURiError and try again with not normalized version of the pattern?
@rafaelfranca since you are also dealing with similar issue, any thoughts on that?

[valscion]

Oh true -- seems like we indeed need some negative assertion specs instead of only positive cases here ☺️. I was quite surprised that none of the existing specs failed with this change here

[valscion]

Sorry, I haven't had the time to do the requested changes in this PR. I can however confirm that the issue as described in this PR is still a problem in v3.8.0 and prevents us from upgrading from v3.4.1 😓

[valscion]

Hi @bblimke, I now added the negative assertion spec case (and you're right, it did fail as expected as the code was not correct).

I went with what I presume was "silently rescue InvalidURiError and try again with not normalized version of the pattern" as you commented in #797 (comment)

The tests do pass now. Is there anything left to address here?

Would love to get us to upgrade Ruby to 2.6 but as we're stuck in webmock 3.4.1 we're not yet able to upgrade to Ruby 2.6 as it needs a newer version of webmock 😅

EDIT: We were able to monkeypatch this change in to get to webmock v3.8.2, so there's no rush on our side, but of course it would be nice if we didn't have to have the following monkeypatch applied:

# TODO: Remove this warning and monkeypatch once https://github.com/bblimke/webmock/pull/797 ships
if WebMock::VERSION != '3.8.2'
  STDERR.puts "!!! " + ("NOTE " * 15) + "!!!"
  STDERR.puts "!!! " + ("NOTE " * 15) + "!!!"
  STDERR.puts "!!! " + ("NOTE " * 15) + "!!!"
  STDERR.puts
  STDERR.puts "--------->>>> Check if webmock monkeypatch still applies in spec/support/webmock.rb <<<<---------"
  STDERR.puts "--------->>>> Is https://github.com/bblimke/webmock/pull/797 merged and in a new release? <<<<---------"
  STDERR.puts
  STDERR.puts "!!! " + ("NOTE " * 15) + "!!!"
  STDERR.puts "!!! " + ("NOTE " * 15) + "!!!"
  STDERR.puts "!!! " + ("NOTE " * 15) + "!!!"
end
module WebMock
  class URIAddressablePattern
    # https://github.com/bblimke/webmock/blob/77e8543b334d9b0fa09597c6de8ba3b4e9feee0d/lib/webmock/request_pattern.rb#L183-L191
    private def matches_with_variations?(uri)
      template =
        begin
          Addressable::Template.new(WebMock::Util::URI.heuristic_parse(@pattern.pattern))
        rescue Addressable::URI::InvalidURIError
          Addressable::Template.new(@pattern.pattern)
        end
      WebMock::Util::URI.variations_of_uri_as_strings(uri).any? { |u| template.match(u) }
    end
  end
end

[bblimke]

Thank you @valscion !

[bblimke]

It's now released as 3.8.3

[valscion]

Nice, thank you!