Blacklist ruby keywords for scopes #14582

arthurnn · 2014-04-03T14:41:12Z

arthurnn · 2014-04-03T14:41:40Z

activerecord/lib/active_record/attribute_methods.rb

@@ -29,6 +29,8 @@ def self.set_name_cache(name, value)
      end
    }

+    BLACKLISTED_CLASS_METHODS = %w(private public protected alias and BEGIN begin break case class def defined? do else elsif END end ensure false for if in module next nil not or redo rescue retry return self super then true undef unless until when while yield).freeze


Not just if I should break this in multiple lines or not. Let me know

Better to break the lines. Also we don't need to freeze

arthurnn · 2014-04-03T14:41:59Z

cc @tenderlove

fw42 · 2014-04-03T14:48:40Z

Do we have to hardcode that list? Can't we get that dynamically from somewhere? Like Class.methods or something like that.

rafaelfranca · 2014-04-03T14:59:45Z

@fw42 this list is not only about method on Class, it is also about some ruby keywords. I think it is fine to use a hardcoded list.

fw42 · 2014-04-03T15:06:11Z

public/protected/private are not keywords in Ruby as far as I know

chancancode · 2014-04-03T16:23:14Z

Can we add the test cases to the enum tests as well? 😄

rafaelfranca · 2014-04-03T17:11:55Z

@fw42 this is why I said "not only about"

chancancode · 2014-04-03T17:51:48Z

Seems good, some comments:

We should (eventually) add this to attribute methods too perhaps? The list would be slightly different, but it seems like the bulk of it can be shared
The list seems slightly on the aggressive side – I don't see most of the keywords actually causing problems. For example, Mission.rescue seems reasonable:

>> class A
>>   def rescue
>>     puts 'lol'
>>   end
>> 
?>   begin
?>     raise
>>   rescue
>>     puts 'omg'
>>   end
>> end
omg
=> nil
>> A.rescue
lol
=> nil

When you call it on the class as an explicit receiver it resolves correctly (the common usage for a scope), and when you use it without the receiver the keyword takes precedence (the common usage for the rescue keyword).

Not that I would recommend defining a scope with those names, but since this is supposed to be a "helpful" feature to help you discover problems early on, perhaps we can start with the most dangerous scenario so we don't stop people from upgrading over the unlikely problems.

Not a huge deal though as I don't think anyone will actually define a scope called if... so I'll leave it to you and @rafaelfranca to decide where to draw the line.

arthurnn · 2014-04-03T19:13:04Z

@chancancode Enum tests added .

About what it should be on the list, I am on the fence about it.
I guess having a bigger list is better, but I am no strong about it too.

Let me know, I can change the list content.

chancancode · 2014-04-03T20:17:06Z

@rafaelfranca thoughts? If you don't feel strongly about this either we can probably just keep it as is and merge
—
Sent from Mailbox for iPhone

On Thu, Apr 3, 2014 at 12:13 PM, Arthur Nogueira Neves
notifications@github.com wrote:

@chancancode Enum tests added .
About what it should be on the list, I am on the fence about it.
I guess having a bigger list is better, but I am no strong about it too.

Let me know, I can change the list content.

Reply to this email directly or view it on GitHub:
#14582 (comment)

rafaelfranca · 2014-04-03T20:23:07Z

@arthurnn I think is better to trim down this list a little. You can check which method would be problematic using the same code @chancancode used in the above comment. WDYT?

rafaelfranca · 2014-04-03T20:23:31Z

I'm more concerned to release a patch release only to remove methods from this list.

arthurnn · 2014-04-03T20:30:41Z

@rafaelfranca make sense. TBH, keywords doesnt need to be in that list, as @chancancode showed in his code... I will have only private protected public on the list for now. Specially because this is going to 4.1 ..

rafaelfranca · 2014-04-03T20:32:36Z

Seems good.

Add tests to make sure scopes cannot be create with names such as: private, protected, public. Make sure enum values don't collide with those methods too.

arthurnn · 2014-04-03T20:40:34Z

Done.

Blacklist ruby keywords for scopes

Blacklist ruby keywords for scopes Conflicts: activerecord/CHANGELOG.md

wincent · 2014-04-11T00:58:55Z

This broke my app, which has a couple of public scopes (which have been working fine for years).

I've fixed it, but I was disappointed to see a non-essential change like this slip in after RC and days before final release. (And more so because there is no rationale expressed here or in the commit message for why this change is a good idea; public, private, protected aren't keywords as far as I know.)

arthurnn · 2014-04-11T01:07:50Z

@wincent you can read it in here #14583 about, why defining a self.public method is a bad idea.
tl;dr: alias_method_chain will not work in models that define a scope :public

wincent · 2014-04-11T02:02:02Z

Thanks for the link!

On Thursday, April 10, 2014, Arthur Nogueira Neves notifications@github.com
wrote:

@wincent https://github.com/wincent you can read it in here #14583 https://github.com/rails/rails/pull/14583about, why defining a
self.public method is a bad idea.
tl;dr: alias_method_chain will not work in models that define a scope
:public

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14582#issuecomment-40161848
.

chancancode · 2014-04-11T06:46:20Z

Good call @wincent, for the future, it would be nice to include the rationale in the commit message for these kind of changes, so that when someone git blame and found the commit they'll know what's going on, I'll keep that in mind when writing commit messages (and reviewing commits) in the future!

In this case, this is dangerous because things would appear to work on the surface but could be strangely broken inside, and it could be very non-obvious and tricky to debug. For example:

class Posts < AR::Base
  enum visibility: [:public, :private]

  private
    def dont_call_me
      ...
    end
end

Post.new.dont_call_me # <- it works, huh.

Avoid collision with `public` class method. As of Rails 4.1, `public` is explicitly black-listed as a scope name: rails/rails#14582

arthurnn reviewed Apr 3, 2014
View reviewed changes

fw42 mentioned this pull request Apr 3, 2014

Fix alias_method_chain for classes which have class methods named 'public', 'protected', or 'private' #14583

Closed

arthurnn mentioned this pull request Apr 3, 2014

Enums need to raise on conflict singular class methods #14586

Closed

rafaelfranca added this to the 4.1.0 milestone Apr 3, 2014

Block a few default Class methods as scope name.

815c9de

Add tests to make sure scopes cannot be create with names such as: private, protected, public. Make sure enum values don't collide with those methods too.

rafaelfranca added a commit that referenced this pull request Apr 3, 2014

Merge pull request #14582 from arthurnn/blacklist_ruby_methods

362203e

Blacklist ruby keywords for scopes

rafaelfranca merged commit 362203e into rails:master Apr 3, 2014

rafaelfranca added a commit that referenced this pull request Apr 3, 2014

Merge pull request #14582 from arthurnn/blacklist_ruby_methods

0f05fbf

Blacklist ruby keywords for scopes

rafaelfranca added a commit that referenced this pull request Apr 3, 2014

Merge pull request #14582 from arthurnn/blacklist_ruby_methods

1778b07

Blacklist ruby keywords for scopes Conflicts: activerecord/CHANGELOG.md

khustochka mentioned this pull request Apr 9, 2014

Rails 4.1.0 no longer allows to define scope named as reserved methods (public, private etc) #14655

Closed

arthurnn deleted the blacklist_ruby_methods branch April 11, 2014 01:05

gsamokovarov mentioned this pull request May 8, 2014

Can't have a scope called public anymore with Rails 4.1? #15032

Closed

myabc added a commit to myabc/openproject that referenced this pull request Jun 29, 2015

Rename Project.public scope to public_projects

75b2f9f

Avoid collision with `public` class method. As of Rails 4.1, `public` is explicitly black-listed as a scope name: rails/rails#14582

myabc added a commit to myabc/openproject that referenced this pull request Jun 29, 2015

Rename Project.public scope to public_projects

265c86f

Avoid collision with `public` class method. As of Rails 4.1, `public` is explicitly black-listed as a scope name: rails/rails#14582

myabc added a commit to myabc/openproject that referenced this pull request Jun 30, 2015

Rename Project.public scope to public_projects

ecfa05f

Avoid collision with `public` class method. As of Rails 4.1, `public` is explicitly black-listed as a scope name: rails/rails#14582

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blacklist ruby keywords for scopes #14582

Blacklist ruby keywords for scopes #14582

arthurnn commented Apr 3, 2014

arthurnn Apr 3, 2014

rafaelfranca Apr 3, 2014

arthurnn Apr 3, 2014

arthurnn commented Apr 3, 2014

fw42 commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

fw42 commented Apr 3, 2014

chancancode commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

chancancode commented Apr 3, 2014

arthurnn commented Apr 3, 2014

chancancode commented Apr 3, 2014

Let me know, I can change the list content.

rafaelfranca commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

arthurnn commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

arthurnn commented Apr 3, 2014

wincent commented Apr 11, 2014

arthurnn commented Apr 11, 2014

wincent commented Apr 11, 2014

chancancode commented Apr 11, 2014

Blacklist ruby keywords for scopes #14582

Blacklist ruby keywords for scopes #14582

Conversation

arthurnn commented Apr 3, 2014

arthurnn Apr 3, 2014

Choose a reason for hiding this comment

rafaelfranca Apr 3, 2014

Choose a reason for hiding this comment

arthurnn Apr 3, 2014

Choose a reason for hiding this comment

arthurnn commented Apr 3, 2014

fw42 commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

fw42 commented Apr 3, 2014

chancancode commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

chancancode commented Apr 3, 2014

arthurnn commented Apr 3, 2014

chancancode commented Apr 3, 2014

Let me know, I can change the list content.

rafaelfranca commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

arthurnn commented Apr 3, 2014

rafaelfranca commented Apr 3, 2014

arthurnn commented Apr 3, 2014

wincent commented Apr 11, 2014

arthurnn commented Apr 11, 2014

wincent commented Apr 11, 2014

chancancode commented Apr 11, 2014