Assumes top-level configuration in app config

[aseigo]

Looking at usage of get_env in stripity_stripe (branch 2.0), there are these uses:

lib/stripe/connect/oauth.ex: Application.get_env(:stripity_stripe, :connect_client_id) lib/stripe/connect/oauth.ex: Application.get_env(:stripity_stripe, :api_key) lib/stripe.ex: Application.get_env(:stripity_stripe, :pool_options) lib/stripe.ex: Application.get_env(:stripity_stripe, :api_base_url) lib/stripe.ex: Application.get_env(:stripity_stripe, :api_upload_url) lib/stripe.ex: case Application.get_env(:stripity_stripe, :api_key) do lib/stripe.ex: Application.get_env(:stripity_stripe, :use_connection_pool)

This is fine if stripity_stripe is being used directly from a "final" application, but if it is being used as part of another library / component that may want to provide these values from somewhere else, then this becomes an issue.

While it is possible to "play with" the application env, this is specifically discouraged in the docs and is indeed quite brittle to change.

Would it be palatable to the stripity_stripe team to instead optionally accept configuration arguments to Stripe.start (the 2nd param, _args, is currently unused)?

In such a scenario:

• Stripe.start would generate the full config from args plus the calls to get_env, placed into an Agent that joins the supervision tree
• the getter functions could remain as is, though they would call into the agent for the relevant config data rather than hitting get_env directly

Such a change could be 100% backwards compatible, without even any changes to private functions. If there is interest in this, I will submit a PR for it...

[aseigo]

Actually, simpler would be to do as Ecto et al do: support an :otp_app option which is then used for the configuration source. (Sorry for the brainfart-overly-complex solution suggestion above..)

[DavidAntaramian]

Could you elaborate more on the use-case by-which an application would include this library in the manner you've suggested?

[joshsmith]

@DavidAntaramian, my guess is that @aseigo's project https://github.com/aseigo/cashier is the use case here.

[swelham]

@JoshSmith @DavidAntaramian yes, I can confirm that this is indeed the use case.

We're building a payment gateway which will support multiple payment providers under a single public interface. Our intention is to use your library to prevent reinventing the work you guys have already put in and the above enhancement would make this possible for us.

[aseigo]

Indeed, that's the one ... and to be fair/clear, it's actualy @swelham's project :) I'm just throwing a bit of code his way ... thanks for the quick response!

[DavidAntaramian]

Gotcha. This seems logical. Not sure that :otp_app would be a complete solution since Ecto is being included by a macro which then uses the variables passed to it. We definitely could tailor the configuration fetching to allow for the configuration to be "injected". I'd like to keep using application configuration as the single source of truth, but figure out a way that we can allow consumer libraries to essentially have a "white label" configuration method.

@swelham @aseigo Can you give me an idea of the ideal experience your users would have (ignoring how that would be implemented in stripity_stripe)? Code examples that you'd expect your end-users to use would be superb. That will give me a better idea of what the pathways available to us are. For example, will would you declare :stripity_stripe as a dependency package regardless of whether users will use stripe, or will you take the ExAws approach and expect the users to install the additional packages they need in order to reduce the dependency surface?

[joshsmith]

If I were consuming the package I would definitely prefer the ExAws approach, fwiw.

[swelham]

My long term goal is the ExAws approach whereby users will have to bring in the payment gateways they want to use. However as it stands at the moment everything is bundled together to allow quick refactoring whilst we work out the approach we want to take.

This is how we would like our users to configure a gateway within cashier.

config :cashier, :stripe,
  url: "https://api.stripe.com",
  client_secret: "<stripe_client_secret>",
  ... other required config option

This would be the only stripe specific thing a user is required to do and from this point on the client code only interacts with the cashier public interface.

[swelham]

@JoshSmith @DavidAntaramian Any further thoughts on this?

I (or maybe @aseigo) would be happy to help with a PR if needed?

[DavidAntaramian]

@swelham Meant to loop back on this, sorry!

I was mulling this over, and I don't know if it's actually necessary for us to change anything. Here's my reasoning:

• We assume that the configuration information that the package relies upon will be located in the application configuration, a standardized and stateful repository of configuration information
• We do not assume that the configuration is final at the time of compilation, therefore we fetch from the application configuration on each call (which under most circumstances should be a constant time operation / O(1))

I think it would be less complicated on both sides if you instead were to simply inject the necessary configuration data into stripity_stripe's application configuration when your package is started:

defmodule Cashier do
  use Application

  # see start/2
  @known_gateways [
    :stripe
  ]

  # other Cashier functions…

  @doc false
  # Implementation of the Application.start/2 callback. Sets up the necessary configuration
  # for gateway modules
  def start(_type, _args) do
    import Supervisor.Spec, warn: false

    # In theory you could have the user specify exactly which gateways they want to
    # use, but I just made a general assumption to attempt configuration for all
    # known gateways
    configure_gateways(@known_gateways)
    
    # Start the application's supervisor
    # (seems like you don't have one currently so this could just have no children)
    children = []
    opts = [strategy: :one_for_one, name: Cashier.Supervisor]
    Supervisor.start_link([], opts)
  end

  defp configure_gateways(gateways) do
    Enum.each(&configure_gateway/1)
  end

  defp configure_gateway(:stripe) do
    # you could do a test here about what the config returns
    # to determine whether any further action is needed (i.e.,
    # if the module isn't being used, don't configure it)
    stripe_config = Application.get_env(:cashier, :stripe)
    
    # I would need to look into your application more about
    # how you are specifying defaults, but I'm assuming you're
    # providing them by default in the `env` key in your Mix project's
    # application definition, so they'll already be present in the
    # stripe_config list fetched above

    Application.put_env(:stripity_stripe, :api_base_url, stripe_config[:url])
    Application.put_env(:stripity_stripe, :api_key, stripe_config[:client_secret])
  end
end

Let me know your thoughts

[swelham]

I agree, this looks like a good solution that will suit both libraries.

Each cashier gateway has an init callback where this setup could be done, or I might enhance the gateway supervisor to sort this out so that each gateway doesn't need to check if it has already been done.

@aseigo As you're working on the stripe gateway, what do you think about this?

[joshsmith]

Awesome let us know if this is indeed workable and perhaps you could set up a PR to document what David outlined.

[github-actions[bot]]

This issue has been automatically marked as "stale:discard". If this issue still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment.

[github-actions[bot]]

Closing this issue after a prolonged period of inactivity. If this issue is still relevant, feel free to re-open the issue. Thank you!