SparkPost / elixir-sparkpost

SparkPost client library for Elixir https://developers.sparkpost.com
Apache License 2.0
44 stars 14 forks source link

Decoding HTTP Responses with Strings vs. Atoms #14

Open DavidAntaramian opened 8 years ago

DavidAntaramian commented 8 years ago

Is there a specific reason why the body is being decoded with keys as atoms?

  # lib/endpoint.ex:85-89@30402ea
  defp decode_response_body(body) do
    # TODO: [key: :atoms] is unsafe for open-ended structures such as
    # metadata and substitution_data
    body |> Poison.decode!([keys: :atoms])
  end

While this can be convenient, and definitely draws in Ruby's preference of symbols over strings, I feel that it unnecessarily grows the atom-space in BEAM which should be left to the determination of the end-programmer if the atom itself is not declared in-code.

ewandennis commented 8 years ago

Its a good point. The reasoning for using atoms was readability - struct fields tend to pervade client code so there's a good amount of re-use of the atom keys here.

In this particular case, the keys returned in a SparkPost API response are a bounded smallish set except for 2 cases: transmission substitution_data and metadata keys. That comment is about keeping those 2 situations under control.

I wonder whether exposing string/atom decoding as an option might be worthwhile? It'd require all field use in the client lib to be string/atom agnostic which sounds non-trivial.

DavidAntaramian commented 8 years ago

@ewandennis I agree with the fact that most of the atoms should be being re-used.

There are three ways I can think of accomplishing this:

1.) As an option, as you suggested. 2.) Allowing the caller to handle transformations before moving forward. 3.) As a specific whitelist of endpoints to atom-ify using pattern matching

Not looking for an immediate decision to be made, but I did want to see it in the issue tracker so that anyone who does run into this issue has somewhere centralized to discuss