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

Scoping reserved names #31179

Merged
merged 1 commit into from Nov 28, 2017
Merged

Scoping reserved names #31179

merged 1 commit into from Nov 28, 2017

Conversation

@kinnrot
Copy link
Contributor

@kinnrot kinnrot commented Nov 18, 2017

Summary

Suggestion for resolving #31120
Don't allow creating scopes named same as ActiveRecord::Relation instance method

@rails-bot
Copy link

@rails-bot rails-bot commented Nov 18, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (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.

@@ -171,6 +171,12 @@ def scope(name, body, &block)
"a class method with the same name."
end

if Relation.instance_methods.include?(name)
Copy link
Member

@eugeneius eugeneius Nov 18, 2017

Relation.instance_methods also includes all instance methods defined on Object. We shouldn't reject those; declaring a scope named object_id works just fine, for example, even if it's an odd thing to do.

I think method_defined_within?(name, Relation) will do the right thing here.

@@ -151,6 +151,22 @@ def test_scopes_body_is_a_callable
assert_equal "The scope body needs to be callable.", e.message
end

def test_scopes_name_is_ralation_method
Copy link
Member

@eugeneius eugeneius Nov 18, 2017

ralation -> relation

end

[:public_method, :protected_method, :private_method].each do |reserved_method|
[:attribute_was, :attribute_changed?, :changed].each do |reserved_method|
Copy link
Member

@eugeneius eugeneius Nov 18, 2017

If you use method_defined_within? as I suggested above, these won't need to change.

@kinnrot
Copy link
Contributor Author

@kinnrot kinnrot commented Nov 19, 2017

@eugeneius 10x, If you already looked at this code, What do you think about a more actionable message when this issue occurs? How about something that explain's how to cancel auto scope generation from enum or any other places that scopes are auto generated on rail load?

@eugeneius
Copy link
Member

@eugeneius eugeneius commented Nov 19, 2017

It looks like ActiveRecord::Enum has its own logic to reject dangerous methods: https://github.com/rails/rails/blob/v5.1.4/activerecord/lib/active_record/enum.rb#L219

I think we should extend that to cover instance methods on Relation too.

I'm not aware of any other places where Active Record automatically creates scopes for you, so I think adding the enum-specific check should be enough to give users reasonably useful error messages.

@kinnrot
Copy link
Contributor Author

@kinnrot kinnrot commented Nov 21, 2017

@eugeneius sounds right, I added the check, so what is the way to tell rails to not create a scope for a enum field named 'record' for example ?

@eugeneius
Copy link
Member

@eugeneius eugeneius commented Nov 22, 2017

Nice one 😊 could you add a test for the enum validation too?

It's not currently possible to suppress enum scopes; that sounds like a reasonable addition to me, but I think you should wait for a maintainer to review what you have here already and then follow up with a separate pull request afterwards if they're 👍 on the idea.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Can you add a CHANGELOG entry and squash your commits after it?

enum category: [:other, value]
end
end
assert_match(/You tried to define a scope named .* on the model/, e.message)
Copy link
Member

@eugeneius eugeneius Nov 23, 2017

This should be raising an enum-specific message ("You tried to define an enum named [...]"), which suggests that the new logic there isn't correct.

@@ -225,6 +225,8 @@ def detect_enum_conflict!(enum_name, method_name, klass_method = false)
raise_conflict_error(enum_name, method_name)
elsif !klass_method && method_defined_within?(method_name, _enum_methods_module, Module)
raise_conflict_error(enum_name, method_name, source: "another enum")
elsif !klass_method && method_defined_within?(method_name, Relation)
Copy link
Member

@eugeneius eugeneius Nov 23, 2017

When this method is called to detect scope name conflicts, klass_method is true:
https://github.com/rails/rails/blob/v5.1.4/activerecord/lib/active_record/enum.rb#L197

A scope is a class method on the model, even if we sometimes access it as an instance method on Relation.

Copy link
Contributor Author

@kinnrot kinnrot Nov 24, 2017

Oh I see, can you show me where is the code that allows accessing the scope via a relation instance?

@kinnrot kinnrot force-pushed the scoping-reserved-names branch 2 times, most recently from 695e4a2 to f87c4cb Nov 24, 2017
@@ -425,6 +425,12 @@
Previously this method always returned an empty array.

*Kevin McPhillips*

* Fix inconsistency when using scope name same as instance method of `ActiveRecord::Relation` class.
Copy link
Member

@eugeneius eugeneius Nov 25, 2017

"Fix inconsistency" sounds like we made this work correctly, when really we don't allow it any more. How about:

Don't allow scopes to be defined which conflict with instance methods on Relation.

New changelog entries should be added at the top of the file.

Copy link
Contributor Author

@kinnrot kinnrot Nov 26, 2017

sounds good

@@ -225,6 +225,8 @@ def detect_enum_conflict!(enum_name, method_name, klass_method = false)
raise_conflict_error(enum_name, method_name)
elsif !klass_method && method_defined_within?(method_name, _enum_methods_module, Module)
raise_conflict_error(enum_name, method_name, source: "another enum")
elsif klass_method && method_defined_within?(method_name, Relation)
raise_conflict_error(enum_name, method_name, source: ActiveRecord::Relation.name)
Copy link
Member

@eugeneius eugeneius Nov 25, 2017

The error raised here currently says:

[...] this will generate a instance method "records", which is already defined by ActiveRecord::Relation.

We should pass type: "class", so that it says:

[...] this will generate a class method "records"

Copy link
Member

@eugeneius eugeneius Nov 25, 2017

What do you think of moving this code up by 4 lines, so that both of the if klass_method checks are next to each other? It might have been easier to spot the type: "class" inconsistency if that was the case.

Copy link
Member

@eugeneius eugeneius Nov 25, 2017

We're referencing Relation on the line above, and ActiveRecord::Relation here; we can use Relation in both places, since this code is inside the ActiveRecord module.

Copy link
Contributor Author

@kinnrot kinnrot Nov 26, 2017

Thanks man!

one question if I may,
When I call a model scope method, Don't I get a relation instance, and when calling a scope method on this relation, it's a class method, where does the delegation happen ?

Copy link
Member

@eugeneius eugeneius Nov 26, 2017

Relation#method_missing delegates methods to the model class if it responds to them:
https://github.com/rails/rails/blob/v5.1.4/activerecord/lib/active_record/relation/delegation.rb#L88-L90

@kaspth
Copy link
Member

@kaspth kaspth commented Nov 25, 2017

@rails-bot rails-bot assigned rafaelfranca and unassigned kaspth Nov 25, 2017

Fixes #31120.

*kinnrot*
Copy link
Member

@eugeneius eugeneius Nov 26, 2017

New changelog entries should be added at the top of the file.

@@ -425,6 +431,5 @@
Previously this method always returned an empty array.

*Kevin McPhillips*



Copy link
Member

@eugeneius eugeneius Nov 26, 2017

Can you undo the whitespace changes here, and squash your commits again? After that, this looks good to me - thanks for being so responsive to feedback @kinnrot! 🙏

Copy link
Contributor Author

@kinnrot kinnrot Nov 27, 2017

is that ok?

Copy link
Member

@eugeneius eugeneius Nov 27, 2017

There's still a diff here, but I'm not sure if it's a blocker for merging - maybe @rafaelfranca can let us know.

Copy link
Member

@kamipo kamipo Nov 27, 2017

This diff can easily be removed by deleting the four extra whitespaces and adding one blank line.

@kinnrot kinnrot force-pushed the scoping-reserved-names branch 2 times, most recently from a8bdc1c to 7247816 Nov 27, 2017
@kinnrot
Copy link
Contributor Author

@kinnrot kinnrot commented Nov 27, 2017

@kinnrot kinnrot force-pushed the scoping-reserved-names branch from 7247816 to d0e969a Nov 28, 2017
@kinnrot
Copy link
Contributor Author

@kinnrot kinnrot commented Nov 28, 2017

The changelog thing is good now.

@@ -1,5 +1,11 @@
## Rails 5.2.0.beta1 (November 27, 2017) ##
Copy link
Member

@eugeneius eugeneius Nov 28, 2017

So close 😄 but the new changelog entry needs to be above this line, at the very top of the file.

Rails 5.2.0.beta1 has already been released, so it doesn't make sense to include this change under that heading.

@kinnrot
Copy link
Contributor Author

@kinnrot kinnrot commented Nov 28, 2017

Due to inconsistent behavior when chaining scopes and one scope named after a Relation method

Validation code added in 2 places:

- scope, to prevent problematic scope names.
- enum, cause it tries to auto define scope.
@kinnrot kinnrot force-pushed the scoping-reserved-names branch from d0e969a to 6552ce0 Nov 28, 2017
@rafaelfranca rafaelfranca merged commit 6552ce0 into rails:master Nov 28, 2017
2 checks passed
rafaelfranca added a commit that referenced this issue Nov 28, 2017
@hw676018683
Copy link

@hw676018683 hw676018683 commented Apr 9, 2018

when I write the following code:

  class Attachemnt < ApplicationRecord
    enum extension: {
      unknown: 0,
      image: 1,
      video: 2,
      audio: 3
    }
  end

Get the following error:

You tried to define an enum named "extension" on the model "Attachment", but this will generate a class method "extensions", which is already defined by ActiveRecord::Relation. (ArgumentError)

But I never use something like Attachment.all.extensions. So I think it may be more appropriate to only limit scope.
detect_enum_conflict!(name, name.pluralize, true), the class method name.pluralize should not be limited.

kamipo added a commit to kamipo/rails that referenced this issue Oct 9, 2018
The delegation methods to named scope are defined when `method_missing`
is invoked on the relation.

Since rails#29301, the receiver in the named scope is changed to the relation
like others (e.g. `default_scope`, etc) for consistency.

Most named scopes would be delegated from relation by `method_missing`,
since we don't allow scopes to be defined which conflict with instance
methods on `Relation` (rails#31179). But if a named scope is defined with the
same name as any method on the `superclass` (e.g. `Kernel.open`), the
`method_missing` on the relation is not invoked.

To address the issue, make the delegation methods to named scope is
generated in the definition time.

Fixes rails#34098.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants