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

Add validations for t.references/add_reference options #33347

Conversation

georgewambold
Copy link
Contributor

This adds a validation on which options can be passed when adding a
reference to a table. The valid options are: :type, :index, :polymorphic,
:unique, :foreign_key and :limit. Any other key is invalid.

For example, this would not raise an error before this commit, now it does:

create_table :posts do |t|
t.references :users, fake_option: true
end

Summary

Provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.

If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.

Finally, if your pull request affects documentation or any non-code
changes, guidelines for those changes are available
here

Thanks for contributing to Rails!

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@rafaelfranca
Copy link
Member

rafaelfranca commented Jul 11, 2018

What if we have an external database adapter that support other options that are not :type, :index, :polymorphic, :unique, :foreign_key, :limit. How would that adapter add support for that option?

@georgewambold
Copy link
Contributor Author

@rafaelfranca Hmm, that's a good point. I guess they'd overwrite the :add_reference method?

@georgewambold georgewambold force-pushed the add_errors_to_adding_table_references branch from a8d2dac to ccad1a8 Compare July 11, 2018 20:59
@rafaelfranca
Copy link
Member

That was the reason why we never added validations to those methods. Given those methods generates SQL and those SQL can be easily verified we thought that users testing their migrations were enough to make sure the options they are passing are the correct one.

But at the same time we also added to add_index, so I think this is fine.

@eugeneius
Copy link
Member

I think we should preserve the old behaviour for migrations generated on previous Rails versions:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/migration/compatibility.rb

@georgewambold
Copy link
Contributor Author

@rafaelfranca Ah, I see.

@georgewambold
Copy link
Contributor Author

georgewambold commented Jul 11, 2018

@eugeneius It makes sense that a lot of people run db:migrate to build their database (when they should probably be using db:schema:load instead), in which case old migrations could error when they update their rails version. That could be a bad experience.

That said, I wonder about people who have these errors (misspelling options like unique or null in existing migrations) who don't realize it. Finding misspelled options in legacy code is what inspired me to create the issue in the first place.

@rafaelfranca
Copy link
Member

@eugeneius good point.

@georgewambold can you change the PR to implement the old behavior in the compatibility class?

@georgewambold
Copy link
Contributor Author

@rafaelfranca Would I put it in the V5_2 class?

@rafaelfranca
Copy link
Member

Yes

@georgewambold georgewambold force-pushed the add_errors_to_adding_table_references branch from ccad1a8 to 4295cbd Compare July 11, 2018 21:45
@eugeneius
Copy link
Member

Hmm. Adding a reference ultimately calls table.column here, passing through any options it doesn't understand - which means we need to allow all of the options that are valid when adding a column.

I also think this suggests that the validation should apply to adding a column, rather than a reference.

@georgewambold georgewambold force-pushed the add_errors_to_adding_table_references branch 2 times, most recently from 42db8b8 to c250e54 Compare July 19, 2018 17:38
@georgewambold
Copy link
Contributor Author

@eugeneius That's a great point-- This expands the scope of the issue a bit, but I agree that as references are really just columns, we could kill a few birds with one stone and put the validation on column creation. I just pushed up an updated branch-- Let me know what you think.

@rafaelfranca What are your thoughts on adding options validations to all column definitions? Also what does it mean for compatibility?

@georgewambold georgewambold force-pushed the add_errors_to_adding_table_references branch from c250e54 to 6b2711a Compare July 19, 2018 20:05
@@ -427,6 +427,8 @@ def new_column_definition(name, type, **options) # :nodoc:

private
def create_column_definition(name, type, options)
options.assert_valid_keys(:type, :force, :null, :default, :id, :precision, :scale, :array, :primary_key, :index, :polymorphic, :unique, :foreign_key, :limit, :first, :after)
Copy link
Member

Choose a reason for hiding this comment

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

Failing tests with :comment, :collation, and :auto_increment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Good call

@eugeneius
Copy link
Member

I'd like to hear @rafaelfranca's opinion on the idea before giving feedback on the implementation, but I think the approach to compatibility should be the same: only apply the validation to new migrations by restoring the old behaviour in the V5_2 migration compatibility class.

This adds a validation on which options can be passed when adding a
reference to a table. The valid options are: :type, :index, :polymorphic,
:unique, :foreign_key and :limit. Any other key is invalid.

For example, this would not raise an error before this commit, now it does:

create_table :posts do |t|
  t.references :users, fake_option: true
end
@georgewambold georgewambold force-pushed the add_errors_to_adding_table_references branch from 6b2711a to c8d37ea Compare July 25, 2018 18:44
@dillonwelch
Copy link
Contributor

I like this idea. I've ran into this before where I've added options that I thought a method had but didn't and ended up with an incomplete database setup. Feels like a much more realistic scenario to guard against than some future external adapter that would have new keys outside of the list (that could just be updated in a new PR to be included in the list).

:after, :array, :as, :auto_increment, :charset, :collation, :comment,
:default, :first, :force, :foreign_key, :id, :index, :limit, :null,
:options, :polymorphic, :precision, :primary_key, :scale, :stored,
:temporary, :type, :unique, :unsigned
Copy link
Contributor

Choose a reason for hiding this comment

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

A thought instead of hardcoding the list here: if these were defined on the adapter class (this here being the abstract adapter), then each database could have its own set of allowed options. (def SubclassAdapter.valid_keys; super + [:custom_option])

@rails-bot
Copy link

rails-bot bot commented Mar 5, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Mar 5, 2020
@georgewambold
Copy link
Contributor Author

I'm going to work on this when I get a chance -- I think it's a worthwhile feature.

@rails-bot
Copy link

rails-bot bot commented Jun 8, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jun 8, 2020
@rails-bot rails-bot bot closed this Jun 15, 2020
@tgxworld
Copy link
Contributor

Still a WIP but I picked this up in #39659

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.

9 participants