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 a false positive for Rails/NotNullColumn when adding a :virtual column #892

Merged
merged 1 commit into from
Jan 8, 2023

Conversation

fatkodima
Copy link
Contributor

Fixes #887.

From documentation:

Several restrictions apply to the definition of generated columns and tables involving generated columns:

  • A generated column cannot have a column default or an identity definition.

pairs = add_not_null_column?(node)
check_pairs(pairs)
add_not_null_column?(node) do |type, pairs|
return if type.source.include?('virtual')
Copy link
Member

Choose a reason for hiding this comment

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

include? is a partial match, so I'm worried about false positives. What about below?

Suggested change
return if type.source.include?('virtual')
return if type.source == ':virtual'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to specify column types as a string (while not as popular as symbol) or a symbol, so I tried to match both. Do you think we can skip it?

Copy link
Member

Choose a reason for hiding this comment

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

IMO, It's a corner case, but I think String#include? is not suitable because it matches all strings containing virtual. This patch will need to add a test case for column types as a string and updating an appropriate logic.

Copy link
Contributor Author

@fatkodima fatkodima Jan 6, 2023

Choose a reason for hiding this comment

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

Added explicit check and a test case.

@fatkodima fatkodima force-pushed the not_null_column-accept-virtual branch from 54bba14 to d723ab8 Compare January 6, 2023 11:06
pairs = add_not_null_column?(node)
check_pairs(pairs)
add_not_null_column?(node) do |type, pairs|
return if type.source == ':virtual' || type.source == "'virtual'"
Copy link
Member

Choose a reason for hiding this comment

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

type.value should be used instead of type.source to detect double quoted "virtual".

Suggested change
return if type.source == ':virtual' || type.source == "'virtual'"
return if type.value == :virtual || type.value == 'virtual'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Thanks! Fixed.

@fatkodima fatkodima force-pushed the not_null_column-accept-virtual branch from d723ab8 to d60ff74 Compare January 8, 2023 13:23
@koic koic merged commit 7dc5ed7 into rubocop:master Jan 8, 2023
@koic
Copy link
Member

koic commented Jan 8, 2023

Thanks!

@fatkodima fatkodima deleted the not_null_column-accept-virtual branch January 8, 2023 13:29
@MrJoy
Copy link

MrJoy commented Jan 14, 2023

Thank you!

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.

Rails/NotNullColumn suggesting default value for virtual column.
3 participants