From 6b5d1b2924a9afdcc24658f1d48f807b6096e1df Mon Sep 17 00:00:00 2001 From: Peter Mueller <6015288+petermueller@users.noreply.github.com> Date: Thu, 11 Jul 2024 01:54:01 -0400 Subject: [PATCH 1/4] allow `:constraint_handler` option defaults to existing adapter connection module --- integration_test/myxql/constraints_test.exs | 186 +++++++++++++++++++- integration_test/myxql/test_helper.exs | 7 +- lib/ecto/adapters/myxql.ex | 2 +- lib/ecto/adapters/sql.ex | 12 +- 4 files changed, 194 insertions(+), 13 deletions(-) diff --git a/integration_test/myxql/constraints_test.exs b/integration_test/myxql/constraints_test.exs index 2323823f..00cc0568 100644 --- a/integration_test/myxql/constraints_test.exs +++ b/integration_test/myxql/constraints_test.exs @@ -4,6 +4,29 @@ defmodule Ecto.Integration.ConstraintsTest do import Ecto.Migrator, only: [up: 4] alias Ecto.Integration.PoolRepo + defmodule CustomConstraintHandler do + @quotes ~w(" ' `) + + # An example of a custom handler a user might write + def to_constraints(%MyXQL.Error{mysql: %{name: :ER_SIGNAL_EXCEPTION}, message: message}, opts) do + # Assumes this is the only use-case of `ER_SIGNAL_EXCEPTION` the user has implemented custom errors for + with [_, quoted] <- :binary.split(message, "Overlapping values for key "), + [_, index | _] <- :binary.split(quoted, @quotes, [:global]) do + [exclusion: strip_source(index, opts[:source])] + else + _ -> [] + end + end + + def to_constraints(err, opts) do + # Falls back to default `ecto_sql` handler for all others + Ecto.Adapters.MyXQL.Connection.to_constraints(err, opts) + end + + defp strip_source(name, nil), do: name + defp strip_source(name, source), do: String.trim_leading(name, "#{source}.") + end + defmodule ConstraintMigration do use Ecto.Migration @@ -21,6 +44,50 @@ defmodule Ecto.Integration.ConstraintsTest do end end + defmodule ProcedureEmulatingConstraintMigration do + use Ecto.Migration + + @table_name :constraints_test + + def up do + insert_trigger_sql = trigger_sql(@table_name, "INSERT") + update_trigger_sql = trigger_sql(@table_name, "UPDATE") + + drop_triggers(@table_name) + repo().query!(insert_trigger_sql) + repo().query!(update_trigger_sql) + end + + def down do + drop_triggers(@table_name) + end + + defp trigger_sql(table_name, before_type) do + ~s""" + CREATE TRIGGER #{table_name}_#{String.downcase(before_type)}_overlap + BEFORE #{String.upcase(before_type)} + ON #{table_name} FOR EACH ROW + BEGIN + DECLARE v_rowcount INT; + DECLARE v_msg VARCHAR(200); + + SELECT COUNT(*) INTO v_rowcount FROM #{table_name} + WHERE (NEW.from <= `to` AND NEW.to >= `from`); + + IF v_rowcount > 0 THEN + SET v_msg = CONCAT('Overlapping values for key \\'#{table_name}.cannot_overlap\\''); + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = v_msg, MYSQL_ERRNO = 1644; + END IF; + END; + """ + end + + defp drop_triggers(table_name) do + repo().query!("DROP TRIGGER IF EXISTS #{table_name}_insert_overlap") + repo().query!("DROP TRIGGER IF EXISTS #{table_name}_update_overlap") + end + end + defmodule Constraint do use Ecto.Integration.Schema @@ -31,12 +98,23 @@ defmodule Ecto.Integration.ConstraintsTest do end end + defmodule CustomConstraint do + use Ecto.Integration.Schema + + schema "procedure_constraints_test" do + field :member_id, :integer + field :started_at, :utc_datetime_usec + field :ended_at, :utc_datetime_usec + end + end + @base_migration 2_000_000 setup_all do ExUnit.CaptureLog.capture_log(fn -> num = @base_migration + System.unique_integer([:positive]) up(PoolRepo, num, ConstraintMigration, log: false) + up(PoolRepo, num + 1, ProcedureEmulatingConstraintMigration, log: false) end) :ok @@ -46,10 +124,13 @@ defmodule Ecto.Integration.ConstraintsTest do test "check constraint" do # When the changeset doesn't expect the db error changeset = Ecto.Changeset.change(%Constraint{}, price: -10) + exception = - assert_raise Ecto.ConstraintError, ~r/constraint error when attempting to insert struct/, fn -> - PoolRepo.insert(changeset) - end + assert_raise Ecto.ConstraintError, + ~r/constraint error when attempting to insert struct/, + fn -> + PoolRepo.insert(changeset) + end assert exception.message =~ "\"positive_price\" (check_constraint)" assert exception.message =~ "The changeset has not defined any constraint." @@ -60,24 +141,111 @@ defmodule Ecto.Integration.ConstraintsTest do changeset |> Ecto.Changeset.check_constraint(:price, name: :positive_price) |> PoolRepo.insert() - assert changeset.errors == [price: {"is invalid", [constraint: :check, constraint_name: "positive_price"]}] + + assert changeset.errors == [ + price: {"is invalid", [constraint: :check, constraint_name: "positive_price"]} + ] + assert changeset.data.__meta__.state == :built # When the changeset does expect the db error and gives a custom message changeset = Ecto.Changeset.change(%Constraint{}, price: -10) + {:error, changeset} = changeset - |> Ecto.Changeset.check_constraint(:price, name: :positive_price, message: "price must be greater than 0") + |> Ecto.Changeset.check_constraint(:price, + name: :positive_price, + message: "price must be greater than 0" + ) |> PoolRepo.insert() - assert changeset.errors == [price: {"price must be greater than 0", [constraint: :check, constraint_name: "positive_price"]}] + + assert changeset.errors == [ + price: + {"price must be greater than 0", + [constraint: :check, constraint_name: "positive_price"]} + ] + assert changeset.data.__meta__.state == :built # When the change does not violate the check constraint changeset = Ecto.Changeset.change(%Constraint{}, price: 10, from: 100, to: 200) - {:ok, changeset} = + + {:ok, result} = changeset - |> Ecto.Changeset.check_constraint(:price, name: :positive_price, message: "price must be greater than 0") + |> Ecto.Changeset.check_constraint(:price, + name: :positive_price, + message: "price must be greater than 0" + ) + |> PoolRepo.insert() + + assert is_integer(result.id) + end + + test "custom handled constraint" do + changeset = Ecto.Changeset.change(%Constraint{}, from: 0, to: 10) + {:ok, item} = PoolRepo.insert(changeset) + + non_overlapping_changeset = Ecto.Changeset.change(%Constraint{}, from: 11, to: 12) + {:ok, _} = PoolRepo.insert(non_overlapping_changeset) + + overlapping_changeset = Ecto.Changeset.change(%Constraint{}, from: 9, to: 12) + + msg_re = ~r/constraint error when attempting to insert struct/ + + # When the changeset doesn't expect the db error + exception = + assert_raise Ecto.ConstraintError, msg_re, fn -> PoolRepo.insert(overlapping_changeset) end + + assert exception.message =~ "\"cannot_overlap\" (exclusion_constraint)" + assert exception.message =~ "The changeset has not defined any constraint." + assert exception.message =~ "call `exclusion_constraint/3`" + + ##### + + # When the changeset does expect the db error + # but the key does not match the default generated by `exclusion_constraint` + exception = + assert_raise Ecto.ConstraintError, msg_re, fn -> + overlapping_changeset + |> Ecto.Changeset.exclusion_constraint(:from) + |> PoolRepo.insert() + end + assert exception.message =~ "\"cannot_overlap\" (exclusion_constraint)" + + # When the changeset does expect the db error, but doesn't give a custom message + {:error, changeset} = + overlapping_changeset + |> Ecto.Changeset.exclusion_constraint(:from, name: :cannot_overlap) + |> PoolRepo.insert() + assert changeset.errors == [from: {"violates an exclusion constraint", [constraint: :exclusion, constraint_name: "cannot_overlap"]}] + assert changeset.data.__meta__.state == :built + + # When the changeset does expect the db error and gives a custom message + {:error, changeset} = + overlapping_changeset + |> Ecto.Changeset.exclusion_constraint(:from, name: :cannot_overlap, message: "must not overlap") + |> PoolRepo.insert() + assert changeset.errors == [from: {"must not overlap", [constraint: :exclusion, constraint_name: "cannot_overlap"]}] + assert changeset.data.__meta__.state == :built + + + # When the changeset does expect the db error, but a different handler is used + exception = + assert_raise MyXQL.Error, fn -> + overlapping_changeset + |> Ecto.Changeset.exclusion_constraint(:from, name: :cannot_overlap) + |> PoolRepo.insert(constraint_handler: Ecto.Adapters.MyXQL.Connection) + end + assert exception.message =~ "Overlapping values for key 'constraints_test.cannot_overlap'" + + # When custom error is coming from an UPDATE + overlapping_update_changeset = Ecto.Changeset.change(item, from: 0, to: 9) + + {:error, changeset} = + overlapping_update_changeset + |> Ecto.Changeset.exclusion_constraint(:from, name: :cannot_overlap, message: "must not overlap") |> PoolRepo.insert() - assert is_integer(changeset.id) + assert changeset.errors == [from: {"must not overlap", [constraint: :exclusion, constraint_name: "cannot_overlap"]}] + assert changeset.data.__meta__.state == :loaded end end diff --git a/integration_test/myxql/test_helper.exs b/integration_test/myxql/test_helper.exs index 922f4d68..5fecdc42 100644 --- a/integration_test/myxql/test_helper.exs +++ b/integration_test/myxql/test_helper.exs @@ -57,7 +57,9 @@ Application.put_env(:ecto_sql, PoolRepo, url: Application.get_env(:ecto_sql, :mysql_test_url) <> "/ecto_test", pool_size: 5, pool_count: String.to_integer(System.get_env("POOL_COUNT", "1")), - show_sensitive_data_on_connection_error: true + show_sensitive_data_on_connection_error: true, + # Passes through into adapter_meta + constraint_handler: Ecto.Integration.ConstraintsTest.CustomConstraintHandler ) defmodule Ecto.Integration.PoolRepo do @@ -84,6 +86,9 @@ _ = Ecto.Adapters.MyXQL.storage_down(TestRepo.config()) :ok = Ecto.Adapters.MyXQL.storage_up(TestRepo.config()) {:ok, _pid} = TestRepo.start_link() + +# Passes through into adapter_meta, overrides Application config +# {:ok, _pid} = PoolRepo.start_link([constraint_handler: Ecto.Integration.ConstraintsTest.CustomConstraintHandler]) {:ok, _pid} = PoolRepo.start_link() %{rows: [[version]]} = TestRepo.query!("SELECT @@version", []) diff --git a/lib/ecto/adapters/myxql.ex b/lib/ecto/adapters/myxql.ex index 1c589aeb..4c02bcfe 100644 --- a/lib/ecto/adapters/myxql.ex +++ b/lib/ecto/adapters/myxql.ex @@ -314,7 +314,7 @@ defmodule Ecto.Adapters.MyXQL do {:ok, last_insert_id(key, last_insert_id)} {:error, err} -> - case @conn.to_constraints(err, source: source) do + case Ecto.Adapters.SQL.to_constraints(adapter_meta, opts, err, source: source) do [] -> raise err constraints -> {:invalid, constraints} end diff --git a/lib/ecto/adapters/sql.ex b/lib/ecto/adapters/sql.ex index ab43205c..ce021f6d 100644 --- a/lib/ecto/adapters/sql.ex +++ b/lib/ecto/adapters/sql.ex @@ -658,6 +658,12 @@ defmodule Ecto.Adapters.SQL do sql_call(adapter_meta, :query_many, [sql], params, opts) end + def to_constraints(adapter_meta, opts, err, err_opts) do + %{constraint_handler: constraint_handler} = adapter_meta + constraint_handler = Keyword.get(opts, :constraint_handler) || constraint_handler + constraint_handler.to_constraints(err, err_opts) + end + defp sql_call(adapter_meta, callback, args, params, opts) do %{pid: pool, telemetry: telemetry, sql: sql, opts: default_opts} = adapter_meta conn = get_conn_or_pool(pool, adapter_meta) @@ -872,6 +878,7 @@ defmodule Ecto.Adapters.SQL do """ end + constraint_handler = Keyword.get(config, :constraint_handler, connection) stacktrace = Keyword.get(config, :stacktrace, nil) telemetry_prefix = Keyword.fetch!(config, :telemetry_prefix) telemetry = {config[:repo], log, telemetry_prefix ++ [:query]} @@ -884,6 +891,7 @@ defmodule Ecto.Adapters.SQL do meta = %{ telemetry: telemetry, sql: connection, + constraint_handler: constraint_handler, stacktrace: stacktrace, opts: Keyword.take(config, @pool_opts) } @@ -1130,7 +1138,7 @@ defmodule Ecto.Adapters.SQL do @doc false def struct( adapter_meta, - conn, + _conn, sql, operation, source, @@ -1165,7 +1173,7 @@ defmodule Ecto.Adapters.SQL do operation: operation {:error, err} -> - case conn.to_constraints(err, source: source) do + case to_constraints(adapter_meta, opts, err, source: source) do [] -> raise_sql_call_error(err) constraints -> {:invalid, constraints} end From 14a73acde7caf779e4961a751bf72716ecdc25cf Mon Sep 17 00:00:00 2001 From: Peter Mueller <6015288+petermueller@users.noreply.github.com> Date: Sun, 28 Jul 2024 17:05:47 -0400 Subject: [PATCH 2/4] don't fail MySQL 5.x constraint tests --- integration_test/myxql/constraints_test.exs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/integration_test/myxql/constraints_test.exs b/integration_test/myxql/constraints_test.exs index 00cc0568..2df1c08b 100644 --- a/integration_test/myxql/constraints_test.exs +++ b/integration_test/myxql/constraints_test.exs @@ -27,7 +27,7 @@ defmodule Ecto.Integration.ConstraintsTest do defp strip_source(name, source), do: String.trim_leading(name, "#{source}.") end - defmodule ConstraintMigration do + defmodule ConstraintTableMigration do use Ecto.Migration @table table(:constraints_test) @@ -38,7 +38,15 @@ defmodule Ecto.Integration.ConstraintsTest do add :from, :integer add :to, :integer end + end + end + + defmodule ConstraintMigration do + use Ecto.Migration + @table table(:constraints_test) + + def change do # Only valid after MySQL 8.0.19 create constraint(@table.name, :positive_price, check: "price > 0") end @@ -113,7 +121,7 @@ defmodule Ecto.Integration.ConstraintsTest do setup_all do ExUnit.CaptureLog.capture_log(fn -> num = @base_migration + System.unique_integer([:positive]) - up(PoolRepo, num, ConstraintMigration, log: false) + up(PoolRepo, num, ConstraintTableMigration, log: false) up(PoolRepo, num + 1, ProcedureEmulatingConstraintMigration, log: false) end) @@ -122,6 +130,11 @@ defmodule Ecto.Integration.ConstraintsTest do @tag :create_constraint test "check constraint" do + num = @base_migration + System.unique_integer([:positive]) + ExUnit.CaptureLog.capture_log(fn -> + :ok = up(PoolRepo, num, ConstraintMigration, log: false) + end) + # When the changeset doesn't expect the db error changeset = Ecto.Changeset.change(%Constraint{}, price: -10) From c1446c237f8491ff2f11aac92835a87b87b02a99 Mon Sep 17 00:00:00 2001 From: Peter Mueller <6015288+petermueller@users.noreply.github.com> Date: Sun, 28 Jul 2024 23:20:32 -0400 Subject: [PATCH 3/4] add `Ecto.Adapters.SQL.Constraint` --- lib/ecto/adapters/sql.ex | 1 + lib/ecto/adapters/sql/constraint.ex | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 lib/ecto/adapters/sql/constraint.ex diff --git a/lib/ecto/adapters/sql.ex b/lib/ecto/adapters/sql.ex index ce021f6d..cf6f9961 100644 --- a/lib/ecto/adapters/sql.ex +++ b/lib/ecto/adapters/sql.ex @@ -658,6 +658,7 @@ defmodule Ecto.Adapters.SQL do sql_call(adapter_meta, :query_many, [sql], params, opts) end + @doc false def to_constraints(adapter_meta, opts, err, err_opts) do %{constraint_handler: constraint_handler} = adapter_meta constraint_handler = Keyword.get(opts, :constraint_handler) || constraint_handler diff --git a/lib/ecto/adapters/sql/constraint.ex b/lib/ecto/adapters/sql/constraint.ex new file mode 100644 index 00000000..6e52524b --- /dev/null +++ b/lib/ecto/adapters/sql/constraint.ex @@ -0,0 +1,19 @@ +defmodule Ecto.Adapters.SQL.Constraint do + @moduledoc """ + Specifies the constraint handling API + """ + + @doc """ + Receives the exception returned by `c:Ecto.Adapters.SQL.Connection.query/4`. + + The constraints are in the keyword list and must return the + constraint type, like `:unique`, and the constraint name as + a string, for example: + + [unique: "posts_title_index"] + + Must return an empty list if the error does not come + from any constraint. + """ + @callback to_constraints(exception :: Exception.t(), options :: Keyword.t()) :: Keyword.t() +end From 4f80046ef965d6d48a7725ae418d74002ca92e7e Mon Sep 17 00:00:00 2001 From: Peter Mueller <6015288+petermueller@users.noreply.github.com> Date: Mon, 29 Jul 2024 00:45:59 -0400 Subject: [PATCH 4/4] remove unused schema from constraint test --- integration_test/myxql/constraints_test.exs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/integration_test/myxql/constraints_test.exs b/integration_test/myxql/constraints_test.exs index 2df1c08b..0650a04a 100644 --- a/integration_test/myxql/constraints_test.exs +++ b/integration_test/myxql/constraints_test.exs @@ -106,16 +106,6 @@ defmodule Ecto.Integration.ConstraintsTest do end end - defmodule CustomConstraint do - use Ecto.Integration.Schema - - schema "procedure_constraints_test" do - field :member_id, :integer - field :started_at, :utc_datetime_usec - field :ended_at, :utc_datetime_usec - end - end - @base_migration 2_000_000 setup_all do @@ -213,8 +203,6 @@ defmodule Ecto.Integration.ConstraintsTest do assert exception.message =~ "The changeset has not defined any constraint." assert exception.message =~ "call `exclusion_constraint/3`" - ##### - # When the changeset does expect the db error # but the key does not match the default generated by `exclusion_constraint` exception =