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

Customizable querystring encoder #122

Closed
christhekeele opened this issue Oct 13, 2017 · 7 comments
Closed

Customizable querystring encoder #122

christhekeele opened this issue Oct 13, 2017 · 7 comments

Comments

@christhekeele
Copy link

christhekeele commented Oct 13, 2017

Hey! I'm wrapping a Bad API™ that needs raw +s in its querystring.

Currently I can't prevent Tesla from doing the sane thing and building a valid URI in Tesla.build_url/2 that follows the URI specification, subbing my plusses out with %2B. Nor are there options to configure this at the stdlib URI.encode_query/1 level that Tesla calls out to. This is a just way to handle the uncertainty and darkness of a world gone mad and I have no objection.

Rather than laboring to support stupid crap like this, though, would you be open to making it possible to override the query (or url) encoder for a completely bespoke one, so those of us that must deal with these aberrant APIs needn't write our own Tesla from scratch?

I'm happy to contribute to the implementation of this, just wanted to sound it out first.

(For now I've copy-pasted the hackney adapter wholesale that gsubs the plusses back in.)

@teamon
Copy link
Member

teamon commented Oct 13, 2017

I feel you!

Right now it's not possible, since Tesla.build_url is used directly in every adapter..

BUT

We can easily change that.

If we move Tesla.build_url call to Normalize middleware and just use env.url in adapters then we can inject another "post" middleware with dynamic client that would replace %2B back to +.

How does that sound?

@christhekeele
Copy link
Author

christhekeele commented Oct 13, 2017

That would work for my purposes, but I think that would be a backwards incompatible change, no?

Suddenly, all post middlewares are receiving a %Tesla.Env{}with the env.query already applied to the env.url, right? Also any changes they make to the env.query will not be reflected. Or am I misunderstanding the flow of control in the source code?

One way we could avoid this would be to add a new property to the env, an env.uri_encoder function, that Normalize could set to &Tesla.build_url/2, but allow to be overriden in post middleware.

Then all the adapters would need to do is env.uri_encoder(env.url, env.query) instead of Tesla.build_url(env.url, env.query).

On the other hand this does extend the core data-structure of Tesla, which might be overkill for the problem it solves. What do you think?

@teamon
Copy link
Member

teamon commented Oct 18, 2017

You're right.

But still, you can achieve it like this:

Default behaviour

defmodule Default do
  use Tesla

  def go do
    get("https://requestb.in/1itig5a1?x=1+2", query: [default: "1+2"])
  end
end

produces

2017-10-18 at 12 03

As you can see the "url" part is not encoded, so that's one way to hack it (not the most elegant obviously)

Override adapter

defmodule Hack do
  use Tesla

  def go do
    get(client(), "https://requestb.in/1itig5a1", query: [hack: "1+2"])
  end

  def client do
    # use Tesla.build_adapter to override the default adapter
    # and then use Tesla.run_default_adapter after modifying the env
    Tesla.build_adapter fn env ->
      env = %{env | url: build_url(env), query: []}
      Tesla.run_default_adapter(env)
    end
  end

  defp build_url(env) do
    # use standard Tesla.build_url but replace %2B with + after encoding
    env.url
    |> Tesla.build_url(env.query)
    |> String.replace("%2B", "+")
  end
end

and this will produce:

2017-10-18 at 12 02

If you want to use some specific adapter you can use it directly with Tesla.Adapter.Hackney.call(env, [])

@teamon
Copy link
Member

teamon commented Oct 18, 2017

Fun fact - I've forgotten about module-local adapters :D

defmodule Hack do
  use Tesla

  adapter :adapter

  def go do
    get("https://requestb.in/1itig5a1", query: [hack: "1+2"])
  end

  def adapter(env) do
    url =
      env.url
      |> Tesla.build_url(env.query)
      |> String.replace("%2B", "+")

    env = %{env | url: url, query: []}
    Tesla.run_default_adapter(env)
  end
end

@christhekeele
Copy link
Author

christhekeele commented Oct 31, 2017

That module-local bit is pretty slick, I went with that for a while.

Then, I discovered a place in the same API (😱) where I do need normally encoded +s (inside the search endpoint, plusses are a pretty standard part of search terms in this domain), so I had to revert to my copy-paste of the hackney adapter that specifically re-implements Tesla's private querystring encoder methods:

  • encode_pair now handles normal key/values pairs by joining them with a place-holder (U+271A: heavy greek cross) character.
  • encode_query finishes with the extra step of gsubbing the encoded version of the place-holder into a normal plus (String.replace("%E2%9C%9A", "+")).

So, this would still be a nice-to-have feature for me and my absurd edge case. I can only pray that I don't find a corner of this API where the heavy greek cross has special significance. ;~;

@teamon
Copy link
Member

teamon commented Nov 28, 2017

I think you can still achieve that with encoding query into url before passing it it run_default_adapter without the need to copy-paste-edit whole adapter.

@teamon
Copy link
Member

teamon commented Mar 5, 2018

Closing this one, @christhekeele feel free to reopen if you have anything to add here.

@teamon teamon closed this as completed Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants