This is an automated email from the ASF dual-hosted git repository. hsluoyz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/casbin-ex.git
commit 2eddf766e4ae438f08142651b199bb993c3d7fc0 Author: Yang Luo <[email protected]> AuthorDate: Fri Aug 7 00:26:26 2026 +0800 refactor: resolve credo --strict violations across lib and test The pre-push hook never actually ran against this codebase, so 21 strict violations had accumulated. Extract helpers to flatten deeply nested bodies in Enforcer, replace the per-column case statements in the Ecto and readonly file adapters with field lookups, and clean up the test files. No behaviour change; test suite output is identical. --- lib/casbin/enforcer.ex | 125 ++++++++++----------- lib/casbin/persist/ecto_adapter.ex | 51 ++++----- lib/casbin/persist/readonly_file_adapter.ex | 22 +--- test/persist/ecto_sandbox_transaction_test.exs | 8 +- .../readonly_file_adapter_filtered_test.exs | 4 +- 5 files changed, 90 insertions(+), 120 deletions(-) diff --git a/lib/casbin/enforcer.ex b/lib/casbin/enforcer.ex index 3960d85..d2675b0 100644 --- a/lib/casbin/enforcer.ex +++ b/lib/casbin/enforcer.ex @@ -1003,21 +1003,15 @@ defmodule Casbin.Enforcer do """ @spec key_get(String.t(), String.t()) :: String.t() def key_get(key1, key2) do - i = :binary.match(key2, "*") - - case i do + case :binary.match(key2, "*") do :nomatch -> "" {pos, _} -> - if String.length(key1) > pos do - prefix = String.slice(key2, 0, pos) - - if String.starts_with?(key1, prefix) do - String.slice(key1, pos, String.length(key1) - pos) - else - "" - end + prefix = String.slice(key2, 0, pos) + + if String.length(key1) > pos and String.starts_with?(key1, prefix) do + String.slice(key1, pos, String.length(key1) - pos) else "" end @@ -1083,23 +1077,23 @@ defmodule Casbin.Enforcer do keys <- Regex.scan(r1, key2) |> Enum.map(fn [k] -> k end), key2 <- Regex.replace(r1, key2, "([^/]+)"), {:ok, r2} <- Regex.compile("^" <> key2 <> "$"), - matches <- Regex.run(r2, key1) do - if matches && length(matches) > 1 do - values = Enum.drop(matches, 1) - - keys - |> Enum.zip(values) - |> Enum.find_value("", fn {k, v} -> - if String.slice(k, 1..-1//1) == path_var, do: v - end) - else - "" - end + [_full | values] when values != [] <- Regex.run(r2, key1) do + find_path_var(keys, values, path_var) else _ -> "" end end + # Pairs the `:var` names captured from the pattern with their matched values + # and returns the value bound to `path_var`, or "" when it is not present. + defp find_path_var(keys, values, path_var) do + keys + |> Enum.zip(values) + |> Enum.find_value("", fn {k, v} -> + if String.slice(k, 1..-1//1) == path_var, do: v + end) + end + @doc """ Returns `true` if `key1` matches the pattern of `key2`. @@ -1149,36 +1143,32 @@ defmodule Casbin.Enforcer do tokens <- Regex.scan(r1, key2) |> Enum.map(fn [_, token] -> token end), key2_pattern <- Regex.replace(r1, key2, "([^/]+)"), {:ok, r2} <- Regex.compile("^" <> key2_pattern <> "$"), - matches <- Regex.run(r2, key1) do - if matches do - values = Enum.drop(matches, 1) - - if length(tokens) != length(values) do - false - else - # Build a map of token -> value and check for conflicts - tokens - |> Enum.zip(values) - |> Enum.reduce_while(%{}, fn {token, value}, acc -> - case Map.get(acc, token) do - nil -> {:cont, Map.put(acc, token, value)} - ^value -> {:cont, acc} - _ -> {:halt, :mismatch} - end - end) - |> case do - :mismatch -> false - _ -> true - end - end - else - false - end + [_full | values] <- Regex.run(r2, key1) do + tokens_consistent?(tokens, values) else _ -> false end end + # Every repetition of the same `{token}` has to capture the same value. + defp tokens_consistent?(tokens, values) when length(tokens) == length(values) do + tokens + |> Enum.zip(values) + |> Enum.reduce_while(%{}, fn {token, value}, acc -> + case Map.get(acc, token) do + nil -> {:cont, Map.put(acc, token, value)} + ^value -> {:cont, acc} + _ -> {:halt, :mismatch} + end + end) + |> case do + :mismatch -> false + _ -> true + end + end + + defp tokens_consistent?(_tokens, _values), do: false + @doc """ Returns `true` if IP address `ip1` matches the pattern of `ip2`. @@ -1196,20 +1186,23 @@ defmodule Casbin.Enforcer do @spec ip_match?(String.t(), String.t()) :: boolean() def ip_match?(ip1, ip2) do case parse_ip(ip1) do - {:ok, ip1_tuple} -> - case parse_cidr(ip2) do - {:ok, network, prefix_len} -> - ip_in_cidr?(ip1_tuple, network, prefix_len) - - :error -> - case parse_ip(ip2) do - {:ok, ip2_tuple} -> ip1_tuple == ip2_tuple - :error -> false - end - end + {:ok, ip1_tuple} -> ip_matches_target?(ip1_tuple, ip2) + :error -> false + end + end + + # `ip2` is either a CIDR range the address has to fall into, or a plain + # address it has to equal. + defp ip_matches_target?(ip1_tuple, ip2) do + case parse_cidr(ip2) do + {:ok, network, prefix_len} -> + ip_in_cidr?(ip1_tuple, network, prefix_len) :error -> - false + case parse_ip(ip2) do + {:ok, ip2_tuple} -> ip1_tuple == ip2_tuple + :error -> false + end end end @@ -1249,17 +1242,15 @@ defmodule Casbin.Enforcer do defp ip_to_bits({a, b, c, d}) do <<a, b, c, d>> |> :binary.bin_to_list() - |> Enum.map(&Integer.to_string(&1, 2)) - |> Enum.map(&String.pad_leading(&1, 8, "0")) - |> Enum.join() + |> Enum.map_join(&(&1 |> Integer.to_string(2) |> String.pad_leading(8, "0"))) end defp ip_to_bits({a, b, c, d, e, f, g, h}) do # Convert each 16-bit segment to binary representation - [a, b, c, d, e, f, g, h] - |> Enum.map(&Integer.to_string(&1, 2)) - |> Enum.map(&String.pad_leading(&1, 16, "0")) - |> Enum.join() + Enum.map_join( + [a, b, c, d, e, f, g, h], + &(&1 |> Integer.to_string(2) |> String.pad_leading(16, "0")) + ) end @doc """ diff --git a/lib/casbin/persist/ecto_adapter.ex b/lib/casbin/persist/ecto_adapter.ex index 7d3edac..8601767 100644 --- a/lib/casbin/persist/ecto_adapter.ex +++ b/lib/casbin/persist/ecto_adapter.ex @@ -178,6 +178,11 @@ defmodule Casbin.Persist.EctoAdapter do end defimpl Casbin.Persist.PersistAdapter, for: Casbin.Persist.EctoAdapter do + alias Casbin.Persist.EctoAdapter + + # Columns a policy filter is allowed to constrain. + @filterable_fields [:ptype, :v0, :v1, :v2, :v3, :v4, :v5, :v6] + @doc """ Queries the list of policy rules from the database and returns them as a list of strings. @@ -193,7 +198,7 @@ defmodule Casbin.Persist.EctoAdapter do end def load_policies(adapter) do - repo = Casbin.Persist.EctoAdapter.get_repo(adapter) + repo = EctoAdapter.get_repo(adapter) policies = repo.all(CasbinRule) @@ -230,7 +235,7 @@ defmodule Casbin.Persist.EctoAdapter do end def load_filtered_policy(adapter, filter) when is_map(filter) do - repo = Casbin.Persist.EctoAdapter.get_repo(adapter) + repo = EctoAdapter.get_repo(adapter) query = build_filtered_query(filter) policies = @@ -249,39 +254,23 @@ defmodule Casbin.Persist.EctoAdapter do end) end - # Helper function to add WHERE clause for a single filter condition - defp add_where_clause(query, field, values) when is_list(values) do + # Helper function to add WHERE clause for a single filter condition. + # Unknown fields are ignored so a stray filter key cannot widen the query. + defp add_where_clause(query, field, values) + when field in @filterable_fields and is_list(values) do import Ecto.Query - case field do - :ptype -> where(query, [r], r.ptype in ^values) - :v0 -> where(query, [r], r.v0 in ^values) - :v1 -> where(query, [r], r.v1 in ^values) - :v2 -> where(query, [r], r.v2 in ^values) - :v3 -> where(query, [r], r.v3 in ^values) - :v4 -> where(query, [r], r.v4 in ^values) - :v5 -> where(query, [r], r.v5 in ^values) - :v6 -> where(query, [r], r.v6 in ^values) - _ -> query - end + where(query, [r], field(r, ^field) in ^values) end - defp add_where_clause(query, field, value) do + defp add_where_clause(query, field, value) when field in @filterable_fields do import Ecto.Query - case field do - :ptype -> where(query, [r], r.ptype == ^value) - :v0 -> where(query, [r], r.v0 == ^value) - :v1 -> where(query, [r], r.v1 == ^value) - :v2 -> where(query, [r], r.v2 == ^value) - :v3 -> where(query, [r], r.v3 == ^value) - :v4 -> where(query, [r], r.v4 == ^value) - :v5 -> where(query, [r], r.v5 == ^value) - :v6 -> where(query, [r], r.v6 == ^value) - _ -> query - end + where(query, [r], field(r, ^field) == ^value) end + defp add_where_clause(query, _field, _value), do: query + @doc """ Uses the configured repo to insert a Policy into the casbin_rule table. @@ -299,7 +288,7 @@ defmodule Casbin.Persist.EctoAdapter do end def add_policy(adapter, {_key, _attrs} = policy) do - repo = Casbin.Persist.EctoAdapter.get_repo(adapter) + repo = EctoAdapter.get_repo(adapter) changeset = CasbinRule.create_changeset(policy) # on_conflict: :nothing prevents Ecto.ConstraintError when a duplicate rule is @@ -331,7 +320,7 @@ defmodule Casbin.Persist.EctoAdapter do end def remove_policy(adapter, {_key, _attr} = policy) do - repo = Casbin.Persist.EctoAdapter.get_repo(adapter) + repo = EctoAdapter.get_repo(adapter) f = CasbinRule.changeset_to_queryable(policy) case repo.delete_all(f) do @@ -341,7 +330,7 @@ defmodule Casbin.Persist.EctoAdapter do end def remove_filtered_policy(adapter, key, idx, attrs) do - repo = Casbin.Persist.EctoAdapter.get_repo(adapter) + repo = EctoAdapter.get_repo(adapter) f = CasbinRule.changeset_to_queryable({key, attrs}, idx) case repo.delete_all(f) do @@ -367,7 +356,7 @@ defmodule Casbin.Persist.EctoAdapter do end def save_policies(adapter, policies) do - repo = Casbin.Persist.EctoAdapter.get_repo(adapter) + repo = EctoAdapter.get_repo(adapter) repo.transaction(fn -> insert_policies(repo, adapter, policies) end) end diff --git a/lib/casbin/persist/readonly_file_adapter.ex b/lib/casbin/persist/readonly_file_adapter.ex index dadc54b..157650e 100644 --- a/lib/casbin/persist/readonly_file_adapter.ex +++ b/lib/casbin/persist/readonly_file_adapter.ex @@ -15,6 +15,9 @@ defmodule Casbin.Persist.ReadonlyFileAdapter do end defimpl PersistAdapter, for: Casbin.Persist.ReadonlyFileAdapter do + # Position of each `vN` filter key within a rule's value list. + @value_indexes %{v0: 0, v1: 1, v2: 2, v3: 3, v4: 4, v5: 5, v6: 6} + def load_policies(%Casbin.Persist.ReadonlyFileAdapter{policy_file: nil}) do {:ok, []} end @@ -81,22 +84,9 @@ defmodule Casbin.Persist.ReadonlyFileAdapter do defp get_policy_value([ptype | _values], :ptype), do: ptype defp get_policy_value([_ptype | values], key) do - index = - case key do - :v0 -> 0 - :v1 -> 1 - :v2 -> 2 - :v3 -> 3 - :v4 -> 4 - :v5 -> 5 - :v6 -> 6 - _ -> nil - end - - if index && index < length(values) do - Enum.at(values, index) - else - nil + case Map.fetch(@value_indexes, key) do + {:ok, index} -> Enum.at(values, index) + :error -> nil end end diff --git a/test/persist/ecto_sandbox_transaction_test.exs b/test/persist/ecto_sandbox_transaction_test.exs index 99441f1..516e2a5 100644 --- a/test/persist/ecto_sandbox_transaction_test.exs +++ b/test/persist/ecto_sandbox_transaction_test.exs @@ -57,7 +57,7 @@ defmodule Casbin.Persist.EctoSandboxTransactionTest do # # Add multiple policies atomically # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["alice", "data1", "read"]}) # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["alice", "data1", "write"]}) - # + # # {:ok, :success} # end) # @@ -76,7 +76,7 @@ defmodule Casbin.Persist.EctoSandboxTransactionTest do # result = @repo.transaction(fn -> # # Add a policy # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["bob", "data2", "read"]}) - # + # # # Simulate a failure that causes rollback # @repo.rollback(:simulated_error) # end) @@ -96,11 +96,11 @@ defmodule Casbin.Persist.EctoSandboxTransactionTest do # result = @repo.transaction(fn -> # # Insert a user record (example - replace with your schema) # # {:ok, user} = @repo.insert(%User{name: "charlie"}) - # + # # # Add corresponding Casbin policies # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["charlie", "data3", "read"]}) # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["charlie", "data3", "write"]}) - # + # # {:ok, :complete} # end) # diff --git a/test/persist/readonly_file_adapter_filtered_test.exs b/test/persist/readonly_file_adapter_filtered_test.exs index e4db0d3..e326a12 100644 --- a/test/persist/readonly_file_adapter_filtered_test.exs +++ b/test/persist/readonly_file_adapter_filtered_test.exs @@ -1,8 +1,8 @@ defmodule Casbin.Persist.ReadonlyFileAdapterFilteredTest do use ExUnit.Case, async: true alias Casbin.Enforcer - alias Casbin.Persist.ReadonlyFileAdapter alias Casbin.Persist.PersistAdapter + alias Casbin.Persist.ReadonlyFileAdapter @cfile "../data/rbac_domain.conf" |> Path.expand(__DIR__) @pfile "../data/rbac_domain.csv" |> Path.expand(__DIR__) @@ -112,7 +112,7 @@ defmodule Casbin.Persist.ReadonlyFileAdapterFilteredTest do # Without role mappings loaded, role-based permissions should not work mapping_policies = Enforcer.list_mapping_policies(e) - assert length(mapping_policies) == 0 + assert mapping_policies == [] end end end --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
