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 Ecto.Migration.remove_if_exists/1 #624

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

novaugust
Copy link
Contributor

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

@@ -39,7 +39,8 @@ defmodule Ecto.Adapter.Migration do
| {:remove, field :: atom, type :: Ecto.Type.t() | Reference.t() | binary(),
Keyword.t()}
| {:remove, field :: atom}
| {:remove_if_exists, type :: Ecto.Type.t() | Reference.t() | binary()}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec was missing field

@josevalim
Copy link
Member

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

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 =)

Copy link
Member

@greg-rychlewski greg-rychlewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@greg-rychlewski greg-rychlewski merged commit 5a29789 into elixir-ecto:master Jul 24, 2024
10 checks passed
@novaugust novaugust deleted the me/remove-if-exists/1 branch July 24, 2024 16:41
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