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

config URI normalization by default #378

Closed
sysout opened this issue Sep 30, 2016 · 9 comments
Closed

config URI normalization by default #378

sysout opened this issue Sep 30, 2016 · 9 comments

Comments

@sysout
Copy link

sysout commented Sep 30, 2016

Hi,

It seems that HTTP gem do URI normalization by default and I can't find the option to change this behavior. I have an application that won't work if HTTP gem do URI normalization. For example, HTTP.get(url) will decode %2B to +
Please let me know if we can control the URI normalization or not. Thank you!

@tarcieri
Copy link
Member

tarcieri commented Oct 1, 2016

I'd prefer to be able to configure this and shut it off as well. Normalization has been the cause of some past issues, e.g. #206 #246 #259 #325

@ixti
Copy link
Member

ixti commented Oct 3, 2016

@tarcieri I agree that we should allow enable/disable normalization as well as I think it will make sense to allow use different backends for URI (Addressable::URI / ::URI), so that those who don't want to use addressable (there was an issue for that) would be able to avoid it.

@mamoonraja
Copy link
Contributor

mamoonraja commented Jan 30, 2019

@ixti @tarcieri We are running into the same issue, Is the fix coming in near future or not?

@ixti
Copy link
Member

ixti commented Jan 31, 2019

@mamoonraja there's no planned ETA. PRs are welcome - I'll be happy to help with that.

@mamoonraja
Copy link
Contributor

@ixti I am working on a PR right now, and I am approaching it in the following way:

  • I am adding an option normaliz_uri to the request object, which will be true as default to keep the library backward compatible
  • method normalize_uri will be renamed to construct_url, something like:
 # @return [HTTP::URI] URI with all componentes but query being normalized.
 def construct_uri(uri, normalize_uri)
      uri = HTTP::URI.parse uri

      return uri unless normalize_uri

      HTTP::URI.new(
        :scheme     => uri.normalized_scheme,
        :authority  => uri.normalized_authority,
        :path       => uri.normalized_path,
        :query      => uri.query,
        :fragment   => uri.normalized_fragment
      )
 end

Do you think it's a viable solution without hurting current users?

@ixti
Copy link
Member

ixti commented Feb 12, 2019

What if we would do this via feature instead? Like this:

HTTP.use(:uri_normalizer => :itself.to_proc).get(...)

By default normalizer will be what we have now.

@mamoonraja
Copy link
Contributor

I am adding a feature uri_normalizer and I am finishing up the pr right now.

One more query, the way I am adding the feature is that the new feature will handle normalizing the URI, and normalize_uri will move out of request. Following the conventions in the library, I am handling it in wrap_request method inside the feature.

This will break the users calling HTTP::Request.new directly, the way around that is to keep the normalize URI inside the Request module and add an option normalize_uri to the Request module. Which will be passed as false for the case when the user send something like:

HTTP.use(:uri_normalizer => {normalize_uri=>false}).get(...)

Does it make sense? I personally favor the first way, handling normalizing the URI inside the feature. But that will be a breaking change.

@ixti
Copy link
Member

ixti commented Feb 14, 2019

I'm not sure I understand.

  1. You need to extend HTTP::Request to accept URI normalizer:
module HTTP
  class Request
    # Default uri normalizer
    def self.normalize_uri(uri); end

    def initialize(opts)
      # ...
      @uri_normalizer = opts.fetch(:uri_normalizer) { self.class.method(:normalize_uri) }
      @uri = @uri_normalizer.call(opts.fetch(:uri))
    end

    # ...
  end
end

Something like this

  1. You will need to create a feature, that will allow to use custom normalizer, which will be passed to Reques.new from within HTTP::Client#build_request.

@ixti
Copy link
Member

ixti commented Mar 11, 2019

We have released v4.1.0 version that allows you to disable URI normalization:

normalizer = ->(uri) { ::HTTP::URI.parse(uri) }
HTTP.use(:uri_normalizer => { :normalize => normalizer }).get(...)

Notice though that some of the internals depend on HTTP::URI API, so ensure that returned object of your normalizer is HTTP::URI.

@ixti ixti closed this as completed Mar 11, 2019
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

No branches or pull requests

4 participants