RFC - Stripity Stripe 2.0

[dmvt]

Start Date: 2016-10-19

Summary

Stripity Stripe is in need of an upgrade to provide a more flexible Elixir wrapper for the amazing Stripe API. There are two main goals of this rewrite. First, Stripity Stripe 2.0 should have a consistent and easy to understand high level API. Given that Elixir was born of Ruby, we will seek to closely match the documented Ruby Interface. Second, there should be full test coverage which does not rely on recorded responses of a specific version of the API. We want to be testing our functionality, not that of the Stripe API itself.

Motivation

The initial desire for 2.0 came out of a conversation between Dan Matthews of Strumber and Josh Smith of Code Corps. Both teams were/are implementing on top of Stripe's Connect functionality and found the existing version's endpoint modules to be generally lacking best practice support for the Stripe-Account header. Additionally, we discovered unnecessary dependencies, unsupported endpoints, inconsistent variable names and a good deal of repetition.

Rob Conery was gracious enough to hand us the reins several days ago.

Detailed design

HTTPoison will be replaced with Hackney at the suggestion of @DavidAntaramian in Issue #100. Along this thought line, dependencies should be generally avoided unless absolutely necessary. Whenever possible, the lowest level dependency should be used.

Stripe provides a common endpoint structure for each primary endpoint. We will attempt to handle the majority of them through the use of common modules supplemented by heavy documentation. An example of this is as follows:

defmodule Stripe.Customers do
  @moduledoc """
  High Level API for working with Stripe Customers.

  Through this API you can:
  -Create a customer [ref](https://stripe.com/docs/api#create_customer)
  -Retrieve a customer [ref](https://stripe.com/docs/api#retrieve_customer)
  -Update a customer [ref](https://stripe.com/docs/api#update_customer)
  -Delete a customer [ref](https://stripe.com/docs/api#delete_customer)
  -List all customers [ref](https://stripe.com/docs/api#list_customers)
  """

  @endpoint "/customers"

  @doc """
  Creates a Customer. Valid attributes can be found in the
  [Stripe API Customer Docs](https://stripe.com/docs/api#create_customer).
  At the time of this writing, all were optional.

  [ref](https://stripe.com/docs/api#create_customer)

  ## Example
  `` `
    attributes = %{
      email: "dan@strumber.com",
      description: "Customer for dan@strumber.com",
      source: "tok_18pPRnBMvBFPIRGmeLiOEwlm"
    }

    {:ok, customer} = Stripe.Customers.create(attributes)
  `` `
  """

  use Stripe.API.Create

  @doc """
  Retrieves a Customer by ID.

  [ref](https://stripe.com/docs/api/ruby#retrieve_customer)

  ## Example
  `` `
  {:ok, customer} = Stripe.Customers.retrieve("cus_9McRYFr9fAGdKm")
  # or
  {:ok, customer} = Stripe.Customers.retrieve(%{id: "cus_9McRYFr9fAGdKm"})
  `` `
  """

  use Stripe.API.Retrieve

  # ...
end  

We will also only provide functions documented in the official Stripe API libraries. No additional functions are allowed directly on the High Level API modules.

Some endpoints have dependency ids in their URLs. These values will be extracted from the attributes map.

defmodule Stripe.Card do
  @endpoint "/customers/{customer_id}/sources"

  # ...
end

attributes = %{
  id: "card_196RVbBMvBFPIRGmREqyLEUh",
  customer_id: "cus_9McRYFr9fAGdKm"
}

{:ok, customer} = Stripe.Card.retrieve(attributes)

All public functions on High Level API modules have an arity of 4, the last 3 of which are optional.

def create(
  attributes,
  headers \\ %{},
  hackney_options \\ %{},
  secret_key \\ ""
) do
  # ...
end

Drawbacks

There is next to no backwards compatibility with Stripity Stripe Version 1. We're singularizing nearly every module name (Stripe.Customers becomes Stripe.Customer), leaving out (moving?) courtesy functions such as all and changing the arity of almost every function that is kept.

We're also intentionally treating fewer dependancies and DRYness as higher priority concerns than code readability when we choose to use Erlang modules such as Hackney directly, abandoning the syntactic sugar and familiarity of their Elixir wrappers. This choice may deter wider contributions in those areas of the code.

Unresolved questions

Should we provide some or all of the non-standard functions from version 1 in a Stripe.Utils module? The main one I personally find useful is the all function which iterates over the list endpoint.

[joshsmith]

A few comments:

• I like the idea of singularization.
• I also like being able to override the headers. Similarly would like for people to be able to modify the API version if needed, but ideally have a default version we're hitting to match with Stripe's API changes.
• Would like to consider a different approach to use, like a module with some centralized functions for the HTTP verbs.
• Absolutely want to keep the convenience methods of the various top-level API modules. Much of what drew me to stripity-stripe involved the convenience of not having to hit the API manually myself with a lot of verbosity.
• Function signatures with parameters for required attributes on Stripe, combined with optional lists, would seem to provide convenience without the overhead placed on developers to work with attribute maps instead.
• Would like to consider early on how we're going to handle mocking and testing and expose these functions or helpers to developers, so that they do not have to reinvent the wheel when doing their own testing.

Generally I would say I am not very afraid of high churn in Stripe's API. This year there have been 8 changes, almost all fairly trivial. API version changes can be tacked pretty well to semantic versioning ranges in this package. And I have vastly more confidence in the ability of a community to respond to any future Stripe API changes than I am in my own ability to respond to those changes personally via a super low-level library.

I think some of the above comments have significant impact on your proposed design. Obviously happy to discuss further in the course of this issue.

[DavidAntaramian]

I'll ask the same question I did over in #92, with these changes what benefit does this library offer over just using an HTTP client directly? I mean this honestly and not facetiously. I've been working with Elixir for a while now, and I generally discourage people from using libraries that are only lightweight API wrappers in the same way that I recommended not using HTTPoison in favor of using Hackney directly. It creates additional dependency levels that may be better satisfied through in-house code that requires the same level of maintenance.

I think what you're suggesting with "a consistent and easy to understand high level API" is important, and that should be a central focus of the change. At the same time, I strongly disagree with your comment "Given that Elixir was born of Ruby, we will seek to closely match the documented Ruby Interface." Elixir takes inspiration from Ruby, but seeking to match implementations in Ruby to implementations in Elixir will can afoul of paradigmatic differences.

Also, I have to respond to this:

We're also intentionally treating fewer dependancies and DRYness as higher priority concerns than code readability when we choose to use Erlang modules such as Hackney directly, abandoning the syntactic sugar and familiarity of their Elixir wrappers. This choice may deter wider contributions in those areas of the code.

First, this in no way should impact code readability, and code readability should never be sacrificed for that reason. You are not losing any of the value of Elixir by calling an Erlang library. You are not losing any syntatic sugar. You are losing the HTTPoison response wrapper, and instead you have a standardized tuple that you can match on instead. I've replaced HTTPoison with Hackney a few times. I've done more than my fair share of Hackney tuning. Nothing is being lost except a level of dependency resolution.

[joshsmith]

I can see the logic in taking stripe-ruby’s Stripe::APIOperations and reimplementing here as a good basis for the rest of the package. But I do agree that attempting 1:1 mapping is probably not advisable, which was certainly a good lesson learned as we transitioned from our Rails API to Elixir and Phoenix.

[joshsmith]

I would also agree with the reasoning around Hackney vs HTTPoison. There are good reasons to drop down into Erlang and this seems like one of them.

[DavidAntaramian]

I mean, I hesitate to call it "dropping down." The module name is just written differently using atom notation. It's still Elixir.

[joshsmith]

I'm very glad that this seems to be the central takeaway from the conversation so far:

2.0 should have a consistent and easy to understand high level API

What this looks like, exactly, I think is the only point of contention that exists here.

@dmvt I'm hoping there's not significant disagreement with the points I had raised above. I know you want to avoid a heavy API here but don't think we're at odds on that. In the final analysis, really just hopeful that we're not envisioning two entirely distinct packages.

Given that our goals are flexibility, consistency, testability, and ease of use, I don't think we actually do diverge here in a sense that necessitates forking paths. Disagreeing with modeling this on the Ruby implementation I think hardly qualifies as fundamentally divergent.

[begedin]

Overall, I share most of the sentiment with @DavidAntaramian and @joshsmith

Using the ruby implementation as a sort of a baseline feature goal is good, but I wouldn't insist on a 1-to-1 mapping. It can easily lead to trouble, and push as away from using the advantages elixir offers.

I'm OK with the idea of adding some syntactic sugar, but I get the motivation behind not wanting it (yet), so I would strive for 2.0 to get the actual, base functionality done. Maybe a good compromise would be to list what the current library has and decide which of those features are useful to keep around. We might be able to trim it down some, while still keeping most of the usefulness.

For example, the library offers Customer.delete_all, which actually does a Customer.list, followed by calling a Customer.delete for each item in the list. How often is something like that actually used? (I'm actually asking, not a rhetorical question :))

I don't really have much to add. I share the sentiments. I'm also not sure about low-level vs high-level API in that I'm not 100% on what that actually means.

[travisdmathis]

@begedin i agree with you there on delete_all, pretty much never. I'm always for keeping wrappers specifically in line with the API and nothing more. Anytime I build a wrapper I almost never include utility functions myself. I say almost never, because I'm sure there are exceptions, I guess I just haven't found one yet.

If you need a Utility function, write it in house. That's pretty much how I see it. I would like to see us keep it with its intended core functionality.

[DavidAntaramian]

@begedin With regards to low-level vs high-level API, typically a high-level has greater organization of functions that correspond directly to the resources they operate on instead of being a direct pass-through to the HTTP API. For example, Stripe.Customer.create() instead of Stripe.API.request(). In general, low-level API packages provide only necessary plumbing to interface with the remote API like format encoding and required header declaration. Higher level APIs are tailored to have a conceptual plumbing.

Thankfully, Elixir doesn't force any choice. It's traditional for Elixir packages to provide a high-level API for developer convenience while still exposing their low-level API in case the package doesn't directly offer what the developer needs.

[dmvt]

@joshsmith

• The API version used is defined in the Stripe account settings for each individual account. I've never come across a way to specify this when making the call, but if you can find one, let's definitely expose it.
• I'm up for an approach other than use, although do think we should have at least one line of code anchoring each piece of documentation. Perhaps something like import Stripe.API followed by

def create(
  attributes,
  headers \\ %{},
  hackney_options \\ %{},
  secret_key \\ ""
), do: api_create

• Can you elaborate on the pros and cons of using lists instead of maps from a developer's point of view? I chose maps because the reading I've done has indicated that they're the go to key value store in Elixir. Doesn't seem like there would be substantial numbers of reads or writes so performance didn't seem like a big concern to me.
• I totally agree on the need for further specificity around how we'll handle the testing and mocks. Open to suggestions here...

@DavidAntaramian

• Regarding the benefit of having this at all, I do think it's mostly syntactic sugar. Speaking only for myself, I'm in favor of using even simple abstractions that make it easier for junior developers to read and reason about a piece of code and feel that this proposal accomplishes that goal.
• I also agree that when Elixir paradigms conflict with the approach taken by the official Ruby gem, we should diverge. That comment was more of a contextual one than a hard and fast rule. If everyone agrees that it's confusing, I'll strike it.
• I'd love if you'd take on the Hackney replacement. I 100% agree with your reasoning around this change and don't feel that any potential negatives tip the scales at all. I look forward to learning from you in this regard. The drawback is listed because looking at these docs and these docs, one of them is much easier to read to my eyes.

@joshsmith, @begedin - I didn't mean to suggest that we should map our functions one to one with the methods in the Ruby gem, but rather that we should map the functions in the high level API modules one to one with the endpoints that stripe provides. This is mostly because I'd like to maintain feel and interface consistency with the official libs. One of the things that really bugs me about version 1 is the use of get instead of retrieve, etc etc. We can easily accomplish the goals of the extra methods (delete_all, all) by providing an external utility module. In general, I'm willing to be overruled by consensus option and do not feel like any of the comments above represent a significant divergence from my personal goals.

@DavidAntaramian - I couldn't agree more with your final statement. Elixir gives us the ability to easily provide both, and I think we should. I may have misspoken when originally talking to @joshsmith. What I was driving at with low level vs high level is the distinction between the core functions that are represented in the official documentation and what we have taken to calling the utility functions which wrap or iterate over those core functions. By my way of thinking, we don't need to provide those at all, but, as I said above, I'm willing to be overruled.

Thanks for the thoughtful comments so far everyone. I'll be back on and update the text tonight to account for all of these and any more that come along.

[DavidAntaramian]

Hackney Replacement // Low-Level API

Totally happy to handle the Hackney replacement. The documentation on Hackney is certainly not revolutionary, but the modular documentation (e.g., :hackney.request/5) is at least a bit more helpful than the README. I'm hoping the OTP team gets their act together with improving the Erlang documentation tooling.

If you're up for it, I'd like to coordinate the Hackney replacement as part of the low level API system. I typically start by implementing a request/5 function in a core module that mimics the HTTPoison/hackney request/5 function, but pulls in standardized values from the application configuration and handles results in a helpful manner. This function is then called by the higher-level API which knows about its own data domain but only needs to concentrate on that.

Depending on the needs, I expand this to be multiple functions that concentrate on specific contexts. For example, if an API has multiple ways of authenticating depending on the context, each gets its own, properly named request function. I haven't used the Connect system before, so you and Josh may need to point me to anything that is different form the standard API documentation.

Lists (as optional parameter data types)

Regarding optional lists, this is a standard convention in Elixir for passing optional parameters at the end of a function. If everything was ideal it would be maps, but precedent has been set, largely for interoperability and principle-of-least surprise with the Erlang standard library. I believe it's also because keyword lists can be written without [] notation: key: value will become [key: value] which is shorthand for [{:key, value}]. However, since they're lists they can't be pattern matched on a key basis, which can prove frustrating.

Versioning

Re: Versioning, from the documentation:

To set the API version on a specific request, send a Stripe-Version header.

Function Naming

With regards to function naming, get and the get_* form are standard Elixir naming schemes. There's nothing wrong with naming things differently, of course. I was more bugged with version 1 that there was no function to get a plan by ID regardless of whether it was called get, retrieve, or snuffles_the_happy_pig.

The question is really if the primary audience is going to be targeted towards Elixir developers who are using Stripe (expecting Elixir conventions) or developers who have used Stripe before in other languages that are coming to Elixir (expecting Stripe conventions). Being consistent is the thing that really counts.

[DavidAntaramian]

@dmvt @joshsmith Also, I'm happy to use any style guide you want, just would ask that it be decided on prior to anyone starting work. Credo can also be really useful for automated checking

[dmvt]

Thanks for the additional info @DavidAntaramian. I like everything you said.

Versioning

Given that it is handled in a header, this would be passed as an option in the second argument of any of the high level functions. No need to do anything special to support this.

Lists as optional arguments

My personal preference would be to use maps. We seem to agree that having pattern matching capabilities would be nice and, while the key: value shorthand notation would be useful if we only had one optional list, with 2 of them, I don't think that holds. @joshsmith, any strong options to the contrary?

Function Naming

All of the official Stripe libraries focus on Stripe conventions over language specific conventions so I think we should follow that pattern. To expand on your get, retrieve example, in Ruby, the convention would be to use find. I also think that more often than not, developers will have had some experience with Stripe before and / or look at the official documentation for pointers on what to expect, further reinforcing the idea of sticking with Stripe's conventions. Finally, I think there's a path to having Stripe adopt this officially, and I'd like to have our choices now facilitate that potential.

Hackney / Low Level API

I'm fine with you designing this however you see fit, however, I don't think it'll be useful to have two different versions based on Auth types. There are only two auth approaches for Stripe Connect, passing a connected account's secret key or passing your secret key (the defaultly configured one) plus a Stripe-Account header. The second option is preferred by Stripe, which is why I placed the optional key override at the end of the arguments.

Given the Low Level rewrite, we may want to go a step further than I already have and perform a full rewrite of everything. Can you propose a testing strategy for the low level stuff?

[dmvt]

One more thought on the Low Level API and List arguments: I could be convinced to make the high level api functions only accept an attributes map and a headers list. Hackney options should never need to be changed inline and the secret key option at the end is a sub-optimal way to handle Connect auth. Eliminating it would force users to either use the low level api or conform to preferred conventions.

[DavidAntaramian]

I've been writing a basic API integration for my internal application that I'll be using until the v2 work on this library is done. I have some thoughts from that that apply here.

Versioning

See Low-Level API

List as Optional Arguments

I've had a few ideas about this as I've been implementing. Will cover them in the "high-level" api section.

Function Naming

This sounds reasonable to me.

Low Level API

OK, now I looked at Connect and realized it's a lot less complicated then I thought.

In regards to the version and the primary account API key, both of these should be kept in the application configuration in my opinion. That's standard Elixir practice.

This is the bulk of my low-level API function that I'm using internally:

@spec request(method, String.t, nil | map, %{required(String.t) => String.t}, Keyword.t) :: {:ok, map} | {:error, Exception.t}
def request(method, endpoint, body, headers, opts) do
  url = base_url() <> endpoint

  headers = Map.merge(headers, %{
    "Accept" => "application/json; charset=utf8",
    "Accept-Encoding" => "gzip",
    "Authorization" => "Bearer " <> get_api_key(),
    "Content-Type" => "application/x-www-form-urlencoded",
    "Connection" => "keep-alive"
  })

  req_body = encode_body(body)
  req_headers = encode_req_headers(headers)
  req_opts = Keyword.merge(opts,[
    with_body: true
  ])

  :hackney.request(method, url, req_headers, req_body, req_opts)
  |> handle_response()
end

For the purposes of Stripity-Stripe, I would update it so that opts can contain a key :stripe_account_account which would be the account you want to use Connect with.

High Level API

I've also been playing around with a higher level API in my codebase. What I'm actually doing instead of using maps is accepting and returning structs. This is actually pretty easy and according to the documentation is how Stripe prefers libraries be written:

our API libraries convert responses to appropriate language-specific objects.

What this essentially means is that you would have a module Stripe.Plan with a function retrieve/2:

def retrieve(id, opts // opts) do
  endpoint = @stripe_base_endpoint <> "/" <> id

  Stripe.request(:get, endpoint, nil, %{}, opts)
  |> decode_api_map()
end

Which you could call as Stripe.Plan.retrieve("oak") and get a return something like:

%Stripe.Plan{amount: 10000, created: 1477087897, currency: "usd",
 id: "oak", interval: "month", interval_count: 1, metadata: %{}, name: "Oak",
 statement_descriptor: nil, trial_period_days: nil}

You could also call Stripe.Plan.retrieve("oak", stripe_connect_account: "sk_dfew90l3") which would add the appropriate Stripe Connect header.

[joshsmith]

@DavidAntaramian quick clarification: I'm assuming in your "Low Level API" section, at the very end, you meant : stripe_connect_account?

Overall this looks like a strong direction to me and I appreciate the feedback here. Also like that you found that quote from Stripe.

I'd like to direct some love here ASAP since I'd personally not like to rely too terribly heavily on the 1.x implementation of this package in our own application, so I think I'm willing to direct that time and attention here first rather than re-implement its consumption all over on my end later.

[DavidAntaramian]

@joshsmith Yes…typing too fast 😞 trying to catch up from Friday's outage

[dmvt]

@DavidAntaramian Aren't we all :)

Love the comments and the idea of using Structs. Let's do that.

[DavidAntaramian]

Woo, had way too much to do this week, but now I'm back in working order. I can get the base-level stuff committed tonight or tomorrow night depending on my schedule. Are you going to do a v2 working branch for this?

[joshsmith]

@DavidAntaramian yes, that's the plan! @dmvt got started with a 2.0 branch that clears out most of the existing code. https://github.com/code-corps/stripity-stripe/tree/2.0

[joshsmith]

This is good timing since we're now circling back to needing to implement this. We've had some interns learning Elixir and Phoenix for the first time (go them!), so I've been spending a lot of time there. But both @begedin and I should have time freeing up to carry this forward quickly, and I believe @dmvt is similarly expecting to free up here shortly.

[dmvt]

@DavidAntaramian This is indeed perfect timing. I also had a crazy busy week last week, took on a new position and got them going on Elixir. @joshsmith, this will be blocking for us in the next week or so as well. @joshleecreates will probably be jumping in on this with me.

[joshsmith]

Excited to watch this get moving.

[dmvt]

@Dangeranger would love your comments on the testing strategy

[DavidAntaramian]

So, I've been working with Stripe on a corporate project with Elixir for a couple weeks now. In terms of a testing solution, what I've had working so far is a system that uses Bypass. It gets extremely complicated because there's no good way to maintain state. So…

First, I would recommend that any test "mock" system be distributed as a separate package. The reason for this is that it will undoubtedly require various testing specific components that people may not want to include in their production builds. This way, people can do the following:

[
  {:stripity_stripe, "~> 2.0"},
  {:stripity_stripe_testing, "~> 2.0", only: :test} 
]

Second, the test package can distribute a lightweight replica of the Stripe service. That way, testing can be done something like the following:

test "retrieves all plans where the storage limit is greater than 30" do
  Stripe.Test.create(:plan, 10)
  Stripe.Test.create(:plan, 10, metadata: [storage_limit: 10])
  Stripe.Test.create(:plan, 10, metadata: [storage_limit: 35])
  Stripe.Test.create(:plan, 10, metadata: [storage_limit: 30])

  # This is business logic that will call the StripityStripe module
  plans = MyApp.Module.get_plans(:gt, 30) 

  assert Enum.count(plans) == 10
end

My thought process currently is that data can be stored on a per-test basis in ETS, allowing users to build up the necessary state and tear it down without needing to know the intricacies of the system.

[joshsmith]

@DavidAntaramian are you expecting then that most of our tests here should rely on this external package rather than being self-contained?

[DavidAntaramian]

@joshsmith Yes, though I hesitate to think of it as external as much as complementary. Thought it through a bit more. Essentially, what would happen is as follows:

defmodule Stripe.CustomerTest do
  # this may be able to be true
  use ExUnit.Case, async: false

  # Most of this setup process can be abstracted into the library, but putting
  # it here so that others understand
  setup do
    # Starts a separate process
    #   - That process loads a Plug responder that binds to an available port
    #   - It also seeds an ETS table that it claims ownership of
    #   - The ETS table and Plug responder are bound to this specific test,
    #      so they are isolated to this test and any state manipulation on them
    #      will not affect other tests
    #   - Returns a struct of some form like %Stripe.Test.Server{port: 4515, pid: #PID<0.35.0>}
    {:ok, stripe_server} = Stripe.Test.Server.start()

    Application.put_env(:stripity_stripe, :api_base_url, "http://localhost:#{stripe_server.port}")

    on_exit fn ->
       #  Cleans up the ETS table and stops the plug responder and owning process
       Stripe.Test.Server.stop(stripe_server)
    end

    {:ok, stripe: stripe_server}
  end

  describe "Stripe.Customer.retrieve/1" do
    test "retrieves a customer by ID", %{stripe: stripe} do
      # Uses the Stripe.Test.Customer module to create a new customer in the
      # ETS backed server (essentially, this is a factory method)
      %{id: id} = Stripe.Test.Customer.create(stripe)

      # Makes an actual API request from the high level API through the low level
      # API and through the OS network stack
      {:ok, customer} = Stripe.Customer.retrieve(id)

      assert customer.id == id
    end
  end
end

[joshsmith]

I'm closing this RFC since much of what we discussed here in imperfect form is now implemented in the 2.0.0-alpha.1 release.