Make &stringify_keys/1 private. #605

arubis · 2017-01-03T03:21:28Z

What's in this PR?

Define MapUtils::stringify_keys/1 as private (via defp) rather than public. No change of functionality.

Since MapUtils::&keys_to_string/1 is public and calls &stringify_keys, also add a test case for &keys_to_string/1.

References

Fixes #600

joshsmith

Just a couple quick comments here. Thanks for doing this, and especially for adding a new (missing) test!

joshsmith · 2017-01-03T05:16:39Z

lib/code_corps/map_utils.ex

-  def stringify_keys(%DateTime{} = val), do: val
-  def stringify_keys(map = %{}) do
+  defp stringify_keys(nil), do: nil
+  defp stringify_keys(%DateTime{} = val), do: val


I think this may be covered by L20.

It's not. A module struct matches a map, so without this line here, line 13 would match a %DateTime{} and try to recursively stringify those keys as well.

I'm not sure if there's an is_struct guard clause, so maybe we could instead change line 13 to when is_map(map) and not is_struct(map), but without something like that, this has to stay.

@begedin the bigger point is that we should not ever (I think) be hitting a %DateTime{} – moreover, in both cases these functions simply return the value. They are the same function, strictly speaking.

Having poked around in http://erlang.org/doc/man/erlang.html#is_atom-1, there pretty definitively isn't an is_struct and nothing jumps out at me as a good stand-in. I agree with @begedin, this needs to stay here for now.

That said, we do not need L11 (stringify_keys(nil)) because that does get picked up by stringify_keys(val), do: val.

joshsmith · 2017-01-03T05:16:55Z

lib/code_corps/map_utils.ex

+  defp stringify_keys([head | rest]), do: [stringify_keys(head) | stringify_keys(rest)]
  # Default
-  def stringify_keys(not_a_map), do: not_a_map
+  defp stringify_keys(not_a_map), do: not_a_map


Could just be renamed from not_a_map to val.

Indeed, that'll be a LOT clearer.

joshsmith · 2017-01-03T05:17:16Z

test/lib/code_corps/map_utils_test.exs

  use ExUnit.Case, async: true

-  import CodeCorps.MapUtils, only: [rename: 3]
+  import CodeCorps.MapUtils, only: [rename: 3, keys_to_string: 1]


Would you mind reordering in alpha order?

joshsmith · 2017-01-04T02:24:51Z

lib/code_corps/map_utils.ex


  # Goes through a list and stringifies keys of any map member
-  defp stringify_keys(nil), do: nil
  defp stringify_keys(%DateTime{} = val), do: val


I think my point here was misunderstood.

Doing any of the following is the same as doing fn(val), do: val:

fn(%DateTime{} = val), do: val

fn(nil = val), do: val

fn(RandomApp.RandomModule = val), do: val

All of these are equivalent if all we're doing is passing along val.

Just for anyone visiting this thread in the future.

The problem is that

stringify_keys(val) when is_map(val) do

clause needs to go first, in order to match properly against a map at all times.

Our App used to transform large maps which also include DateTime structs. To elixir, a struct is a map, so the code here would try to convert DateTime struct keys recursively, which was causing issues, so we needed a DateTime clause before the map clause.

However, now our code is no longer using DateTime structs within its payloads, so it should be save to remove. We don't need the clause on line 11 anymore.

The clause on line 19 is a duplicate of line 22, so we don't need 19 either.

For discussions sake, I thought about this and I believe we could match against all structs using

defp stringify_keys(%{__struct__: _} = val), do: val

on line 11, but I don't think it's necessary for us to do that right now. I believe it's enough to just remove lines 11 and 19 and then merge as is.

Thanks for the review, guys. @begedin I may revisit this thread for your idea on matching against structs--that's clever!

L19 actually isn't a duplicate of L22--stringify_keys and stringify_key are (somewhat non-obviously) different fns (plural v. singular).

I'll push up a commit dropping L11. LMK if there's further to be done; otherwise I'll rebase against develop, re-push, and move on.

joshsmith · 2017-01-05T05:01:20Z

@arubis the failures are happening because in StripeTesting we define %DateTime{} responses, whereas now stripity_stripe simply sends us Unix timestamp integers. Those will need converted, I think.

arubis · 2017-01-06T01:54:47Z

Gotcha. How liable am I to break something else while converting these? For example, if in test/lib/code_corps/stripe_service/adapters/stripe_connect_subscription_test I swap out {:ok, date} = DateTime.from_unix(1479472835) for just date = 1479472835, that test passes -- but I don't know if I'm defeating any related tests/might break anything for which there isn't coverage.

If that's a decent example of all that needs to happen going forward, it won't be too hard.

joshsmith · 2017-01-06T02:34:27Z

Just commenting for team ref, but we discussed on Slack, so addressed now.

arubis · 2017-01-06T04:15:37Z

Indeed -- see discussion at https://codecorps.slack.com/archives/api/p1483669518000712 if needed.

arubis · 2017-01-08T18:29:57Z

I've found that it's more than just the tests that are still using %DateTime within stripety_stripe; some of this is the implementation. If I drop the %DateTime matcher from map_utils.ex, and then run:

customer = insert(:stripe_platform_customer)
StripePlatformCustomerService.update_from_stripe(customer.id_from_stripe)

it will raise:

** (Protocol.UndefinedError) protocol Enumerable not implemented for #<DateTime(2016-11-18T12:40:35Z Etc/UTC)>

This happens because StripePlatformCustomerService.update_from_stripe/1 is calling MapUtils.keys_to_string/1 with a %DateTime as argument; stringify_keys/ pattern matches the %DateTime as a map, and runs Enum.map against it here:

  # Goes through a list and stringifies keys of any map member
  defp stringify_keys(map = %{}) do
    map
    |> Enum.map(fn {k, v} -> {stringify_key(k), stringify_keys(v)} end)
    |> Enum.into(%{})

My understanding is that the stripety_stripe implementation is, going forward, supposed to use integer (epochal) timestamps, not %DateTime objects. That's outside the scope of this issue, so for now I'm just going continue including the defp stringify_keys(%DateTime{} = val), do: val bypass.

begedin · 2017-01-08T20:38:01Z

@arubis stripity_stripe has already in the 2.0 branch (which we use) switched off of DateTime. The reason we still have issues if you remove that method is because we override stripity_stripe in many of our tests using the code in lib/code_corps/stripe_testing.

Removing the method and replacing any instance of DateTime inside that folder with a simple integer should do the trick. I believe we, in all cases, use something like DateTime.from_unix(x), so replacing that with simply x would work.

That being said, I would agree that it's out of scope of this PR, so I'll approve for merge. I've created issue #623 to continue fixing it there. Feel free to tackle that one if you feel like it.

begedin

Good to go. The new issue discovered in stripe_testing can be tackled with #623

arubis · 2017-01-08T21:13:31Z

Thanks @begedin! I'll probably pick up #623 as well, if nothing else to start wrapping my head around how stripety_stripe works.

joshsmith · 2017-01-10T21:41:47Z

Thanks @arubis! Looks great.

Since &keys_to_string/1 is public, also add a test case to coverage for &stringify_keys/1. Drop useless `stringify_keys(nil)` case. Continue intercepting %DateTime{} before it matches %{}

joshsmith · 2017-01-13T17:46:35Z

🙌

joshsmith reviewed Jan 3, 2017

View reviewed changes

joshsmith added the needs submitter response label Jan 3, 2017

joshsmith modified the milestone: Improve Donations Jan 3, 2017

joshsmith reviewed Jan 4, 2017

View reviewed changes

arubis force-pushed the 600-make-MapUtils__stringify_keys-private branch from 9427dfe to a6b9edb Compare January 7, 2017 17:57

arubis force-pushed the 600-make-MapUtils__stringify_keys-private branch from a6b9edb to 61a482d Compare January 8, 2017 18:31

begedin approved these changes Jan 8, 2017

View reviewed changes

begedin added ready to merge and removed needs submitter response labels Jan 8, 2017

Make &stringify_keys/1 private.

b350ac8

Since &keys_to_string/1 is public, also add a test case to coverage for &stringify_keys/1. Drop useless `stringify_keys(nil)` case. Continue intercepting %DateTime{} before it matches %{}

arubis force-pushed the 600-make-MapUtils__stringify_keys-private branch from 61a482d to b350ac8 Compare January 13, 2017 02:48

joshsmith merged commit fea7d7b into code-corps:develop Jan 13, 2017

Make &stringify_keys/1 private. #605

Make &stringify_keys/1 private. #605

Uh oh!

Conversation

arubis commented Jan 3, 2017

What's in this PR?

References

Uh oh!

joshsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshsmith commented Jan 5, 2017

Uh oh!

arubis commented Jan 6, 2017

Uh oh!

joshsmith commented Jan 6, 2017

Uh oh!

arubis commented Jan 6, 2017

Uh oh!

arubis commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

begedin commented Jan 8, 2017

Uh oh!

begedin left a comment

Choose a reason for hiding this comment

Uh oh!

arubis commented Jan 8, 2017

Uh oh!

joshsmith commented Jan 10, 2017

Uh oh!

joshsmith commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

begedin Jan 4, 2017 •

edited

Loading

arubis commented Jan 8, 2017 •

edited

Loading