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

Raise error when generating attribute with dangerous name #47752

Merged
merged 1 commit into from Sep 14, 2023

Conversation

p8
Copy link
Member

@p8 p8 commented Mar 23, 2023

Generating a model with attributes named hash or save should raise an error, instead of generating a migration with an invalid attribute.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Mar 23, 2023
@p8 p8 force-pushed the activerecord/keyword-attributes branch 5 times, most recently from 7ff68ef to 8e24add Compare March 23, 2023 20:46
@p8 p8 force-pushed the activerecord/keyword-attributes branch from 8e24add to 7c2debd Compare September 13, 2023 09:54
Comment on lines 71 to 74
def unknown_type?(type)
type &&
DEFAULT_TYPES.exclude?(type.to_s) &&
! ActiveRecord::Base.connection.valid_type?(type)
Copy link
Member

Choose a reason for hiding this comment

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

It hard to read the use of exclude? in multiple conditions to me.
Let's keep it as it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @kamipo. I've reverted the refactoring.

@p8 p8 force-pushed the activerecord/keyword-attributes branch 3 times, most recently from f92d1c5 to ea6ce15 Compare September 14, 2023 10:57
Generating a model with attributes named `hash` or `save` should raise
an error, instead of generating a migration with an invalid attribute.
@p8 p8 force-pushed the activerecord/keyword-attributes branch from ea6ce15 to 99b1316 Compare September 14, 2023 10:58
@kamipo kamipo merged commit 6d4abdc into rails:main Sep 14, 2023
4 checks passed
@p8 p8 deleted the activerecord/keyword-attributes branch September 14, 2023 12:36
@camertron
Copy link
Contributor

camertron commented Sep 14, 2023

I just ran into an issue caused by this change in the view_component project. Our preview generator defines an :attributes argument. The NamedBase class eventually converts each attribute into a GeneratedAttribute, which attempts to determine if the attribute is dangerous.

Unfortunately, view_component's test suite doesn't have activerecord configured, so it blows up with:

Error:
PreviewGeneratorTest#test_component_with_namespace_and_attributes:
NameError: uninitialized constant #<Class:Rails::Generators::GeneratedAttribute>::ActiveRecord

I tried requiring activerecord in the dummy app, but doing so means we have to install and configure a database, which I'd rather not do.

The bigger issue in my mind is that the change in this PR perhaps inadvertently couples the Rails generator code to activerecord, which not all apps have set up. Moreover, generated attributes are not just an activerecord thing as our preview generator demonstrates.

Perhaps we could move ActiveRecord::Base.dangerous_attribute_method? into activesupport or to somewhere more easily accessible?

@kamipo
Copy link
Member

kamipo commented Sep 14, 2023

Actually, coupling with activerecord has happened before at #42311.
We probably should do checking defined?(ActiveRecord::Base) like NamedBase already does.

def pluralize_table_names? # :doc:
!defined?(ActiveRecord::Base) || ActiveRecord::Base.pluralize_table_names
end

@rafaelfranca
Copy link
Member

Yes. We should. @p8 mind to open a PR?

@p8
Copy link
Member Author

p8 commented Sep 15, 2023

@rafaelfranca I've created a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants