add support for users to provide custom uri normalizer by mamoonraja · Pull Request #533 · httprb/http

mamoonraja · 2019-02-21T22:07:20Z

This PR tries to tackle #378 while not breaking current functionality. I wanted to get the review while I spend more time cleaning up the code to answer the open questions I mentioned and add some documentation around it.

Adds a feature UriNormalizer
Provides users options to provide their own custom Uri normalizer by using HTTP.use(:uri_normalizer => {:custom_uri_normalizer => CustomUriNormalizer.new})
Updated http_spec test to demonstrate the functionality
By default functionality remains the same.

Open questions:

I changed rubocop for now because I am adding a new condition in request.new, but I will work on moving that code or make client handle that.
Instead of having self.normalize_uri in the request, we can just use the DefaultUriNormalizer in the feature itself. Probably related to the above comment.

…malizer

spec/support/dummy_server/servlet.rb

lib/http/request.rb

lib/http/features/uri_normalizer.rb

ixti · 2019-02-21T23:03:53Z

So, I think API should look like this:

HTTP.use(:normalize_uri => { :normalizer => normalizer })

Also, normalizer object should be ANYTHING that responds to #call method:

module NormalizeMeNot
  def self.call(uri); uri; end
end

HTTP.use(:normalize_uri => { :normalizer => NormalizeMeNot })

wtf = ->(uri) { HTTP::URI.parse "https://example.com" }
HTTP.use(:normalize_uri => { :normalizer => wtf })

To make things easier, we can support extra options in feature, that will be passed to normalizer as second argument.

DEFAULT_NORMALIZER = lambda do |uri, only: [:schema, :query...]|
  URI.new(
    :schema => only.include?(:schame) ? uri.normalized_schema : uri.schema,
    # ...

ixti · 2019-02-25T18:18:01Z

lib/http/request.rb

    # Scheme is normalized to be a lowercase symbol e.g. :http, :https
    attr_reader :scheme

+    attr_reader :uri_normalizer


No need to expose it.

I am exposing it because we are using it in wrap_request method in auto_deflate

ixti · 2019-02-25T18:18:29Z

lib/http/request.rb


    # @option opts [String] :version
    # @option opts [#to_s] :verb HTTP request method
+    # @option opts [#to_s] :uri_normalizer uri normalizer


description is redundant. It's obvious from the key name.

ixti · 2019-02-25T18:23:06Z

spec/lib/http_spec.rb

+        end
+      end
+
+      it "Use the defaul Uri normalizer when user does not use uri normalizer" do


URI should be all caps. And topic of example should start with lowercase. Also typo in defaul.
In this particular example should be moved into default context, either here:

http/spec/lib/http_spec.rb

Line 21 in 1eae155

end

or here:

http/spec/lib/http_spec.rb

Line 466 in 1eae155

end

And should look like:

it "normalizes URI" do response = HTTP.get "#{dummy.endpoint}/hello world" expect(response.to_s).to eq("hello world") end

others examples are OK to be left in here, just make them less "wordy"...

ixti · 2019-02-25T18:31:29Z

spec/lib/http_spec.rb

+        def self.call(uri)
+          uri
+        end
+      end


Don't declare module/class dynamically. You basically don't need this at all.

ixti · 2019-02-25T18:32:54Z

spec/lib/http_spec.rb

+      end
+
+      it "Use the custom Uri Normalizer method" do
+        client = HTTP.use(:normalize_uri => {:normalizer => Normalizer})


As I said above you don't need Normalizer module here. Replace this line with:

Suggested change

client = HTTP.use(:normalize_uri => {:normalizer => Normalizer})

client = HTTP.use(:normalize_uri => {:normalizer => :itself.to_proc})

ixti · 2019-02-25T18:34:05Z

spec/lib/http_spec.rb

+      end
+
+      it "Use the default Uri normalizer when user does not specify custom uri normalizer" do
+        client = HTTP.use :normalize_uri


Worth to add expectation that default normalizer will be called in here:

expect(HTTP::URI::NORMALIZER).to receive(:call).and_call_original

lib/http/request.rb

ixti · 2019-02-25T18:46:03Z

@httprb/core Would like to know your opinions on whenever we should enforce custom URI normalizers to return HTTP::URI or just assume that user knows what he's doing and returns anything that might work later down the road (if we will depend on some of HTTP::URI methods, other implementations might be lack of).

On one hand I think we should enforce, as it will allow to avoid bug reports due to crazy setup. On the other hand, I guess one of the best things this PR can bring on is optional avoidance of HTTP::Addressable (which has been reported as slow - although I'm not buying this argument).

digitalextremist · 2019-02-25T23:15:26Z

@ixti looking at #378 I agree with the claim that normalization ought not be the default behavior; or I'd add that at least that there ought to be a trustworthy method call which overrides normalization, not just requiring specialized configuration every time if you want to turn it off.

ixti · 2019-02-26T00:27:21Z

@digitalextremist Mm, that's actually doable with this PR as well:

module MyNamespace
  HTTP = ::HTTP.use(:normalize_url => :itself.to_proc)
end

But I'm totally fine if we will have some sort of global defaults configuration as well, although that is a bit out of the scope of this PR IMO. Something like (just a crazy idea):

HTTP.defaults.uri_normalizer = ->(_) { HTTP::URI.parse "https://example.com" }

mamoonraja · 2019-02-27T22:46:41Z

@ixti I addressed the comments, and I agree that adding the global defaults configuration is out of scope for this PR.

ixti · 2019-02-27T23:00:35Z

@mamoonraja thank you. I'll take a closer look to the weekend.

mamoonraja · 2019-03-08T14:38:36Z

@ixti Thanks! Do you have an ETA on when this will be merged?

ixti · 2019-03-08T15:43:58Z

@mamoonraja Sure. Sorry had a busy weeks. Will merge this weekend for sure.

mamoonraja · 2019-03-08T16:26:13Z

No problem! thanks a lot!

#533

ixti · 2019-03-11T02:33:08Z

Merged in as 7fd1356 and released as v4.1.0

@ixti

Update ruby-http to 4.4.1. ## 4.4.1 (2020-03-29) * Backport [#590](httprb/http#590) Fix parser failing on some edge cases. ([@ixti]) ## 4.4.0 (2020-03-25) * Backport [#587](httprb/http#587) Fix redirections when server responds with multiple Location headers. ([@ixti]) * Backport [#599](httprb/http#599) Allow passing HTTP::FormData::{Multipart,UrlEncoded} object directly. ([@ixti]) ## 4.3.0 (2020-01-09) * Backport [#581](httprb/http#581) Add Ruby-2.7 compatibility. ([@ixti], [@janko]) ## 4.2.0 (2019-10-22) * Backport [#489](httprb/http#489) Fix HTTP parser. ([@ixti], [@fxposter]) ## 4.1.1 (2019-03-12) * Add `HTTP::Headers::ACCEPT_ENCODING` constant. ([@ixti]) ## 4.1.0 (2019-03-11) * [#533](httprb/http#533) Add URI normalizer feature that allows to swap default URI normalizer. ([@mamoonraja]) ## 4.0.5 (2019-02-15) * Backport [#532](httprb/http#532) from master. Fix pipes support in request bodies. ([@ixti]) ## 4.0.4 (2019-02-12) * Backport [#506](httprb/http#506) from master. Skip auto-deflate when there is no body. ([@Bonias]) ## 4.0.3 (2019-01-18) * Fix missing URL in response wrapped by auto inflate. ([@ixti]) * Provide `HTTP::Request#inspect` method for debugging purposes. ([@ixti]) ## 4.0.2 (2019-01-15) * [#506](httprb/http#506) Fix instrumentation feature. ([@paul]) ## 4.0.1 (2019-01-14) * [#515](httprb/http#515) Fix `#build_request` and `#request` to respect default options. ([@RickCSong]) ## 4.0.0 (2018-10-15) * [#482](httprb/http#482) [#499](httprb/http#499) Introduce new features injection API with 2 new feaures: instrumentation (compatible with ActiveSupport::Notification) and logging. ([@paul]) * [#473](httprb/http#473) Handle early responses. ([@janko-m]) * [#468](httprb/http#468) Rewind `HTTP::Request::Body#source` once `#each` is complete. ([@ixti]) * [#467](httprb/http#467) Drop Ruby 2.2 support. ([@ixti]) * [#436](httprb/http#436) Raise ConnectionError when writing to socket fails. ([@janko-m]) * [#438](httprb/http#438) Expose `HTTP::Request::Body#source`. ([@janko-m]) * [#446](httprb/http#446) Simplify setting a timeout. ([@mikegee]) * [#451](httprb/http#451) Reduce memory usage when reading response body. ([@janko-m]) * [#458](httprb/http#458) Extract HTTP::Client#build_request method. ([@tycoon]) * [#462](httprb/http#462) Fix HTTP::Request#headline to allow two leading slashes in path. ([@scarfacedeb]) * [#454](httprb/http#454) [#464](httprb/http#464) [#384](httprb/http#384) Fix #readpartial not respecting max length argument. ([@janko-m], [@marshall-lee])

feat(uri_normalizer): add support for users to provide custom uri nor…

c862d3e

…malizer

ixti reviewed Feb 21, 2019

View reviewed changes

spec/support/dummy_server/servlet.rb Show resolved Hide resolved

ixti reviewed Feb 21, 2019

View reviewed changes

lib/http/request.rb Outdated Show resolved Hide resolved

ixti reviewed Feb 21, 2019

View reviewed changes

lib/http/features/uri_normalizer.rb Outdated Show resolved Hide resolved

refactor(uri_normalizer): update the functionality of api

1eae155

ixti requested changes Feb 25, 2019

View reviewed changes

ixti reviewed Feb 25, 2019

View reviewed changes

lib/http/request.rb Outdated Show resolved Hide resolved

refactor: update request module and tests for normalize_uri

8728c90

mamoonraja mentioned this pull request Mar 5, 2019

Cannot Delete Custom Words of Full-Size (Double Byte) Alphanumeric. watson-developer-cloud/ruby-sdk#49

Closed

ixti pushed a commit that referenced this pull request Mar 11, 2019

add swappable uri normalizer feature

7fd1356

#533

ixti closed this Mar 11, 2019

janko mentioned this pull request Dec 20, 2019

Undesirable changing of URLs by Addressable janko/down#35

Closed

ixti mentioned this pull request Jun 11, 2020

httprb changes URL encoding with requests #611

Closed

	client = HTTP.use(:normalize_uri => {:normalizer => Normalizer})
	client = HTTP.use(:normalize_uri => {:normalizer => :itself.to_proc})

Conversation

mamoonraja commented Feb 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ixti commented Feb 21, 2019

Uh oh!

ixti Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

mamoonraja Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

ixti Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

ixti Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

ixti Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

ixti Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

ixti Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ixti commented Feb 25, 2019

Uh oh!

digitalextremist commented Feb 25, 2019

Uh oh!

ixti commented Feb 26, 2019

Uh oh!

mamoonraja commented Feb 27, 2019

Uh oh!

ixti commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mamoonraja commented Mar 8, 2019

Uh oh!

ixti commented Mar 8, 2019

Uh oh!

mamoonraja commented Mar 8, 2019

Uh oh!

ixti commented Mar 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ixti commented Feb 27, 2019 •

edited

Loading