Skip to content

Commit c08c2ef

Browse files
committed
Refactor ActiveRecord::TokenFor to not rely on relation delegation
Ref: rails#50396 Ref: rails#51776 `ActiveRecord::Relation` automatically delegates missing methods to the model class wrapped in a `scoping { }` block. This is to support scoping in user defined class methods. The problem however is that it's very error prone for the framework, because we can mistakenly call model methods from inside `Relation` and not realized we're applying a global scope. In the best case scenario it's just a waste of performance, but it can also lead to bugs like rails#51775 I'm planning to restrict this automatic delegation to methods defined in childs of `ActiveRecord::Base` only: rails#50396 but for this to work we must first refactor any Rails code that rely on it.
1 parent 41888b1 commit c08c2ef

File tree

2 files changed

+23
-13
lines changed

2 files changed

+23
-13
lines changed

activerecord/lib/active_record/relation.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ def exec_explain(&block)
6666

6767
include Enumerable
6868
include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches, Explain, Delegation
69-
include SignedId::RelationMethods
69+
include SignedId::RelationMethods, TokenFor::RelationMethods
7070

7171
attr_reader :table, :klass, :loaded, :predicate_builder
7272
attr_accessor :skip_preloading_value

activerecord/lib/active_record/token_for.rb

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,24 @@ def resolve_token(token)
3535
end
3636
end
3737

38+
module RelationMethods
39+
# Finds a record using a given +token+ for a predefined +purpose+. Returns
40+
# +nil+ if the token is invalid or the record was not found.
41+
def find_by_token_for(purpose, token)
42+
raise UnknownPrimaryKey.new(self) unless model.primary_key
43+
model.token_definitions.fetch(purpose).resolve_token(token) { |id| find_by(model.primary_key => id) }
44+
end
45+
46+
# Finds a record using a given +token+ for a predefined +purpose+. Raises
47+
# ActiveSupport::MessageVerifier::InvalidSignature if the token is invalid
48+
# (e.g. expired, bad format, etc). Raises ActiveRecord::RecordNotFound if
49+
# the token is valid but the record was not found.
50+
def find_by_token_for!(purpose, token)
51+
model.token_definitions.fetch(purpose).resolve_token(token) { |id| find(id) } ||
52+
(raise ActiveSupport::MessageVerifier::InvalidSignature)
53+
end
54+
end
55+
3856
module ClassMethods
3957
# Defines the behavior of tokens generated for a specific +purpose+.
4058
# A token can be generated by calling TokenFor#generate_token_for on a
@@ -85,20 +103,12 @@ def generates_token_for(purpose, expires_in: nil, &block)
85103
self.token_definitions = token_definitions.merge(purpose => TokenDefinition.new(self, purpose, expires_in, block))
86104
end
87105

88-
# Finds a record using a given +token+ for a predefined +purpose+. Returns
89-
# +nil+ if the token is invalid or the record was not found.
90-
def find_by_token_for(purpose, token)
91-
raise UnknownPrimaryKey.new(self) unless primary_key
92-
token_definitions.fetch(purpose).resolve_token(token) { |id| find_by(primary_key => id) }
106+
def find_by_token_for(purpose, token) # :nodoc:
107+
all.find_by_token_for(purpose, token)
93108
end
94109

95-
# Finds a record using a given +token+ for a predefined +purpose+. Raises
96-
# ActiveSupport::MessageVerifier::InvalidSignature if the token is invalid
97-
# (e.g. expired, bad format, etc). Raises ActiveRecord::RecordNotFound if
98-
# the token is valid but the record was not found.
99-
def find_by_token_for!(purpose, token)
100-
token_definitions.fetch(purpose).resolve_token(token) { |id| find(id) } ||
101-
(raise ActiveSupport::MessageVerifier::InvalidSignature)
110+
def find_by_token_for!(purpose, token) # :nodoc:
111+
all.find_by_token_for!(purpose, token)
102112
end
103113
end
104114

0 commit comments

Comments
 (0)