elixir-ecto / ecto_sql

SQL-based adapters for Ecto and database migrations
https://hexdocs.pm/ecto_sql
Apache License 2.0
578 stars 312 forks source link

add check constraints for MySQL #621

Closed petermueller closed 4 months ago

petermueller commented 5 months ago

https://dev.mysql.com/doc/mysql-errors/8.0/en/server-error-reference.html#error_er_check_constraint_violated

relies on https://github.com/elixir-ecto/myxql/pull/187

petermueller commented 5 months ago

Doing an ALTER TABLE ... ADD CONSTRAINT, like in the test, is only supported in 8.0.19 and above but I don't imagine that should be an issue?

If it needs to be compliant w/ >= 8.0.16 I can change it to be a CREATE TABLE, since IIRC you can't add fragments inside a create table(...) block

petermueller commented 5 months ago

If wanted I can take a crack at adding the check constraint migrator implementation for MySQL as well, or put it in another PR, now that 8.0.19 supports check constraints outside table definitions

josevalim commented 5 months ago

@petermueller I think it is fine to do it all as part of this PR.

petermueller commented 5 months ago

@josevalim updated,

Questions:

  1. modify(constraint)

There's support for enabling/disabling ENFORCED in MySQL for a check constraint if we wanted to add a alter table, do: modify(constraint(...)).

Postgres supports the equivalent enabling (not disabling) w/ VALIDATE CONSTRAINT

  1. drop_if_exists(constraint) is possible but I think might require a multiple statements, or a procedure and maybe a DROP PROCEDURE IF EXISTS PROC_ECTO_DROP_IF_CHECK ...

I don't know what people's appetite is for having ecto manage stored procedures :-/

josevalim commented 5 months ago
  1. I believe we support the :validate option and we can support it for PG
  2. It is ok to not support all of it right now, we can raise in those cases still
petermueller commented 5 months ago
  1. modify(constraint)

I should clarify, I don't see anywhere that the PG adapter connection does ALTER TABLE table_name VALIDATE CONSTRAINT constraint_name pg docs.

I see in the reference column_change functions that it basically does that same thing by recreating the constraint. But that only works for foreign keys, not check constraints, i.e. it's from modify :group_id, references("group")

I'm not sure this is really worth it right now. I can add to the docs for the :validate option to mention that it's called "ENFORCED" in MySQL.

  1. drop_if_exists(constraint)

πŸ˜… I kind of wanted to see if I could do it with a procedure. I pasted it below, and it works (using a down/4 in the integration test w/ on_exit), but if this is not something we want to include, I completely understand πŸ˜† I had some DELIMITER lines but MyXQL did not seem to want to play nice, and it seems to work without them, surprisingly?

The tests I added seem to work just though. Probably could use a test with multiple migrations to know the delimiter doesn't mess things up.

    def execute_ddl({:drop_if_exists, %Constraint{} = constraint, :restrict}) do
      table_name = [?', to_string(constraint.table), ?']
      constraint_name = [?', to_string(constraint.name), ?']

      table_schema =
        if constraint.prefix do
          [?', to_string(constraint.prefix), ?']
        else
          "DATABASE()"
        end

      [
        ["DROP PROCEDURE IF EXISTS PROC_ECTO_DROP_CONSTRAINT"],
        [
          "CREATE PROCEDURE PROC_ECTO_DROP_CONSTRAINT ( ",
          "IN tableName VARCHAR(200), ",
          "IN constraintName VARCHAR(200), ",
          "IN tableSchema VARCHAR(200) ",
          ") ",
          "BEGIN ",
          "IF EXISTS ( ",
          "SELECT * FROM `information_schema`.`table_constraints` ",
          "WHERE table_schema = tableSchema ",
          "AND table_name = tableName ",
          "AND constraint_name = constraintName ",
          "AND constraint_type in ('UNIQUE', 'FOREIGN KEY', 'CHECK')) ",
          "THEN ",
          "SET @query = CONCAT('ALTER TABLE ', tableName, ' DROP CONSTRAINT ', constraintName, ';'); ",
          "PREPARE stmt FROM @query; ",
          "EXECUTE stmt; ",
          "DEALLOCATE PREPARE stmt; ",
          "END IF; ",
          "END"
        ],
        [
          "CALL PROC_ECTO_DROP_CONSTRAINT(",
          table_name,
          ", ",
          constraint_name,
          ", ",
          table_schema,
          ")"
        ],
        ["DROP PROCEDURE PROC_ECTO_DROP_CONSTRAINT"]
      ]
    end

Ok, Let me stop increasing scope and get to just finishing the docs changes πŸ˜…

josevalim commented 4 months ago

Agreed the changes are out of scope. Let's change the docs and I will do another review soon!

petermueller commented 4 months ago

Updated πŸ™‚ I don't think there are any other references to the scenarios that this code addresses so I think this is it.

Let me know if you prefer a different format or different information ☺️

petermueller commented 4 months ago

Thanks 😊

Once the myxql changes are released, should I update this PR to point to them?

josevalim commented 4 months ago

I released v0.7.1, so we can depend on it!

petermueller commented 4 months ago

Cool. I will update push in a few hours. Just 0.7, or just unlock so only the lockfile changes?

josevalim commented 4 months ago

Update mix.exs please!

petermueller commented 4 months ago

Should be good to go. I also added a tag so 5.7 shouldn't try to run them

greg-rychlewski commented 4 months ago

thanks very much