Fix #979 Address crashes with three state boolean detection #980
Conversation
|
Sorry folks, I don't have the capacity to test locally at the moment or fix the existing test coverage. |
| @@ -60,7 +63,7 @@ def table_node(node) | |||
| node.first_argument | |||
| when :column, :boolean | |||
| ancestor = node.each_ancestor(:block).find do |n| | |||
| n.method?(:create_table) || n.method?(:change_table) | |||
| n.method?(:create_table) || n.method?(:change_table) || n.method?(:drop_table) || n.method?(:create_enum) | |||
There was a problem hiding this comment.
create_enum was meant to be a representative example of a project-specific wrapper for create_table, and I don't think it's appropriate to add project-specific wrappers to a general purpose gem, unless it's via a configuration option.
What does adding drop_table do here? Can it take a block?
There was a problem hiding this comment.
There was a problem hiding this comment.
Removed create_enum; though https://blog.saeloun.com/2022/01/12/rails-7-adds-custom-enum-support-in-postgresql is an addition in rails 7.
See rails/rails#41469 for where it was added to rails, and some of the other create_* methods that potentially should also be skipped by this check. Edit sorry, it's from your original bug report! So you know all that.
There was a problem hiding this comment.
To be clear, drop_table can take a block, it is used when rolling back the migration to recreate the table that was dropped.
| @@ -46,6 +46,9 @@ def on_send(node) | |||
|
|
|||
| def_node = node.each_ancestor(:def, :defs).first | |||
| table_node = table_node(node) | |||
|
|
|||
| return unless table_node && column_node | |||
There was a problem hiding this comment.
The following line calling change_column_null?, I read as: "if we can find code that changes the column nullness later, then don't add a violation". But if that code can't be found, because there's no enclosing def block to search, then the exception can't be made, and it will report a violation, maybe causing a false positive.
To be consistent with the above, if it can't find the nullness change because other parts of the context are missing (like the table name), I think it should also skip the exception and report a violation.
There was a problem hiding this comment.
Not 100% clear here, are you suggesting:
return if def_node && change_column_null?(def_node, table_node&.value, column_node&.value)
Or something else?
There was a problem hiding this comment.
I think I would go with
return if def_node && table_node && column_node && \
change_column_null?(def_node, table_node.value, column_node.value)
|
I'm going to close this PR because no one can do regression testing without a reproduction test. I opened #983 instead. Thank you. |
Fix #979
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.