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

Fix edge cases in migrator when using fill_existing_with with a boolean value #676

Merged
merged 3 commits into from
May 24, 2021
Merged

Fix edge cases in migrator when using fill_existing_with with a boolean value #676

merged 3 commits into from
May 24, 2021

Conversation

davidepaolotua
Copy link
Contributor

This covers a very edge case when performing migrations that imply adding a not null boolean column filling previous records with the false value.
In practice when performing this kind of modification

alter table_for(SomeTable) do
 add a_column : Boolean, fill_existing_with: false

the migration fails as the fill_existing_with is not considered.
The patch fixes this particular case.

Note 1:
There is currently a way to bypass this by switching the former code to:

alter table_for(SomeTable) do
 add a_column : Boolean, fill_existing_with: "false"

(add double quotes)

Note 2:
the resulting statement is now:

UPDATE users SET admin = 'false';

Is it correct? I didn't touch the logic in the BoolColumn, but I would assume that in theory it should be without single quotes. However, it seems that the same applies to FloatColumn as well...

Note 3:
Why is the default value for fill_existing_with in the add_macro nil and not :nothing?

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Good catch! Thanks for the fix. This is definitely the right direction over using "false".

As for why it's defaulted to nil instead of :nothing... I'm not sure 😅 It would seem like we could just set the default to to :nothing, but I would want to dig in a bit more to double check that wouldn't mess up something else.

@jwoertink jwoertink merged commit 02b786a into luckyframework:master May 24, 2021
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.

2 participants