From 10478ddd722c70ad0fb417a9b4fbe21a060006fc Mon Sep 17 00:00:00 2001 From: Hamza Belhaj Date: Wed, 22 Feb 2023 22:29:32 +0100 Subject: [PATCH] ensure code proposal contains a valid appup file with the right versions (#899) * ensure code proposal contains a valid appup file with the right versions * review feedbacks * refactor tests and get_changes function * fixed appup validation to also handle up and down instructions and enhanced tests --- lib/archethic/governance/code.ex | 101 +++++++++++--- .../governance/code/proposal/parser.ex | 3 +- mix.exs | 3 +- mix.lock | 1 + test/archethic/governance/code_test.exs | 127 ++++++++++++++++++ 5 files changed, 218 insertions(+), 17 deletions(-) diff --git a/lib/archethic/governance/code.ex b/lib/archethic/governance/code.ex index 91ed4f0f1..e38a62d95 100644 --- a/lib/archethic/governance/code.ex +++ b/lib/archethic/governance/code.ex @@ -67,8 +67,11 @@ defmodule Archethic.Governance.Code do - Git diff/patch must be valid. """ @spec valid_proposal?(Proposal.t()) :: boolean() - def valid_proposal?(prop = %Proposal{version: version}) do - with true <- succeessor_version?(current_version(), version), + def valid_proposal?(prop = %Proposal{version: version, changes: changes}) do + current_version = current_version() + + with true <- successor_version?(current_version, version), + true <- valid_appup?(changes, version, current_version), true <- applicable_proposal?(prop) do true else @@ -77,6 +80,74 @@ defmodule Archethic.Governance.Code do end end + @doc """ + Ensure the code proposal contains a valid appup file + """ + @spec valid_appup?(binary(), binary(), binary()) :: boolean() + def valid_appup?(changes, version, current_version) do + with {:ok, patches} <- GitDiff.parse_patch(changes), + %GitDiff.Patch{chunks: chunks} <- + Enum.find( + patches, + &(String.starts_with?(&1.to, "rel/appups/archethic") and + String.ends_with?(&1.to, ".appup")) + ), + %GitDiff.Chunk{lines: lines} <- Enum.at(chunks, 0), + code_txt <- + Enum.reduce(lines, "", fn + %GitDiff.Line{type: :add, text: "+" <> text}, acc -> + acc <> text + + _, acc -> + acc + end), + {:ok, {version_char, up_instructions, down_instructions}} <- eval_str(code_txt <> "\n"), + true <- version == to_string(version_char), + current_version_char_up <- + Enum.map(up_instructions, &elem(&1, 0)) + |> Enum.uniq(), + current_version_char_down <- + Enum.map(down_instructions, &elem(&1, 0)) + |> Enum.uniq(), + true <- current_version == to_string(current_version_char_up), + true <- current_version == to_string(current_version_char_down), + :ok <- + up_instructions + |> Enum.map(&elem(&1, 1)) + |> List.flatten() + |> Distillery.Releases.Appup.Utils.validate_instructions(), + :ok <- + down_instructions + |> Enum.map(&elem(&1, 1)) + |> List.flatten() + |> Distillery.Releases.Appup.Utils.validate_instructions() do + true + else + _ -> false + end + end + + # working around a bug in typespecs for :erl_eval.eval_str + defp eval_str(str) do + bin = :erlang.binary_to_list(str) + + {:done, {:ok, token, _}, []} = :erl_scan.tokens([], bin, 0) + {:ok, expr} = :erl_parse.parse_exprs(token) + + {:value, val, _} = + :erl_eval.exprs( + expr, + :erl_eval.new_bindings(), + {:value, &catch_function_calls/2}, + {:value, &catch_function_calls/2} + ) + + {:ok, val} + end + + defp catch_function_calls(_func_name, _args), + do: throw("Appup file contained calls to a function which is not permitted") + @doc """ Ensure the code proposal is an applicable on the current branch. """ @@ -138,49 +209,49 @@ defmodule Archethic.Governance.Code do ## Examples - iex> Code.succeessor_version?("1.1.1", "1.1.2") + iex> Code.successor_version?("1.1.1", "1.1.2") true - iex> Code.succeessor_version?("1.1.1", "1.2.0") + iex> Code.successor_version?("1.1.1", "1.2.0") true - iex> Code.succeessor_version?("1.1.1", "2.0.0") + iex> Code.successor_version?("1.1.1", "2.0.0") true - iex> Code.succeessor_version?("1.1.1", "1.2.2") + iex> Code.successor_version?("1.1.1", "1.2.2") false - iex> Code.succeessor_version?("1.1.1", "1.2.1") + iex> Code.successor_version?("1.1.1", "1.2.1") false - iex> Code.succeessor_version?("1.1.1", "1.1.2-pre0") + iex> Code.successor_version?("1.1.1", "1.1.2-pre0") false """ - @spec succeessor_version?(binary | Version.t(), binary | Version.t()) :: boolean - def succeessor_version?(version1, version2) + @spec successor_version?(binary | Version.t(), binary | Version.t()) :: boolean + def successor_version?(version1, version2) when is_binary(version1) and is_binary(version2) do - succeessor_version?(Version.parse!(version1), Version.parse!(version2)) + successor_version?(Version.parse!(version1), Version.parse!(version2)) end - def succeessor_version?( + def successor_version?( %Version{major: ma, minor: mi, patch: pa1, pre: [], build: nil}, %Version{major: ma, minor: mi, patch: pa2, pre: [], build: nil} ), do: pa1 + 1 == pa2 - def succeessor_version?( + def successor_version?( %Version{major: ma, minor: mi1, patch: _, pre: [], build: nil}, %Version{major: ma, minor: mi2, patch: 0, pre: [], build: nil} ), do: mi1 + 1 == mi2 - def succeessor_version?( + def successor_version?( %Version{major: ma1, minor: _, patch: _, pre: [], build: nil}, %Version{major: ma2, minor: 0, patch: 0, pre: [], build: nil} ), do: ma1 + 1 == ma2 - def succeessor_version?(%Version{}, %Version{}), do: false + def successor_version?(%Version{}, %Version{}), do: false defp current_version do :archethic |> Application.spec(:vsn) |> List.to_string() diff --git a/lib/archethic/governance/code/proposal/parser.ex b/lib/archethic/governance/code/proposal/parser.ex index 7fd259bc8..bca3f62bf 100644 --- a/lib/archethic/governance/code/proposal/parser.ex +++ b/lib/archethic/governance/code/proposal/parser.ex @@ -51,7 +51,7 @@ defmodule Archethic.Governance.Code.Proposal.Parser do index 30787f1..6535f52 100644 --- a/lib/archethic/governance.ex +++ b/lib/archethic/governance.ex - @@ -1,107 +1,107 @@" } + @@ -1,107 +1,107 @@\n" } """ @spec get_changes(binary()) :: {:ok, binary()} | {:error, :missing_changes} def get_changes(content) when is_binary(content) do @@ -61,6 +61,7 @@ defmodule Archethic.Governance.Code.Proposal.Parser do changes_match |> List.first() |> String.trim() + |> Kernel.<>("\n") {:ok, changes} diff --git a/mix.exs b/mix.exs index d16e2d6f7..a82c0b3ce 100644 --- a/mix.exs +++ b/mix.exs @@ -120,7 +120,8 @@ defmodule Archethic.MixProject do {:castore, "~> 1.0", override: true}, {:floki, "~> 0.33"}, {:ex_cldr, "~> 2.7"}, - {:ex_cldr_numbers, "~> 2.29"} + {:ex_cldr_numbers, "~> 2.29"}, + {:git_diff, "~> 0.6.4"} ] end diff --git a/mix.lock b/mix.lock index a2b9740a4..71517c27a 100644 --- a/mix.lock +++ b/mix.lock @@ -41,6 +41,7 @@ "flow": {:hex, :flow, "1.2.3", "1d9239be6a6ec8ef33f22200c8d8b3a756a28145476c7e096fbb3fda04e12f5c", [:mix], [{:gen_stage, "~> 1.0", [hex: :gen_stage, repo: "hexpm", optional: false]}], "hexpm", "77eeb976cb5152a1168290ce9015b8b2d41b70eb5265a36d7538a817237891d9"}, "gen_stage": {:hex, :gen_stage, "1.2.0", "ee49244b57803f54bdab08a60a927e1b4e5bb5d635c52eca0f376a0673af3f8c", [:mix], [], "hexpm", "c3e40992c72e74d9c4eda16d7515bf32c9e7b634e827ab11091fff3290f7b503"}, "gen_state_machine": {:hex, :gen_state_machine, "3.0.0", "1e57f86a494e5c6b14137ebef26a7eb342b3b0070c7135f2d6768ed3f6b6cdff", [:mix], [], "hexpm", "0a59652574bebceb7309f6b749d2a41b45fdeda8dbb4da0791e355dd19f0ed15"}, + "git_diff": {:hex, :git_diff, "0.6.4", "ec53ebf8bf83b4527d938d6433e3686b47a3e2a23135a21038f76736c16bb6e0", [:mix], [], "hexpm", "9e05563c136c91e960a306fd296156b2e8d74e294ae60961e69a36e118023a5f"}, "git_hooks": {:hex, :git_hooks, "0.7.3", "09489e94d88dfc767662e22aff2b6208bd7cf555a19dd0e1477cca4683ce0701", [:mix], [{:blankable, "~> 1.0.0", [hex: :blankable, repo: "hexpm", optional: false]}, {:recase, "~> 0.7.0", [hex: :recase, repo: "hexpm", optional: false]}], "hexpm", "d6ddedeb4d3a8602bc3f84e087a38f6150a86d9e790628ed8bc70e6d90681659"}, "hpax": {:hex, :hpax, "0.1.2", "09a75600d9d8bbd064cdd741f21fc06fc1f4cf3d0fcc335e5aa19be1a7235c84", [:mix], [], "hexpm", "2c87843d5a23f5f16748ebe77969880e29809580efdaccd615cd3bed628a8c13"}, "html_entities": {:hex, :html_entities, "0.5.2", "9e47e70598da7de2a9ff6af8758399251db6dbb7eebe2b013f2bbd2515895c3c", [:mix], [], "hexpm", "c53ba390403485615623b9531e97696f076ed415e8d8058b1dbaa28181f4fdcc"}, diff --git a/test/archethic/governance/code_test.exs b/test/archethic/governance/code_test.exs index 919cba949..ae377554c 100644 --- a/test/archethic/governance/code_test.exs +++ b/test/archethic/governance/code_test.exs @@ -30,5 +30,132 @@ defmodule Archethic.Governance.CodeTest do assert status in [:clean, :dirty] end + test "valid_appup? should pass" do + current_version = "1.0.7" + appup_version = version = "1.0.8" + + instruction = "load_module" + + changes = generate_diff(version, current_version, appup_version, instruction) + + assert Code.valid_appup?(changes, version, current_version) + end + + test "valid_appup? should fail because appup contains illegal instruction" do + current_version = "1.0.7" + appup_version = version = "1.0.8" + + instruction = "invalid" + + changes = generate_diff(version, current_version, appup_version, instruction) + + refute Code.valid_appup?(changes, version, current_version) + end + + test "valid_appup? should throw an error because appup contains illegal function calls" do + current_version = "1.0.7" + appup_version = version = "1.0.8" + + changes = generate_diff_with_function_call(version, current_version, appup_version) + + assert catch_throw(Code.valid_appup?(changes, version, current_version)) == + "Appup file contained calls to a function which is not permitted" + end + + test "valid_appup? should fail because appup contains wrong version" do + current_version = "1.0.7" + version = "1.0.8" + appup_version = "1.0.9" + instruction = "load_module" + + changes = generate_diff(version, current_version, appup_version, instruction) + + refute Code.valid_appup?(changes, version, current_version) + end + + test "valid_appup? should fail because it doens't contain appup" do + current_version = "1.0.7" + appup_version = version = "1.0.8" + instruction = "load_module" + + changes = generate_diff(version, current_version, appup_version, instruction, false) + + refute Code.valid_appup?(changes, version, current_version) + end + + defp generate_diff( + version, + current_version, + appup_version, + instruction, + should_add_appup? \\ true + ) do + base_appup(current_version, version) <> + maybe_add_appup(current_version, appup_version, instruction, should_add_appup?) <> + end_base_appup() + end + + defp generate_diff_with_function_call( + version, + current_version, + appup_version + ) do + base_appup(current_version, version) <> + appup_with_function_call(current_version, appup_version) <> + end_base_appup() + end + + defp base_appup(current_version, version) do + "diff --git a/mix.exs b/mix.exs\n" <> + "index c615ed94..d16e2d6f 100644\n" <> + "--- a/mix.exs\n" <> + "+++ b/mix.exs\n" <> + "@@ -4,7 +4,7 @@ defmodule Archethic.MixProject do\n" <> + " def project do\n" <> + " [\n" <> + " app: :archethic,\n" <> + "- version: \"#{current_version}\",\n" <> + "+ version: \"#{version}\",\n" <> + " build_path: \"_build\",\n" <> + " config_path: \"config/config.exs\",\n" <> + " deps_path: \"deps\",\n" + end + + defp end_base_appup() do + "\\ No newline at end of file" + end + + defp appup_with_function_call(current_version, appup_version) do + "diff --git a/rel/appups/archethic/1.0.7_to_1.0.8.appup b/rel/appups/archethic/1.0.7_to_1.0.8.appup\n" <> + "new file mode 100644\n" <> + "index 00000000..0f18b8e1\n" <> + "--- /dev/null\n" <> + "+++ b/rel/appups/archethic/1.0.7_to_1.0.8.appup\n" <> + "@@ -0,0 +1,4 @@\n" <> + "+{\"#{appup_version}\",\n" <> + "+ [{\"#{current_version}\",\n" <> + "+ [{load_module, io:format(\"execute some code\"), []}]}],\n" <> + "+ [{\"#{current_version}\",\n" <> + "+ [{load_module, 'TOTO', []}]}]\n" <> + "+}.\n" + end + + defp maybe_add_appup(_, _, _, false), do: "" + + defp maybe_add_appup(current_version, appup_version, instruction, true) do + "diff --git a/rel/appups/archethic/1.0.7_to_1.0.8.appup b/rel/appups/archethic/1.0.7_to_1.0.8.appup\n" <> + "new file mode 100644\n" <> + "index 00000000..0f18b8e1\n" <> + "--- /dev/null\n" <> + "+++ b/rel/appups/archethic/1.0.7_to_1.0.8.appup\n" <> + "@@ -0,0 +1,4 @@\n" <> + "+{\"#{appup_version}\",\n" <> + "+ [{\"#{current_version}\",\n" <> + "+ [{#{instruction},'TOTO', []}]}],\n" <> + "+ [{\"#{current_version}\",\n" <> + "+ [{#{instruction},'TOTO', []}]}]\n" <> + "+}.\n" + end + doctest Code end