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

Decoding HTTP Responses with Strings vs. Atoms #14

Open
DavidAntaramian opened this issue Feb 29, 2016 · 2 comments
Open

Decoding HTTP Responses with Strings vs. Atoms #14

DavidAntaramian opened this issue Feb 29, 2016 · 2 comments
Assignees

Comments

@DavidAntaramian
Copy link
Contributor

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

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

@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

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