Dayron icon indicating copy to clipboard operation
Dayron copied to clipboard

Create a Tesla Adapter

Open flaviogranero opened this issue 8 years ago • 8 comments

Tesla is a new Elixir Http Client with middleware support, similar to ruby faraday.

https://medium.com/@teamon/introducing-tesla-the-flexible-http-client-for-elixir-95b699656d88#.kfhd11vjv

flaviogranero avatar Sep 07 '16 13:09 flaviogranero

Started on this.. I'll submit a pull request to track progress, but it's far from done yet :)

jwarlander avatar Oct 04 '16 21:10 jwarlander

When doing the initial work on #57, I noted that in the HTTPoison adapter, map keys from the JSON decoding are converted to atoms:

    defp process_decoded_body(body) do
      body
      |> Enum.into(%{})
      |> Crutches.Map.dkeys_update(fn (key) -> String.to_atom(key) end)
    end

I'll implement the same in the Tesla adapter of course, but it seems a bit unsafe since at some point one risks filling up the atom table, especially if there are weird responses from the API being queries. Is it something that would be difficult to redesign later on?

jwarlander avatar Oct 05 '16 06:10 jwarlander

I think this is a very important point @jwarlander. Poison if I recall corectly have two options keys: :atom and keys: :atom!. The first tries to use only atoms that already exists and fail when they don't, and the second one is unsafe and tries to convert everything to an atom.

Atoms by design are made for fast comparison, so they are put in a table (that map them to integers I think) and they are also not garbage collected, this is why is considered dangerous to convert an arbitrary list of keys to atoms like that.

I think the Dayron case is simple to solve, since we are only interested on the keys that are in the schema (and those atoms already exist) we can just filter by the keys that are in the schema and convert only this keys to atoms.

We can also leave the atom conversion to the model's __from_json__ function.

tiagoengel avatar Oct 05 '16 11:10 tiagoengel

Interestingly, for Poison, it seems to be the other way around - keys: :atoms will convert everything to an atom, while keys: :atoms! will convert only to existing atoms (raising on anything else).

The least intrusive change would probably be to just make sure to filter the decoded map, and use String.to_existing_atom.

jwarlander avatar Oct 06 '16 07:10 jwarlander

for Poison, it seems to be the other way around - keys: :atoms will convert everything to an atom, while keys: :atoms! will convert only to existing atoms (raising on anything else).

Yes, you're right. Both changes are potential break changes, filtering the keys in the adapter will break code that rely in fields that are not in the schema, for instance:

def __from_json__(data, _opts) do
  full_name = "#{data[:first_name]} #{data[:last_name]}"
  ....
end

I think this should be planned for a new release that will contain break changes.

tiagoengel avatar Oct 06 '16 11:10 tiagoengel

A question here.. I think the Tesla adapter is feature complete except for streaming. Given that :stream_to in the HTTPoison adapter is just passed on to HTTPoison currently, without any translation of the resulting HTTPoison-specific messages back to the receiving process, I'm not sure it's a good idea to support it for Tesla as-is, since it would require both Tesla and HTTPoison (to create the proper structs). What do you guys think?

jwarlander avatar Oct 14 '16 05:10 jwarlander

I agree with you, even for the HTTPoison adapter this will not work very well right now because the response is not handled by the adapter which basically removes the advantage of using Dayron.

It may be good to add a section in the readme (or a wiki page) explaining this.

tiagoengel avatar Oct 17 '16 10:10 tiagoengel

I've fixed test coverage, and update the README with info about the Tesla adapter + the streaming issue. Anything else needed? :)

jwarlander avatar Oct 25 '16 10:10 jwarlander