Remove response mappings #165

SViccari · 2016-12-19T06:14:16Z

Why:
This PR was created after the discussion I had with @joshsmith around existing PR #153 and how we want to avoid curating a response_mapping list and defining modules that aren't associated with a Stripe resource (ex: Address).

TO-DOs:

Squash commits
Ensure untested areas are not broken

This PR:

removes the use of a response_map
uses a struct's keys to determine which keys are included/excluded in the Struct (aka, if a key in the Stripe response isn't listed within defstruct, the key will not be ignored when converted to a Struct)
adds unit test coverage
extracts the conversion of a Stripe response to a struct to it's own module

Concerns:
Being new to this code base, there's a strong chance I've misinterpreted what's necessary and the end goal . Also, given the lack of test coverage, I'd appreciate help/advice in ensuring untested areas are still sound.

Side-Effects:
This PR introduces the notion of a relationships map. This is a simplification of what used to be the response_mapping. Ideally, I'd like to remove the necessity of singling out particular keys and mapping them to their proper struct but given the example created: DateTime, we can't always rely on the object type that's returned in the Stripe API response.

- added test coverage - placed relationship information at the individual module level - added a test for when a relationship key is not in the response map because otherwise it produces a particularly awful, vague error

…ide a conditional

coveralls · 2016-12-19T06:15:33Z

Coverage increased (+8.2%) to 18.593% when pulling bbbc92d on remove-response-mappings into df6c770 on 2.0.

DavidAntaramian · 2016-12-19T16:21:08Z

Just glanced over this briefly, but based on what I see , it's amazing 🎉 This really simplifies the conversion logic.

Faolain · 2016-12-19T17:04:34Z

Also quickly looked over this, amazing work @SViccari

SViccari · 2016-12-19T17:11:30Z

lib/stripe/converter.ex

+
+  defp convert_value(value) when is_map(value) do
+    Enum.reduce(value, %{}, fn({key, value}, acc) ->
+      Map.put(acc, String.to_atom(key), convert_value(value))


It's a bit unfortunate that String.to_atom(key) has to be used here but it is necessary as string keys are ignored when creating a Struct.

Since this should always be going to a key in a struct, presumably String.to_existing_atom/1 should be safe, right?

Unfortunately, that's not necessarily true :( . A good example is legal_entity on Account (which hasn't been added yet but I believe we would like to add it.) Here's a small example for the docs:

"legal_entity": { "address": { "city": null, "country": "US", "line1": null, "line2": null, "postal_code": null, "state": null } }

If I use to_existing_atom, all the keys listed within legal_entity would need to be added to the Account's defstruct. Right now, we can just add legal_entity to Account and all nested keys will still be included in the Struct.

Ah! I see what you mean.

Two ideas:

I'm wondering if you could break it out so that top-level definitions are String.to_existing_atom/1 and lower levels are taken without restriction (I believe that's the same logic, but with added complexity in the conversion implementation).

Is there a way to invert the conversion such that the input atom is converted to a string instead of vice-versa? Then you can just use the original input atom for the Map.put/3.

In theory the algorithm you designed is restricted enough that a user would have to intentionally misuse it (at great length, no less) to cause an atom table overflow, but I still prefer to be safer with atom conversions if we can.

@SViccari @DavidAntaramian how do we feel about addressing this in a follow-up refactoring issue? I'm comfortable merging as-is and circling back if you are.

@DavidAntaramian I'm afraid I don't quite follow idea #2. Perhaps we can find a time to chat on Slack?

As for idea #1, I believe what you're describing is what is currently happening as all "top-level definitions" are being derived from [_|struct_keys] = Map.keys(module.__struct__). The only time string to atom conversion is occurring is when convert_value encounters low-level/nested maps.

I'd still love to chat about this when you have the time! I'm also okay with merging this PR and issuing another PR with any improvements our conversation uncovers :)

@SViccari I think merging this and then possibly refactoring later would be the best plan. Will send a message on Slack when I get the chance. It's possible that I'm missing a key piece that makes my idea infeasible.

joshsmith · 2016-12-19T18:42:09Z

lib/stripe/plan.ex

-    statement_descriptor: :string,
-    trial_period_days: :integer
+  @relationships %{
+    created: DateTime
  }


I'm a little uncomfortable with the naming of relationships in the cases where we're only specifying DateTime for some fields. Can't think of a better name, though, so this is a rather empty critique.

That's a very good point, relationships isn't a great name for "keys that qualify to be converted into a struct". I'm not sure of a better name at the moment but I'll see if I can come up with one.

Is there any way for us to determine whether a unix timestamp is a timestamp by pattern matching instead? @DavidAntaramian?

Hm, I can't think of way to distinguish an integer that is not a timestamp from an integer that does represent a timestamp.

Ideally, I'd like to remove the use of relationships from the modules as the knowledge of "keys that qualify as structs" isn't something the modules should manage. One should not create a new module and be required to define relationships on it. Earlier in this PR, I started down the path of removing this mapping entirely but then decided that would be best done in a separate PR.

I think we could achieve this by creating a lib/defined_struct_keys.ex file that is the sole source of which data keys are mapped to a struct. OR, it could be done by using the object value in the Stripe response information in combination with pattern matching functions that target the keys created, trial_end, trial_start etc.

I would recommend the DefinedStructKeys approach as I find it more obvious and it's easy maintain.

@joshsmith In direct answer to your question, no, there is no way to determine whether an integer is a valid UNIX timestamp since any integer is conceivably a valid UNIX timestamp.

I wish that Stripe had some sort of discoverable schema we could make use of, but unfortunately that's not the case. I think @SViccari's proposes a good middle-ground in this case. Either approach is good with me, but if I were personally implementing this I would actually choose to pattern match on the key name. It's less obvious, I agree, but I think it's easier to maintain in the long run. Though only time will tell on the second part 😄

sbatson5

This looks great 👍 !

sbatson5 · 2016-12-19T19:40:30Z

lib/stripe/converter.ex

+          fetch_value(response, key)
+          |> convert_value()
+
+        value = Map.get(module.relationships, key)


Above we newline on value but here we don't. Thoughts on newlining here as well for consistency?

Also, what are your thoughts on piping everything?

value = module.relationships |> Map.get(key) |> build_struct(value)

I like it! You also reminded me that build_struct should really be maybe_build_struct as there are two cases where build_struct does not return a struct. I'll update in a bit.

sbatson5 · 2016-12-19T19:41:24Z

lib/stripe/converter.ex

+      Map.put(acc, String.to_atom(key), convert_value(value))
+    end)
+  end
+  defp convert_value(value), do: value


This whole block is really clever! 👏 It must have been tedious to figure out. Nice work!

Yes, yes it was 😬 I'm afraid it's more clever than I like as there is still sooo much happening but I think this is a good amount of change for one PR :)

sbatson5 · 2016-12-19T19:51:02Z

test/stripe/converter_test.exs

+  defmodule PersonWithMissingRelationship do
+    defstruct [:card]
+    def relationships, do: %{card: Stripe.ConverterTest.CreditCard}
+  end


I've never seen defining modules with structs like this in a test. This is handy to know! 👍

sbatson5 · 2016-12-19T19:58:13Z

test/stripe/converter_test.exs

+      }
+    }
+
+    result = Converter.stripe_map_to_struct(Stripe.ConverterTest.Person, json_response)


Thoughts on aliasing Stripe.ConverterTest.Person? It may make this a bit easier to read through.

sbatson5 · 2016-12-19T19:59:05Z

test/stripe/converter_test.exs

+    def relationships, do: %{}
+  end
+
+  defmodule PersonWithMissingRelationship do


PersonWithMissingRelationship sounds depressing 😞

This is actually an amazing module and should never change. If ever needed, I am willing to constantly rearchitect everything around this, no matter how tedious or unnecessary.

sbatson5 · 2016-12-19T20:13:03Z

test/stripe/converter_test.exs

+  test "converts a Stripe response into a struct" do
+    expected_result = %Stripe.ConverterTest.Person{
+      first_name: "Leslie",
+      last_name: "Knope",


coveralls · 2016-12-20T05:17:24Z

Coverage increased (+8.08%) to 18.5% when pulling 85889d3 on remove-response-mappings into df6c770 on 2.0.

DavidAntaramian

This seems to be feature-sound to me. I suggest we merge this and any further refactoring opportunities we identify can be ticketed and then addressed at a future point in time.

joshsmith · 2016-12-20T07:37:04Z

@SViccari do you mind opening a separate issue as you had suggested? Wanted this merged so we could work from it.

SViccari added 5 commits December 19, 2016 00:33

extracted stripe_map_to_struct to a new module

1c61d84

- added test coverage - placed relationship information at the individual module level - added a test for when a relationship key is not in the response map because otherwise it produces a particularly awful, vague error

address unused alias warning

4f77771

specify relationships rather than all attributes and type

d497927

resolves the warning: variable value is unsafe as it has been set ins…

6ed9015

…ide a conditional

remove unused functions

bbbc92d

SViccari mentioned this pull request Dec 19, 2016

[v2] Subscription Update -> Plan Same ID as Subscription? #162

Closed

SViccari commented Dec 19, 2016

View reviewed changes

joshsmith reviewed Dec 19, 2016

View reviewed changes

sbatson5 reviewed Dec 19, 2016

View reviewed changes

address PR feedback

85889d3

DavidAntaramian approved these changes Dec 20, 2016

View reviewed changes

joshsmith merged commit 539ea72 into 2.0 Dec 20, 2016

joshsmith deleted the remove-response-mappings branch December 20, 2016 07:36

begedin mentioned this pull request Dec 20, 2016

Add Account.create #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove response mappings #165

Remove response mappings #165

SViccari commented Dec 19, 2016 •

edited

coveralls commented Dec 19, 2016

DavidAntaramian commented Dec 19, 2016

Faolain commented Dec 19, 2016

SViccari Dec 19, 2016

DavidAntaramian Dec 19, 2016

SViccari Dec 19, 2016 •

edited

DavidAntaramian Dec 19, 2016

joshsmith Dec 19, 2016

SViccari Dec 20, 2016 •

edited

DavidAntaramian Dec 20, 2016

joshsmith Dec 19, 2016

SViccari Dec 19, 2016

joshsmith Dec 20, 2016

SViccari Dec 20, 2016 •

edited

DavidAntaramian Dec 20, 2016

sbatson5 left a comment

sbatson5 Dec 19, 2016

SViccari Dec 19, 2016 •

edited

SViccari Dec 20, 2016

sbatson5 Dec 19, 2016

SViccari Dec 19, 2016

sbatson5 Dec 19, 2016

sbatson5 Dec 19, 2016

sbatson5 Dec 19, 2016

SViccari Dec 19, 2016

joshsmith Dec 20, 2016

sbatson5 Dec 19, 2016

coveralls commented Dec 20, 2016

DavidAntaramian left a comment

joshsmith commented Dec 20, 2016

Remove response mappings #165

Remove response mappings #165

Conversation

SViccari commented Dec 19, 2016 • edited

coveralls commented Dec 19, 2016

DavidAntaramian commented Dec 19, 2016

Faolain commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SViccari Dec 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SViccari Dec 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SViccari Dec 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbatson5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SViccari Dec 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 20, 2016

DavidAntaramian left a comment

Choose a reason for hiding this comment

joshsmith commented Dec 20, 2016

SViccari commented Dec 19, 2016 •

edited

SViccari Dec 19, 2016 •

edited

SViccari Dec 20, 2016 •

edited

SViccari Dec 20, 2016 •

edited

SViccari Dec 19, 2016 •

edited