diff --git a/guides/migration_guide.md b/guides/migration_guide.md index 4bdfdbe..a910f0a 100644 --- a/guides/migration_guide.md +++ b/guides/migration_guide.md @@ -158,6 +158,21 @@ config :bandera, ] ``` +> **⚠ Boolean gate compatibility note.** FunWithFlags stored boolean gates with +> a different internal sentinel than Bandera uses. If any flags had their boolean +> state set while FunWithFlags was active, your table may have duplicate boolean +> gate rows that cause the toggle to appear stuck. Run the one-time cleanup +> migration after switching: +> +> ``` +> mix bandera.gen.fix_fun_with_flags_migration +> mix ecto.migrate +> ``` +> +> This generates and runs a migration that normalises any legacy rows. It is safe +> to run on a table that has already been fully migrated — it finds nothing to +> change. + **Option B: create a fresh Bandera table** and copy your rows over. Generate the table from a migration: diff --git a/lib/bandera/ecto/migrations.ex b/lib/bandera/ecto/migrations.ex index 89357e4..12887e1 100644 --- a/lib/bandera/ecto/migrations.ex +++ b/lib/bandera/ecto/migrations.ex @@ -65,6 +65,55 @@ if Code.ensure_loaded?(Ecto.Migration) do :ok end + @doc """ + Fixes boolean gate rows left by a FunWithFlags-to-Bandera migration. + + FunWithFlags stored boolean gates with a legacy `target` value; Bandera uses + `"_bandera_none"`. Because the Ecto adapter's upsert conflict target is + `(flag_name, gate_type, target)`, toggling a flag via Bandera after migration + inserts a second boolean row rather than updating the legacy one. This leaves + two rows with contradictory `enabled` values, causing the dashboard toggle and + summary to disagree and making the toggle appear broken. + + Run once from a migration after switching to Bandera: + + defmodule MyApp.Repo.Migrations.FixFunWithFlagsBooleanGates do + use Ecto.Migration + def up, do: Bandera.Ecto.Migrations.fix_fun_with_flags_boolean_gates() + def down, do: :ok + end + + Safe to run on a database that has already been fully migrated — it will find + nothing to change. Not reversible (`down` should be a no-op). + """ + @spec fix_fun_with_flags_boolean_gates() :: :ok + def fix_fun_with_flags_boolean_gates do + table = Bandera.Config.ecto_table_name() + # `table` is developer-controlled config; '_bandera_none' is a fixed sentinel. + + # When both a legacy row and a Bandera row exist for the same flag, delete + # the legacy row. The Bandera row reflects the most recent intent. + execute(""" + DELETE FROM #{table} + WHERE gate_type = 'boolean' + AND target != '_bandera_none' + AND flag_name IN ( + SELECT flag_name FROM #{table} + WHERE gate_type = 'boolean' AND target = '_bandera_none' + ) + """) + + # Rename any remaining legacy rows so future Bandera writes upsert correctly. + execute(""" + UPDATE #{table} + SET target = '_bandera_none' + WHERE gate_type = 'boolean' + AND target != '_bandera_none' + """) + + :ok + end + @doc "Drops the flags table. Call from the `down/0` of your own migration." @spec down() :: :ok def down do diff --git a/lib/bandera/store/persistent/ecto.ex b/lib/bandera/store/persistent/ecto.ex index ed6de23..68ca198 100644 --- a/lib/bandera/store/persistent/ecto.ex +++ b/lib/bandera/store/persistent/ecto.ex @@ -73,6 +73,26 @@ if Code.ensure_loaded?(Ecto.Adapters.SQL) do end end + @impl Bandera.Store.Persistent + def put(flag_name, %Gate{type: :boolean} = gate) do + # Delete any existing boolean row first — regardless of the `target` value + # stored there. Without this, migrating from FunWithFlags (which used + # `target = "boolean"`) leaves a stale row alongside Bandera's + # `target = "_bandera_none"` row, because the upsert conflict target is + # `[:flag_name, :gate_type, :target]` and the two targets differ. + name = to_string(flag_name) + row = Serializer.to_row(flag_name, gate) + + repo().delete_all( + from(r in {table(), Record}, + where: r.flag_name == ^name and r.gate_type == "boolean" + ) + ) + + repo().insert_all({table(), Record}, [row]) + get(flag_name) + end + @impl Bandera.Store.Persistent def put(flag_name, %Gate{} = gate) do row = Serializer.to_row(flag_name, gate) diff --git a/lib/mix/tasks/bandera.gen.fix_fun_with_flags_migration.ex b/lib/mix/tasks/bandera.gen.fix_fun_with_flags_migration.ex new file mode 100644 index 0000000..21807dc --- /dev/null +++ b/lib/mix/tasks/bandera.gen.fix_fun_with_flags_migration.ex @@ -0,0 +1,63 @@ +defmodule Mix.Tasks.Bandera.Gen.FixFunWithFlagsMigration do + @moduledoc """ + Generates an Ecto migration that fixes duplicate boolean gate rows left by a + FunWithFlags-to-Bandera migration. + + ## Usage + + mix bandera.gen.fix_fun_with_flags_migration [--path PATH] + + ## Options + + * `--path` — directory to write the migration file into. + Defaults to `priv/repo/migrations`. + + ## What it generates + + defmodule MyApp.Repo.Migrations.FixFunWithFlagsBooleanGates do + use Ecto.Migration + def up, do: Bandera.Ecto.Migrations.fix_fun_with_flags_boolean_gates() + def down, do: :ok + end + + Run `mix ecto.migrate` after generating the file. + """ + @shortdoc "Generate a migration to fix FunWithFlags boolean gate rows" + use Mix.Task + + @default_path "priv/repo/migrations" + + @impl Mix.Task + def run(args) do + {opts, _, _} = OptionParser.parse(args, strict: [path: :string]) + path = Keyword.get(opts, :path, @default_path) + abs_path = Path.expand(path) + + File.mkdir_p!(abs_path) + + filename = "#{timestamp()}_fix_fun_with_flags_boolean_gates.exs" + filepath = Path.join(abs_path, filename) + + File.write!(filepath, migration_content()) + Mix.shell().info("Generated migration: #{filepath}") + end + + defp timestamp do + {{y, m, d}, {hh, mm, ss}} = :calendar.universal_time() + "#{y}#{pad(m)}#{pad(d)}#{pad(hh)}#{pad(mm)}#{pad(ss)}" + end + + defp pad(i) when i < 10, do: <> + defp pad(i), do: to_string(i) + + defp migration_content do + """ + defmodule MyApp.Repo.Migrations.FixFunWithFlagsBooleanGates do + use Ecto.Migration + + def up, do: Bandera.Ecto.Migrations.fix_fun_with_flags_boolean_gates() + def down, do: :ok + end + """ + end +end diff --git a/test/bandera/ecto/migrations_test.exs b/test/bandera/ecto/migrations_test.exs index 58ff97d..adf3de5 100644 --- a/test/bandera/ecto/migrations_test.exs +++ b/test/bandera/ecto/migrations_test.exs @@ -75,4 +75,97 @@ defmodule Bandera.Ecto.MigrationsTest do assert "value" in column_names(table) end + + defmodule FixFunWithFlagsMigration do + use Ecto.Migration + + @spec up() :: :ok + def up, do: Bandera.Ecto.Migrations.fix_fun_with_flags_boolean_gates() + end + + describe "fix_fun_with_flags_boolean_gates/0" do + setup do + Bandera.TestRepo.query!("DELETE FROM schema_migrations WHERE version = 20260601000001") + + on_exit(fn -> + Bandera.TestRepo.query!("DELETE FROM schema_migrations WHERE version = 20260601000001") + end) + end + + test "renames a lone legacy boolean row to _bandera_none" do + Bandera.TestRepo.query!(""" + INSERT INTO bandera_flags (flag_name, gate_type, target, enabled, value) + VALUES ('my_flag', 'boolean', 'boolean', true, NULL) + """) + + run_fix_migration() + + %{rows: rows} = + Bandera.TestRepo.query!( + "SELECT target, enabled FROM bandera_flags WHERE flag_name='my_flag' AND gate_type='boolean'" + ) + + assert rows == [["_bandera_none", 1]] + end + + test "deletes legacy row when a Bandera row already exists, keeping Bandera row's enabled value" do + Bandera.TestRepo.query!(""" + INSERT INTO bandera_flags (flag_name, gate_type, target, enabled, value) + VALUES ('my_flag', 'boolean', 'boolean', true, NULL), + ('my_flag', 'boolean', '_bandera_none', false, NULL) + """) + + run_fix_migration() + + %{rows: rows} = + Bandera.TestRepo.query!( + "SELECT target, enabled FROM bandera_flags WHERE flag_name='my_flag' AND gate_type='boolean'" + ) + + assert rows == [["_bandera_none", 0]] + end + + test "does not touch actor or group rows" do + Bandera.TestRepo.query!(""" + INSERT INTO bandera_flags (flag_name, gate_type, target, enabled, value) + VALUES ('my_flag', 'boolean', 'boolean', true, NULL), + ('my_flag', 'actor', 'user:1', true, NULL) + """) + + run_fix_migration() + + %{rows: rows} = + Bandera.TestRepo.query!( + "SELECT gate_type, target FROM bandera_flags WHERE flag_name='my_flag' ORDER BY gate_type" + ) + + assert rows == [["actor", "user:1"], ["boolean", "_bandera_none"]] + end + + test "is a no-op when all boolean rows already use _bandera_none" do + Bandera.TestRepo.query!(""" + INSERT INTO bandera_flags (flag_name, gate_type, target, enabled, value) + VALUES ('my_flag', 'boolean', '_bandera_none', true, NULL) + """) + + run_fix_migration() + + %{rows: rows} = + Bandera.TestRepo.query!( + "SELECT target, enabled FROM bandera_flags WHERE flag_name='my_flag' AND gate_type='boolean'" + ) + + assert rows == [["_bandera_none", 1]] + end + + defp run_fix_migration do + Ecto.Migrator.run( + Bandera.TestRepo, + [{20_260_601_000_001, FixFunWithFlagsMigration}], + :up, + all: true, + log: false + ) + end + end end diff --git a/test/mix/tasks/bandera_gen_fix_fun_with_flags_migration_test.exs b/test/mix/tasks/bandera_gen_fix_fun_with_flags_migration_test.exs new file mode 100644 index 0000000..afe2d0e --- /dev/null +++ b/test/mix/tasks/bandera_gen_fix_fun_with_flags_migration_test.exs @@ -0,0 +1,59 @@ +defmodule Mix.Tasks.Bandera.Gen.FixFunWithFlagsMigrationTest do + use ExUnit.Case, async: true + import ExUnit.CaptureIO + + @task Mix.Tasks.Bandera.Gen.FixFunWithFlagsMigration + + setup do + path = + Path.join( + System.tmp_dir!(), + "bandera_test_migrations_#{:erlang.unique_integer([:positive])}" + ) + + File.mkdir_p!(path) + on_exit(fn -> File.rm_rf!(path) end) + {:ok, path: path} + end + + test "generates a migration file in the given path", %{path: path} do + capture_io(fn -> @task.run(["--path", path]) end) + + files = File.ls!(path) + assert length(files) == 1 + assert hd(files) =~ ~r/^\d{14}_fix_fun_with_flags_boolean_gates\.exs$/ + end + + test "generated file contains the correct module and helper call", %{path: path} do + capture_io(fn -> @task.run(["--path", path]) end) + + content = path |> File.ls!() |> hd() |> then(&File.read!(Path.join(path, &1))) + + assert content =~ "use Ecto.Migration" + assert content =~ "Bandera.Ecto.Migrations.fix_fun_with_flags_boolean_gates()" + assert content =~ "def up" + assert content =~ "def down, do: :ok" + end + + test "prints the path of the generated file", %{path: path} do + output = capture_io(fn -> @task.run(["--path", path]) end) + + assert output =~ path + assert output =~ "_fix_fun_with_flags_boolean_gates.exs" + end + + test "defaults to priv/repo/migrations when no --path given" do + default_path = Path.join(File.cwd!(), "priv/repo/migrations") + + on_exit(fn -> + File.ls!(default_path) + |> Enum.filter(&(&1 =~ "fix_fun_with_flags")) + |> Enum.each(&File.rm!(Path.join(default_path, &1))) + end) + + output = capture_io(fn -> @task.run([]) end) + + assert output =~ default_path + assert output =~ "_fix_fun_with_flags_boolean_gates.exs" + end +end