The way to use this library for different Aliyun OSS endpoints

[c4710n]

Thanks

Hi, @ug0, thanks for implementing this great library.

The problem

Recently, I have to operate on different Aliyun OSS endpoints.
But,:aliyun_oss is using Application.get_env/2 to getting the global configurations, which makes it impossible to setup :aliyun_oss for different Aliyun OSS endpoints. For example, in config/runtime.exs:

# these configurations are global. No chance to opt out ;(
config :aliyun_oss,
  endpoint: "oss-cn-shenzhen.aliyuncs.com",
  access_key_id: System.fetch_env!("ALIYUN_ACCESS_KEY_ID"),
  access_key_secret: System.fetch_env!("ALIYUN_ACCESS_KEY_SECRET")

I want to solve this problem in a more community-friendly way - don't fork this repo or create my own implementation, which wastes people's energy and weakens community's cohesion.

Some official guidelines

I searched a lot about how to solve this problem. And I found, in the official docs, there is some guidelines about the design of a library:

The application environment should be reserved only for configurations that are truly global, for example, to control your application boot process and its supervision tree. And, generally speaking, it is best to avoid global configuration. If you must use configuration, then prefer runtime configuration instead of compile-time configuration. See the Application module for more information.

For all remaining scenarios, libraries should not force their users to use the application environment for configuration. If the user of a library believes that certain parameter should be configured globally, then they can wrap the library functionality with their own application environment configuration.

Don't misunderstand me, I'm not saying you are doing it wrong. It just doesn't meet the needs at the moment, so maybe we need to make some changes.

A possible solution

local configuration provided as a function argument

If we don't use global configuration any more, what should we use? Local configuration provided as a function argument.

Take Aliyun.Oss.Bucket.list_buckets/1 as an example, the current function signature is:

list_buckets(query_params \\ %{})

After adding a local config, it becomes:

list_buckets(config, query_params \\ %{})

improve the type of config

It's bad to use a random map as configuration, we can do it better - reuse existing Aliyun.Oss.Config and use it provide a %Config{} struct, such as:

defmodule Aliyun.Oss.Config do
  @enforce_keys [:endpoint, :access_key_id, :access_key_secret]
  defstruct @enforce_keys

  @type config() :: %{
          endpoint: String.t(),
          access_key_id: String.t(),
          access_key_secret: String.t()
        }

  @type t :: %__MODULE__{
          endpoint: String.t(),
          access_key_id: String.t(),
          access_key_secret: String.t()
        }

  @spec new!(config()) :: __MODULE__.t()
  def new!(config) when is_map(config) do
    config
    # we can do more checks here
    |> as_struct!()
  end

  defp as_struct!(config) do
    struct!(__MODULE__, config)
  end
end

And the function signature becomes:

alias Aliyun.Oss.Config
list_buckets(%Config{}, query_params \\ %{})

When we want to call this API, we do this:

alias Aliyun.Oss.Config

config = Config.new!(%{
  endpoint: "...",
  access_key_id: "...",
  access_key_secret: "..."
})

list_buckets(config, %{})

Is the possible solution works?

Suppose that I have 2 Aliyun OSS endpoints, and I want to operate on them with two different modules. Then I can implement something like this:

defmodule MyApp.OssA do
  alias Aliyun.Oss.Config
  alias Aliyun.Oss.Bucket

  def list_buckets(query_params \\ %{}) do
    Bucket.list_buckets(config(), query_params)
  end

  def config() do
    :my_app
    |> Application.fetch_env!(MyApp.OssA)
    |> Config.new!()
  end
end

# In the config/runtime.exs
config :my_app, MyApp.OssA,
  endpoint: "...",
  access_key_id: "...",
  access_key_secret: "..."

defmodule MyApp.OssB do
  alias Aliyun.Oss.Config
  alias Aliyun.Oss.Bucket

  def list_buckets(query_params \\ %{}) do
    Bucket.list_buckets(config(), query_params)
  end

  def config() do
    :my_app
    |> Application.fetch_env!(MyApp.OssB)
    |> Config.new!()
  end
end

# In the config/runtime.exs
config :my_app, MyApp.OssB,
  endpoint: "...",
  access_key_id: "...",
  access_key_secret: "..."

These two modules are using different configurations as expected.

"Wait. Does that means users have to map every API?"

Users don't have to every API, but only the API they need.

In real apps, it's rare that all the API provided by :aliyun_oss are required. In general, only a few API are required.

From the perspective of code organization, it's also recommended to map the functions provided by :aliyun_oss to a self-owned module, which helps to abstract the terminology of OSS from business logic, such as a module for managing images:

defmodule MyApp.ImageManagement do
  def upload_image(binary) when is_binary do
    Aliyun.Oss.put_object(
      #...
    )
  end
  
  # more actions, such as get, delete, etc.
end

"Oh, Good. But what should I do?"

The things that need to be done are:

• adjusting all the public functions to accept 1st argument as config.
• removing the calls of global configuration.

But I'm not going to let you do it, after all you've done all of this library.

If you like my ideas and trust me, I can do this.

Last

Any ideas? Please let me know.

[ug0]

Hi, @c4710n, thanks for your suggestion. I agree the global configuration is something we need to change. Your solution sounds perfect to me. It would be great if you can implement this. I'll be more than happy and grateful to accept a PR.

[c4710n]

@ug0. I'm very happy that you like my idea. ;-)

The first step I would like to do is to mix format this repo. Then I can adjust the code without worrying about code style.

I notice that this repo already has a .formatter.exs:

# Used by "mix format"
[
  inputs: ["{mix,.formatter}.exs", "{config,lib,test}/**/*.{ex,exs}"]
]

It's ok to use it. But, before using it, I want to confirm your preference on line_length, the default 98 or a customized one?

For me, either choice is fine. It's just a execution of mix format.

[ug0]

It's ok to use it. But, before using it, I want to confirm your preference on line_length, the default 98 or a customized one?

The default is fine.

[c4710n]

The default is fine.

Got it.

I have created a PR which formats the existing code. - #79

Then, I'll work toward the goal.

[ug0]

#79 has been merged.