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

Remove default_uri_parser config #1487

Open
iMacTia opened this issue Jan 23, 2023 · 4 comments
Open

Remove default_uri_parser config #1487

iMacTia opened this issue Jan 23, 2023 · 4 comments

Comments

@iMacTia
Copy link
Member

iMacTia commented Jan 23, 2023

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
Copy link

CKolkey commented Apr 14, 2023

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
Copy link
Member Author

iMacTia commented Apr 14, 2023

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

This comment was marked as off-topic.

@ksol
Copy link

ksol commented Feb 17, 2024

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!

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