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

Don't check type on add_column via column_exists?` #42212

Merged

Conversation

eileencodes
Copy link
Member

@eileencodes eileencodes commented May 12, 2021

I originally added the type to the column_exists? check in
add_column because I thought it would be useful to still raise if you
tried to add a column with a different type. It seemed bad to silently
ignore.

The type check works fine for standard, non-type casted types like
string, but migrations that use types like blob in mysql/sqlite3 or
char in postgres cast these to other types. When c.type == type.to_sym is called in those cases they won't match. For example
c.type for a blob column will return binary so the comparison
won't match. I attempted to cast the type passed in but that's error
prone because for dbs like postgres we need a lot more information
than just a type to determine how to cast it. I also tried going the
other way but the column doesn't store the original type, just the type
casted type (as type) and the sql type which is a string representation
of type that doesn't always match the original.

This functionality was originally extracted from GitHub and when I
looked back at our old implementation I found that we originally weren't
passing type. This came up in a migration for enterprise that wasn't
properly handling the if_not_exists because our type check was
comparing binary to mediumblob. I think it makes the most sense to
leave the public column_exists? method as is and remove the type
check from column_exists? in the add_column method.

@eileencodes
Copy link
Member Author

Welp that didn't work for postgres - looking into that.

I originally added the `type` to the `column_exists?` check in
`add_column` because I thought it would be useful to still raise if you
tried to add a column with a different type. It seemed bad to silently
ignore.

The `type` check works fine for standard, non-type casted types like
`string`, but migrations that use types like `blob` in mysql/sqlite3 or
`char` in postgres cast these to other types. When `c.type ==
type.to_sym` is called in those cases they won't match. For example
`c.type` for a `blob` column will return `binary` so the comparison
won't match. I attempted to cast the `type` passed in but that's error
prone because for dbs like postgres we need _a lot_ more information
than just a type to determine how to cast it. I also tried going the
other way but the column doesn't store the original type, just the type
casted type (as type) and the sql type which is a string representation
of type that doesn't always match the original.

This functionality was originally extracted from GitHub and when I
looked back at our old implementation I found that we originally weren't
passing type. This came up in a migration for enterprise that wasn't
properly handling the `if_not_exists` because our `type` check was
comparing `binary` to `mediumblob`. I think it makes the most sense to
leave the public `column_exists?` method as is and remove the `type`
check from `column_exists?` in the `add_column` method.
@eileencodes eileencodes force-pushed the fix-type-column-casting-in-column_exists branch from 8e41573 to 627da23 Compare May 12, 2021 21:12
@eileencodes eileencodes changed the title Fix column_exists when type is cast Don't check type on add_column via column_exists?` May 12, 2021
@eileencodes
Copy link
Member Author

eileencodes commented May 12, 2021

I reimplemented this differently than my original commit said. I dropped the casting altogether and am just checking for the same column name on the table. This has a slight behavior change in that we no longer will raise if you try to add the same column name with a different type, but I think that's rare.

@federicoaldunate
Copy link
Contributor

federicoaldunate commented May 12, 2021

I reimplemented this differently than my original commit said. I dropped the casting altogether and am just checking for the same column name on the table. This has a slight behavior change in that we no longer will raise if you try to add the same column name with a different type, but I think that's rare.

I agree with what you did. Also, I think it's consistent with the remove_column with if_exists option in true.

def remove_column(table_name, column_name, type = nil, **options)
  return if options[:if_exists] == true && !column_exists?(table_name, column_name)
  .
  .
  .

Note: I don't like the naming of the option if_not_exists, I think it's confusing.

@eileencodes
Copy link
Member Author

Note: I don't like the naming of the option if_not_exists, I think it's confusing.

I followed existing options on create/drop table when I implemented this. It doesn't make sense to have different options.

@eileencodes eileencodes merged commit 84266c0 into rails:main May 12, 2021
@eileencodes eileencodes deleted the fix-type-column-casting-in-column_exists branch May 12, 2021 21:47
eileencodes added a commit that referenced this pull request May 13, 2021
stefannibrasil pushed a commit to hexdevs/rails that referenced this pull request May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants