Repository: rrrene/credo
Pull request #782 (2020-06-16 02:49:08)

Add check for unsafe Application calls in module attributes 
This check looks for following calls in module attributes:
- `Application.fetch_env/2`
- `Application.fetch_env!/2`
- `Application.get_all_env/1`
- `Application.get_env/3`
- `Application.get_env/2`

With the introduction of `Application.compile_env` in Elixir 1.10, developers should be leveraging that instead.

I personally think this should be in core Credo versus a 3rd party plugin as this is something I see happen almost weekly by people new to Elixir and even some seasoned Elixir developers.

As a side note, thanks a bunch for Credo! Been using it for years and appreciate all the hard work you have put into it :).



Patch:
diff --git a/lib/credo/check/warning/application_config_in_module_attribute.ex b/lib/credo/check/warning/application_config_in_module_attribute.ex
new file mode 100644
index 00000000..3abca374
--- /dev/null
+++ b/lib/credo/check/warning/application_config_in_module_attribute.ex
@@ -0,0 +1,103 @@
+defmodule Credo.Check.Warning.ApplicationConfigInModuleAttribute do
+  use Credo.Check,
+    base_priority: :high,
+    tags: [:controversial],
+    category: :warning,
+    explanations: [
+      check: """
+      Module attributes are evaluated at compile time and not at run time. As
+      a result, certain configuration read calls made in your module attributes
+      may work as expected during local development, but may break once in a
+      deployed context.
+
+      This check analyzes all of the module attributes present within a module,
+      and validates that there are no unsafe calls. Unsafe calls include:
+        - `Application.fetch_env/2`
+        - `Application.fetch_env!/2`
+        - `Application.get_all_env/1`
+        - `Application.get_env/3`
+        - `Application.get_env/2`
+
+      As of Elixir 1.10 you can leverage `Application.compile_env/3` and
+      `Application.compile_env!/2` if you wish to set configuration at
+      compile time using module attributes.
+      """
+    ]
+
+  @doc false
+  @impl true
+  def run(%SourceFile{} = source_file, params \\ []) do
+    issue_meta = IssueMeta.for(source_file, params)
+
+    Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+  end
+
+  defp traverse({:@, meta, [attribute_definition]} = ast, issues, issue_meta) do
+    case traverse_attribute(attribute_definition) do
+      nil ->
+        {ast, issues}
+
+      {attribute, call} ->
+        {ast, issues_for_call(attribute, call, meta, issue_meta, issues)}
+    end
+  end
+
+  defp traverse(ast, issues, _issue_meta) do
+    {ast, issues}
+  end
+
+  defp traverse_attribute({attribute, _, _} = ast) do
+    case Macro.prewalk(ast, nil, &get_forbidden_call/2) do
+      {_ast, nil} ->
+        nil
+
+      {_ast, call} ->
+        {attribute, call}
+    end
+  end
+
+  defp get_forbidden_call(
+         {{:., _, [{:__aliases__, _, [:Application]}, :fetch_env]}, _meta, _args} = ast,
+         _acc
+       ) do
+    {ast, "Application.fetch_env/2"}
+  end
+
+  defp get_forbidden_call(
+         {{:., _, [{:__aliases__, _, [:Application]}, :fetch_env!]}, _meta, _args} = ast,
+         _acc
+       ) do
+    {ast, "Application.fetch_env!/2"}
+  end
+
+  defp get_forbidden_call(
+         {{:., _, [{:__aliases__, _, [:Application]}, :get_all_env]}, _meta, _args} = ast,
+         _acc
+       ) do
+    {ast, "Application.get_all_env/1"}
+  end
+
+  defp get_forbidden_call(
+         {{:., _, [{:__aliases__, _, [:Application]}, :get_env]}, _meta, args} = ast,
+         _acc
+       ) do
+    {ast, "Application.get_env/#{length(args)}"}
+  end
+
+  defp get_forbidden_call(ast, acc) do
+    {ast, acc}
+  end
+
+  defp issues_for_call(attribute, call, meta, issue_meta, issues) do
+    options = [
+      message:
+        "Module attribute @#{Atom.to_string(attribute)} makes use of unsafe Application configuration call #{
+          call
+        }",
+      trigger: call,
+      line_no: meta[:line]
+    ]
+
+    [format_issue(issue_meta, options) | issues]
+  end
+end


Test patch:
diff --git a/test/credo/check/warning/application_config_in_module_attribute_test.exs b/test/credo/check/warning/application_config_in_module_attribute_test.exs
new file mode 100644
index 00000000..91fa3539
--- /dev/null
+++ b/test/credo/check/warning/application_config_in_module_attribute_test.exs
@@ -0,0 +1,90 @@
+defmodule Credo.Check.Warning.ApplicationConfigInModuleAttributeTest do
+  use Credo.Test.Case
+
+  @described_check Credo.Check.Warning.ApplicationConfigInModuleAttribute
+
+  #
+  # cases NOT raising issues
+  #
+
+  test "it should NOT report an issue if compile_env and compile_env! are used" do
+    """
+    defmodule CredoSampleModule do
+      @config_1 Application.compile_env!(:my_app, :key)
+      @config_2 Application.compile_env(:my_app, :key, :default)
+      @config_3 Application.compile_env(:my_app, :key)
+      @config_4 :some_hard_coded_value
+
+      def some_function(parameter1, parameter2) do
+        parameter1 + parameter2
+      end
+    end
+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> refute_issues()
+  end
+
+  #
+  # cases raising issues
+  #
+
+  test "it should report a violation when bad calls are made" do
+    errors =
+      """
+      defmodule CredoSampleModule do
+        @config_1 Application.fetch_env(:my_app, :key)
+        @config_2 :valid
+        @config_3 Application.fetch_env!(:my_app, :key)
+        @config_4 "Another value"
+        @config_5 Application.get_all_env(:my_app)
+        @config_6 999_999_999
+        @config_7 Application.get_env(:my_app, :key)
+        @config_8 "More padding"
+        @config_9 Application.get_env(:my_app, :key, :some_default)
+
+        def some_function(parameter1, parameter2) do
+          IO.inspect parameter1 + parameter2
+        end
+      end
+      """
+      |> to_source_file
+      |> run_check(@described_check)
+
+    assert_errors = [
+      {
+        "Module attribute @config_1 makes use of unsafe Application configuration call Application.fetch_env/2",
+        2
+      },
+      {
+        "Module attribute @config_3 makes use of unsafe Application configuration call Application.fetch_env!/2",
+        4
+      },
+      {
+        "Module attribute @config_5 makes use of unsafe Application configuration call Application.get_all_env/1",
+        6
+      },
+      {
+        "Module attribute @config_7 makes use of unsafe Application configuration call Application.get_env/2",
+        8
+      },
+      {
+        "Module attribute @config_9 makes use of unsafe Application configuration call Application.get_env/3",
+        10
+      }
+    ]
+
+    assert length(errors) == 5
+
+    Enum.each(assert_errors, fn {error_message, line_number} ->
+      assert error_exists?(errors, error_message, line_number)
+    end)
+  end
+
+  defp error_exists?(errors, error_message, line_number) do
+    Enum.any?(errors, fn
+      %Credo.Issue{message: ^error_message, line_no: ^line_number} -> true
+      _ -> false
+    end)
+  end
+end