Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add check constraints for MySQL #621

Merged

Conversation

petermueller
Copy link
Contributor

@petermueller petermueller commented Jul 1, 2024

  • adds constraint/2 and constraint/3 support for adding and dropping check constraints in MySQL migrations
  • handles the 3819 / ER_CHECK_CONSTRAINT_VIOLATED MySQL server error number, introduced in 8.0.16

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

relies on elixir-ecto/myxql#187

@petermueller
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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

@petermueller petermueller changed the title add check constraint error handling for MyXQL add check constraints for MySQL Jul 1, 2024
@petermueller
Copy link
Contributor Author

petermueller commented Jul 1, 2024

@josevalim updated,

  • I still need to update docs.

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
Copy link
Member

josevalim commented Jul 1, 2024

  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
Copy link
Contributor Author

petermueller commented Jul 2, 2024

  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
Copy link
Member

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

@petermueller
Copy link
Contributor Author

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
Copy link
Contributor Author

Thanks 😊

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

@josevalim
Copy link
Member

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

@petermueller
Copy link
Contributor Author

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

@josevalim
Copy link
Member

Update mix.exs please!

@petermueller
Copy link
Contributor Author

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

@greg-rychlewski greg-rychlewski merged commit b2c9c98 into elixir-ecto:master Jul 6, 2024
10 checks passed
@greg-rychlewski
Copy link
Member

thanks very much

@petermueller petermueller deleted the pkm/constraint-handler branch July 7, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants