From b2c9c98b42c166400ff9147fdc758d35f20fbcca Mon Sep 17 00:00:00 2001 From: Peter Mueller <6015288+petermueller@users.noreply.github.com> Date: Sat, 6 Jul 2024 06:44:48 -0400 Subject: [PATCH] add check constraints for MySQL (#621) --- integration_test/myxql/constraints_test.exs | 83 ++++++++++++++++++++ integration_test/myxql/test_helper.exs | 2 +- lib/ecto/adapters/myxql/connection.ex | 59 ++++++++++++-- lib/ecto/migration.ex | 29 ++++++- mix.exs | 2 +- mix.lock | 2 +- test/ecto/adapters/myxql_test.exs | 86 ++++++++++++++------- 7 files changed, 223 insertions(+), 40 deletions(-) create mode 100644 integration_test/myxql/constraints_test.exs diff --git a/integration_test/myxql/constraints_test.exs b/integration_test/myxql/constraints_test.exs new file mode 100644 index 00000000..2323823f --- /dev/null +++ b/integration_test/myxql/constraints_test.exs @@ -0,0 +1,83 @@ +defmodule Ecto.Integration.ConstraintsTest do + use ExUnit.Case, async: true + + import Ecto.Migrator, only: [up: 4] + alias Ecto.Integration.PoolRepo + + defmodule ConstraintMigration do + use Ecto.Migration + + @table table(:constraints_test) + + def change do + create @table do + add :price, :integer + add :from, :integer + add :to, :integer + end + + # Only valid after MySQL 8.0.19 + create constraint(@table.name, :positive_price, check: "price > 0") + end + end + + defmodule Constraint do + use Ecto.Integration.Schema + + schema "constraints_test" do + field :price, :integer + field :from, :integer + field :to, :integer + 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) + end) + + :ok + end + + @tag :create_constraint + 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 exception.message =~ "\"positive_price\" (check_constraint)" + assert exception.message =~ "The changeset has not defined any constraint." + assert exception.message =~ "call `check_constraint/3`" + + # When the changeset does expect the db error, but doesn't give a custom message + {:error, changeset} = + 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.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") + |> PoolRepo.insert() + 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} = + changeset + |> Ecto.Changeset.check_constraint(:price, name: :positive_price, message: "price must be greater than 0") + |> PoolRepo.insert() + assert is_integer(changeset.id) + end +end diff --git a/integration_test/myxql/test_helper.exs b/integration_test/myxql/test_helper.exs index fb966982..4bede7b8 100644 --- a/integration_test/myxql/test_helper.exs +++ b/integration_test/myxql/test_helper.exs @@ -121,7 +121,7 @@ excludes = [ if Version.match?(version, ">= 8.0.0") do ExUnit.configure(exclude: excludes) else - ExUnit.configure(exclude: [:values_list, :rename_column | excludes]) + ExUnit.configure(exclude: [:create_constraint, :values_list, :rename_column | excludes]) end :ok = Ecto.Migrator.up(TestRepo, 0, Ecto.Integration.Migration, log: false) diff --git a/lib/ecto/adapters/myxql/connection.ex b/lib/ecto/adapters/myxql/connection.ex index 31b334e5..4f1fb57c 100644 --- a/lib/ecto/adapters/myxql/connection.ex +++ b/lib/ecto/adapters/myxql/connection.ex @@ -78,6 +78,18 @@ if Code.ensure_loaded?(MyXQL) do end end + def to_constraints( + %MyXQL.Error{mysql: %{name: :ER_CHECK_CONSTRAINT_VIOLATED}, message: message}, + _opts + ) do + with [_, quoted] <- :binary.split(message, ["Check constraint "]), + [_, constraint | _] <- :binary.split(quoted, @quotes, [:global]) do + [check: constraint] + else + _ -> [] + end + end + def to_constraints(_, _), do: [] @@ -1026,12 +1038,31 @@ if Code.ensure_loaded?(MyXQL) do def execute_ddl({:create_if_not_exists, %Index{}}), do: error!(nil, "MySQL adapter does not support create if not exists for index") - def execute_ddl({:create, %Constraint{check: check}}) when is_binary(check), - do: error!(nil, "MySQL adapter does not support check constraints") + def execute_ddl({:create, %Constraint{check: check} = constraint}) when is_binary(check) do + table_name = quote_name(constraint.prefix, constraint.table) + [["ALTER TABLE ", table_name, " ADD ", new_constraint_expr(constraint)]] + end def execute_ddl({:create, %Constraint{exclude: exclude}}) when is_binary(exclude), do: error!(nil, "MySQL adapter does not support exclusion constraints") + def execute_ddl({:drop, %Constraint{}, :cascade}), + do: error!(nil, "MySQL does not support `CASCADE` in `DROP CONSTRAINT` commands") + + def execute_ddl({:drop, %Constraint{} = constraint, _}) do + [ + [ + "ALTER TABLE ", + quote_name(constraint.prefix, constraint.table), + " DROP CONSTRAINT ", + quote_name(constraint.name) + ] + ] + end + + def execute_ddl({:drop_if_exists, %Constraint{}, _}), + do: error!(nil, "MySQL adapter does not support `drop_if_exists` for constraints") + def execute_ddl({:drop, %Index{}, :cascade}), do: error!(nil, "MySQL adapter does not support cascade in drop index") @@ -1047,12 +1078,6 @@ if Code.ensure_loaded?(MyXQL) do ] end - def execute_ddl({:drop, %Constraint{}, _}), - do: error!(nil, "MySQL adapter does not support constraints") - - def execute_ddl({:drop_if_exists, %Constraint{}, _}), - do: error!(nil, "MySQL adapter does not support constraints") - def execute_ddl({:drop_if_exists, %Index{}, _}), do: error!(nil, "MySQL adapter does not support drop if exists for index") @@ -1244,6 +1269,17 @@ if Code.ensure_loaded?(MyXQL) do defp null_expr(true), do: " NULL" defp null_expr(_), do: [] + defp new_constraint_expr(%Constraint{check: check} = constraint) when is_binary(check) do + [ + "CONSTRAINT ", + quote_name(constraint.name), + " CHECK (", + check, + ")", + validate(constraint.validate) + ] + end + defp default_expr({:ok, nil}), do: " DEFAULT NULL" @@ -1401,6 +1437,9 @@ if Code.ensure_loaded?(MyXQL) do defp reference_on_update(:restrict), do: " ON UPDATE RESTRICT" defp reference_on_update(_), do: [] + defp validate(false), do: " NOT ENFORCED" + defp validate(_), do: [] + ## Helpers defp get_source(query, sources, ix, source) do @@ -1423,6 +1462,10 @@ if Code.ensure_loaded?(MyXQL) do defp maybe_add_column_names(_, name), do: name + defp quote_name(nil, name), do: quote_name(name) + + defp quote_name(prefix, name), do: [quote_name(prefix), ?., quote_name(name)] + defp quote_name(name) when is_atom(name) do quote_name(Atom.to_string(name)) end diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex index 52cdee91..5a7462c0 100644 --- a/lib/ecto/migration.ex +++ b/lib/ecto/migration.ex @@ -1491,11 +1491,34 @@ defmodule Ecto.Migration do * `:check` - A check constraint expression. Required when creating a check constraint. * `:exclude` - An exclusion constraint expression. Required when creating an exclusion constraint. * `:prefix` - The prefix for the table. - * `:validate` - Whether or not to validate the constraint on creation (true by default). Only - available in PostgreSQL, and should be followed by a command to validate the new constraint in - a following migration if false. + * `:validate` - Whether or not to validate the constraint on creation (true by default). See the section below for more information * `:comment` - adds a comment to the constraint. + + ## Using `validate: false` + + Validation/Enforcement of a constraint is enabled by default, but disabling on constraint + creation is supported by PostgreSQL, and MySQL, and can be done by setting `validate: false`. + + Setting `validate: false` as an option can be useful, as the creation of a constraint will cause + a full table scan to check existing rows. The constraint will still be enforced for subsequent + inserts and updates, but should then be updated in a following command or migration to enforce + the new constraint. + + Validating / Enforcing the constraint in a later command, or migration, can be done like so: + + ``` + def change do + # PostgreSQL +   execute "ALTER TABLE products VALIDATE CONSTRAINT price_must_be_positive", "" + + # MySQL +   execute "ALTER TABLE products ALTER CONSTRAINT price_must_be_positive ENFORCED", "" + end + ``` + + See the [Safe Ecto Migrations guide](https://fly.io/phoenix-files/safe-ecto-migrations/) for an + in-depth explanation of the benefits of this approach. """ def constraint(table, name, opts \\ []) diff --git a/mix.exs b/mix.exs index dd0c0cd3..a722130c 100644 --- a/mix.exs +++ b/mix.exs @@ -92,7 +92,7 @@ defmodule EctoSQL.MixProject do if path = System.get_env("MYXQL_PATH") do {:myxql, path: path} else - {:myxql, "~> 0.6", optional: true} + {:myxql, "~> 0.7", optional: true} end end diff --git a/mix.lock b/mix.lock index fb853ec5..2daca590 100644 --- a/mix.lock +++ b/mix.lock @@ -10,7 +10,7 @@ "makeup": {:hex, :makeup, "1.1.2", "9ba8837913bdf757787e71c1581c21f9d2455f4dd04cfca785c70bbfff1a76a3", [:mix], [{:nimble_parsec, "~> 1.2.2 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "cce1566b81fbcbd21eca8ffe808f33b221f9eee2cbc7a1706fc3da9ff18e6cac"}, "makeup_elixir": {:hex, :makeup_elixir, "0.16.2", "627e84b8e8bf22e60a2579dad15067c755531fea049ae26ef1020cad58fe9578", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.2.3 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "41193978704763f6bbe6cc2758b84909e62984c7752b3784bd3c218bb341706b"}, "makeup_erlang": {:hex, :makeup_erlang, "1.0.0", "6f0eff9c9c489f26b69b61440bf1b238d95badae49adac77973cbacae87e3c2e", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "ea7a9307de9d1548d2a72d299058d1fd2339e3d398560a0e46c27dab4891e4d2"}, - "myxql": {:hex, :myxql, "0.6.3", "3d77683a09f1227abb8b73d66b275262235c5cae68182f0cfa5897d72a03700e", [:mix], [{:db_connection, "~> 2.4.1 or ~> 2.5", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.6 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:geo, "~> 3.4", [hex: :geo, repo: "hexpm", optional: true]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "af9eb517ddaced5c5c28e8749015493757fd4413f2cfccea449c466d405d9f51"}, + "myxql": {:hex, :myxql, "0.7.1", "7c7b75aa82227cd2bc9b7fbd4de774fb19a1cdb309c219f411f82ca8860f8e01", [:mix], [{:db_connection, "~> 2.4.1 or ~> 2.5", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.6 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:geo, "~> 3.4", [hex: :geo, repo: "hexpm", optional: true]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "a491cdff53353a09b5850ac2d472816ebe19f76c30b0d36a43317a67c9004936"}, "nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"}, "postgrex": {:hex, :postgrex, "0.17.3", "c92cda8de2033a7585dae8c61b1d420a1a1322421df84da9a82a6764580c503d", [:mix], [{:db_connection, "~> 2.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "946cf46935a4fdca7a81448be76ba3503cff082df42c6ec1ff16a4bdfbfb098d"}, "statistex": {:hex, :statistex, "1.0.0", "f3dc93f3c0c6c92e5f291704cf62b99b553253d7969e9a5fa713e5481cd858a5", [:mix], [], "hexpm", "ff9d8bee7035028ab4742ff52fc80a2aa35cece833cf5319009b52f1b5a86c27"}, diff --git a/test/ecto/adapters/myxql_test.exs b/test/ecto/adapters/myxql_test.exs index 4baac56d..89da3fb4 100644 --- a/test/ecto/adapters/myxql_test.exs +++ b/test/ecto/adapters/myxql_test.exs @@ -1621,7 +1621,8 @@ defmodule Ecto.Adapters.MyXQLTest do # DDL - import Ecto.Migration, only: [table: 1, table: 2, index: 2, index: 3, constraint: 3] + import Ecto.Migration, + only: [table: 1, table: 2, index: 2, index: 3, constraint: 2, constraint: 3] test "executing a string during migration" do assert execute_ddl("example") == ["example"] @@ -1963,23 +1964,6 @@ defmodule Ecto.Adapters.MyXQLTest do assert execute_ddl(drop) == [~s|DROP TABLE `foo`.`posts`|] end - test "drop constraint" do - assert_raise ArgumentError, ~r/MySQL adapter does not support constraints/, fn -> - execute_ddl( - {:drop, constraint(:products, "price_must_be_positive", prefix: "foo"), :restrict} - ) - end - end - - test "drop_if_exists constraint" do - assert_raise ArgumentError, ~r/MySQL adapter does not support constraints/, fn -> - execute_ddl( - {:drop_if_exists, constraint(:products, "price_must_be_positive", prefix: "foo"), - :restrict} - ) - end - end - test "alter table" do alter = {:alter, table(:posts), @@ -2152,15 +2136,34 @@ defmodule Ecto.Adapters.MyXQLTest do end test "create constraints" do - assert_raise ArgumentError, "MySQL adapter does not support check constraints", fn -> - create = {:create, constraint(:products, "foo", check: "price")} - assert execute_ddl(create) - end + create = {:create, constraint(:products, "price_must_be_positive", check: "price > 0")} - assert_raise ArgumentError, "MySQL adapter does not support check constraints", fn -> - create = {:create, constraint(:products, "foo", check: "price", validate: false)} - assert execute_ddl(create) - end + assert execute_ddl(create) == + [ + ~s|ALTER TABLE `products` ADD CONSTRAINT `price_must_be_positive` CHECK (price > 0)| + ] + + create = + {:create, + constraint(:products, "price_must_be_positive", check: "price > 0", prefix: "foo")} + + assert execute_ddl(create) == + [ + ~s|ALTER TABLE `foo`.`products` ADD CONSTRAINT `price_must_be_positive` CHECK (price > 0)| + ] + + create = + {:create, + constraint(:products, "price_must_be_positive", + check: "price > 0", + prefix: "foo", + validate: false + )} + + assert execute_ddl(create) == + [ + ~s|ALTER TABLE `foo`.`products` ADD CONSTRAINT `price_must_be_positive` CHECK (price > 0) NOT ENFORCED| + ] assert_raise ArgumentError, "MySQL adapter does not support exclusion constraints", fn -> create = {:create, constraint(:products, "bar", exclude: "price")} @@ -2173,6 +2176,37 @@ defmodule Ecto.Adapters.MyXQLTest do end end + test "drop constraint" do + drop = {:drop, constraint(:products, "price_must_be_positive"), :restrict} + + assert execute_ddl(drop) == + [~s|ALTER TABLE `products` DROP CONSTRAINT `price_must_be_positive`|] + + drop = {:drop, constraint(:products, "price_must_be_positive", prefix: "foo"), :restrict} + + assert execute_ddl(drop) == + [~s|ALTER TABLE `foo`.`products` DROP CONSTRAINT `price_must_be_positive`|] + + drop_cascade = {:drop, constraint(:products, "price_must_be_positive"), :cascade} + + assert_raise ArgumentError, + ~r/MySQL does not support `CASCADE` in `DROP CONSTRAINT` commands/, + fn -> + execute_ddl(drop_cascade) + end + end + + test "drop_if_exists constraint" do + assert_raise ArgumentError, + ~r/MySQL adapter does not support `drop_if_exists` for constraints/, + fn -> + execute_ddl( + {:drop_if_exists, + constraint(:products, "price_must_be_positive", prefix: "foo"), :restrict} + ) + end + end + test "create an index using a different type" do create = {:create, index(:posts, [:permalink], using: :hash)}