I tried to add a test to check if the opts in from are applied correctly, but I can't find the tests for rollbacks.

Am I overlooking those?

Above comment is not relevant anymore, I added a test for a rollback too 😄

Hi,

Thanks for the PR. I'm not entirely sure this will work. For example, the Postgres docs say this

5.6.4. Removing a Constraint

To remove a constraint you need to know its name. If you gave it a name then that's easy. Otherwise the system assigned a generated name, which you need to find out. The psql command \d tablename can be helpful here; other interfaces might also provide a way to inspect table details. Then the command is:

ALTER TABLE products DROP CONSTRAINT some_name;
(If you are dealing with a generated constraint name like $2, don't forget that you'll need to double-quote it to make it a valid identifier.)

As with dropping a column, you need to add CASCADE if you want to drop a constraint that something else depends on. An example is that a foreign key constraint depends on a unique or primary key constraint on the referenced column(s).

This works the same for all constraint types except not-null constraints. To drop a not null constraint use:

ALTER TABLE products ALTER COLUMN product_no DROP NOT NULL

So it looks to me like this PR will drop the foreign key constraint but not the null constraint.

Hey,

Good find! I didn't think that would be relevant for this PR, because the null option already existed, e.g.:

# This is already possible in the current version of ecto_sql
modify :author_id, references(:authors), null: false
# This becomes possible by this PR, but is implicitly the same (correct me if I'm wrong :))
modify :author_id, references(:authors), null: false, from: {references(:authors), null: true}

But to be sure, I added integration tests, and it seems ecto_sql already handles the DROP NOT NULL! 🥳

However, the MySQL integration test may fail, because it's not possible to DROP and ADD a constraint with the same name in a single ALTER TABLE statement in a specific condition:

Adding and dropping a foreign key in the same ALTER TABLE statement is supported for ALTER TABLE ... ALGORITHM=INPLACE but not for ALTER TABLE ... ALGORITHM=COPY.

It seems that the default ALGORITHM is INPLACE, but I get an error using my local MySQL instance. I'm not sure if it should be fixed in this PR, as this error also occurs using the migration above, which is already possible in current ecto_sql?

I've adjusted the integration tests to migrate from a FK to an integer and back. This way the change in this PR is tested (passing opts to from), without breaking the MySQL adapter.

It seems that the MySQL adapter currently can't handle migrating an existing FK column to another FK, I'll create a separate issue for that.

Edit: I'm not sure why 1 check is failing, it's working locally and in my fork. Can someone try if restarting the check works?

Sorry so what I meant was I don't think this works:

modify :author_id, references(:authors), null: true, from: {references(:authors), null: false}

When it drops the old behaviour it will drop the foreign key constraint but not the NOT NULL constraint. This is what it looks like to me. We can see in your unit test that the query string does not contain the statement removing it.

BTW it's ok to leave out the integration tests. I believe I misspoke before about adding them and I edited my comment. We are basically just looking at the query string anyway which is what the unit test is doing.

When it drops the old behaviour it will drop the foreign key constraint but not the NOT NULL constraint. This is what it looks like to me. We can see in your unit test that the query string does not contain the statement removing it.

Actually I apologize, it's because in your unit test you are not setting a new :null value so it is not changing it. So what you have looks correct to me. Could you please do the following:

1. Can you add null: true to the unit tests so we see it dropping the NOT NULL constraint?
2. Can you remove the integration tests because they are basically just duplicating the unit tests.

Then we can merge. Thanks!

Using your example, the migration query for Postgres becomes:

ALTER TABLE "comments" DROP CONSTRAINT "comments_author_id_fkey", ALTER COLUMN "author_id" TYPE bigint, ADD CONSTRAINT "comments_author_id_fkey" FOREIGN KEY ("author_id") REFERENCES "authors"("id"), ALTER COLUMN "author_id" DROP NOT NULL

So it does DROP NOT NULL on the author_id column. Also, when rolling back, it adds the NOT NULL constraint again:

ALTER TABLE "comments" DROP CONSTRAINT "comments_author_id_fkey", ALTER COLUMN "author_id" TYPE bigint, ADD CONSTRAINT "comments_author_id_fkey" FOREIGN KEY ("author_id") REFERENCES "authors"("id"), ALTER COLUMN "author_id" SET NOT NULL

Is this what you mean, or am I misunderstanding you here?

Maybe I wasn't clear in what I'm trying to fix here. Passing the opts to from is not a new feature, according to the docs it should work already. The docs use from: {:string, null: true} as an example. But currently it doesn't work when the type is a Reference, because the pattern match in drop_reference_expr didn't match.

Ah okay, makes sense then! I'll fix the PR and let you know!

Yeah I apologize I didn't look carefully enough at your unit tests. I thought you were passing null: true and it was still not dropping the null constraint. So I thought in addition to not handling the reference name it was not handling the reference opts.

No problem, I'm glad it's looking good now! 😄

The tests are fixed.

Thanks!