lostisland / faraday

Simple, but flexible HTTP client library, with support for multiple backends.
https://lostisland.github.io/faraday
MIT License
5.73k stars 976 forks source link

Remove `default_uri_parser` config #1487

Open iMacTia opened 1 year ago

iMacTia commented 1 year ago

This was introduced 9 years ago and it's unclear as to why we need it. Most likely, back in the days the standard Kernel#URI was simply not powerful enough, which is arguably not true anymore.

Moreover, by making this configurable without any rule, there's no way to tell what the return type of Utils.URI actually is, causing issues downstream and poor test coverage for edge cases that might result from the use of this configuration.

Read more: #1484

CKolkey commented 1 year ago

Just to add a case for "why we need it", I've run into a use case where Kernel#URI does not correctly handle parsing something that Addressable does.

The path thats getting passed in is "/api/v2/dk/document_version_top_ids/LBKG2006649_§56", which causes URI to raise the following exception due to the § character:

eval error: URI must be ascii only "https://documents.karnov-test.com/api/v2/dk/document_version_top_ids/LBKG2006649_\u00A756"
  /Users/u0158204/.asdf/installs/ruby/3.2.2/lib/ruby/3.2.0/uri/rfc3986_parser.rb:20:in `split'
  /Users/u0158204/.asdf/installs/ruby/3.2.2/lib/ruby/3.2.0/uri/rfc3986_parser.rb:71:in `parse'
  /Users/u0158204/.asdf/installs/ruby/3.2.2/lib/ruby/3.2.0/uri/common.rb:193:in `parse'

However, Addressable handles this just fine, correctly escaping the unicode § and producing the following path:

"/api/v2/dk/document_version_top_ids/LBKG2006649_%C2%A756"

I came across this issue because I was trying to figure out why this exception was being raised at all, since I'm setting Addressable as our URI parser via an initializer. However, I discovered that the specific functionality needed was removed a few months back in #1484.

I attempted to work around this by writing a custom middleware to handle the normalization, but it seems that that is further down the call-stack. For the time being, I've settled on pre-normalizing the path before passing it into Faraday.

Regardless, thanks for your work on a great library :)

iMacTia commented 1 year ago

Thank you for the great feedback @CKolkey, we'll definitely keep this scenario in mind when we get to work on this. This is quite the edge case, so pre-normalising makes total sense, but I agree that in ideal world Faraday should be smart enough to handle this kind of stuff for you 😉

ahmadgandar commented 1 year ago

Perbaiki

Pada tanggal Jum, 14 Apr 2023 20.57, Matt @.***> menulis:

Thank you for the great feedback @CKolkey https://github.com/CKolkey, we'll definitely keep this scenario in mind when we get to work on this. This is quite the edge case, so pre-normalising makes total sense, but I agree that in ideal world Faraday should be smart enough to handle this kind of stuff for you 😉

— Reply to this email directly, view it on GitHub https://github.com/lostisland/faraday/issues/1487#issuecomment-1508563804, or unsubscribe https://github.com/notifications/unsubscribe-auth/A5EEM37DBUP3B7QJ4I45UKLXBFJT7ANCNFSM6AAAAAAUDUJLZU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ksol commented 7 months ago

hello there!

I have a use-case that may not be shared by a majority of users and that is not exactly tied to this issue, but it's a neighbor topic I'd say.

Basically, I wanted to implemented support for URI templates. The idea was being able to do conn.get("/resources/{parent_id}/subresources/{id}{?query*}", parent_id: 1, id: 2). At first I tried to do it using a middleware, but the URI parsing doesn't accept uri templates.

I've since rolled out custom code that takes care of the template expansion ahead of calling conn.get; I wanted to check if supporting URI templates had been discussed before and I found this ticket.

It's possible overriding the default uri parser might have been a viable solution for rolling out my feature and it might have resulted in a better code design (or at least, something that could have been extracted into an open-source middleware)

Also, I can understand that if it's not something that has been requested before, and if the use cases for overriding the URI parser is more a maintenance burden than anything else, dropping it makes sense.

However, if it's not too much of a cost to keep it around, there's still use cases around for it.

Happy to discuss this more if needed, and thanks for a solid library, well designed that is a pleasure to work with!