search

elixir-ecto / ecto_sql

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

Add `Ecto.Migration.remove_if_exists/1` #624

Closed novaugust closed 1 month ago

novaugust commented 1 month ago

This creates a ...if_exists mirror for remove/1. I'm guessing remove_if_exists/2 is needed for mysql, but psql will drop fkey constraints as part of dropping the column, so remove_if_exists/2 is unnecessary in that case.

I also updated the documentation for remove_if_exists/2 to help make it clearer why the type can/should be passed

josevalim commented 1 month ago

Sorry, I may be missing something, but shouldn't we promote remove_if_exists/2 instead? I believe remove/1 exists mostly for backwards compatibility.

novaugust commented 1 month ago

Sorry, I may be missing something, but shouldn't we promote remove_if_exists/2 instead?

unlike remove/2, remove_if_exists/2 isn't reversible. it uses the type solely to drop fkey constraints - any non references(...) type is just fully ignored. if it is a references(...) type, it still doesn't matter in the case of psql, which removes any related constraints as part of removing the column. i can't speak to mysql, but assume it still requires fkey constraints are dropped before their columns are, and is the whole reason /2 exists.

personally, i often reach for remove/1 over remove/2 because we only ever roll production forward - there's no ecto.rollback in prod. i went to do the same thing here but with remove_if_exists and was surprised that this didn't exist, then sad that i had to go look up the exact types for the 12 columns i was dropping, only to discover that actually that argument was ignored anyways and thus didn't matter.

since the second argument doesn't matter for any use-case for psql users and lots of use-cases for mysql folks, remove_if_exists/1 seemed like a better api to me =)