inaka / Dayron

A repository `similar` to Ecto.Repo that maps to an underlying http client, sending requests to an external rest api instead of a database
http://inaka.net/blog/2016/05/24/introducing-dayron/
Apache License 2.0
159 stars 20 forks source link

Create a Tesla Adapter #54

Open flaviogranero opened 8 years ago

flaviogranero commented 8 years ago

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

jwarlander commented 8 years ago

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

jwarlander commented 8 years ago

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?

tiagoengel commented 8 years ago

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.

jwarlander commented 8 years ago

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.

tiagoengel commented 8 years ago

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.

jwarlander commented 8 years ago

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?

tiagoengel commented 8 years ago

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.

jwarlander commented 8 years ago

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