Fix relation.create to avoid leaking scope to initialization block and callbacks

[kamipo]

relation.create populates scope attributes to new record by scoping,
it is necessary to assign the scope attributes to the record and to find
STI subclass from the scope attributes.

But the effect of scoping is class global, it was caused undesired
behavior that pollute all class level querying methods in initialization
block and callbacks (after_initialize, before_validation,
before_save, etc), which are user provided code.

To avoid the leaking scope issue, restore the original current scope
before initialization block and callbacks are invoked.

Fixes #9894.
Fixes #17577.
Closes #31526.

[rafaelfranca]

Would not this cause breaking changes?

[rafaelfranca]

It feels to me that this is being done at the wrong level. Relation should not know about scoping. Maybe this should be done in the scoping class?

cc @matthewd

[kamipo]

I agree that here is not best place to do this.
But Relation has scoping method, and already need to know the result of klass.current_scope(true) in some places (in scoping, spawn, etc), so I just do this here since here is only place to do this.

If we'd not like to do this here, an option is just extracting to method in the scoping class like the following:

diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 32f0609798..347d745d19 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -67,11 +67,7 @@ def bind_attribute(name, value) # :nodoc:
     #   user = users.new { |user| user.name = 'Oscar' }
     #   user.name # => Oscar
     def new(attributes = nil, &block)
-      current_scope = klass.current_scope(true)
-      block = -> record do
-        klass.current_scope = current_scope
-        yield record if block_given?
-      end
+      block = klass.current_scope_restoring_block(&block)
       scoping { klass.new(attributes, &block) }
     end
 
diff --git a/activerecord/lib/active_record/scoping.rb b/activerecord/lib/active_record/scoping.rb
index 35e9dcbffc..1142a87d25 100644
--- a/activerecord/lib/active_record/scoping.rb
+++ b/activerecord/lib/active_record/scoping.rb
@@ -30,6 +30,14 @@ def current_scope(skip_inherited_scope = false)
       def current_scope=(scope)
         ScopeRegistry.set_value_for(:current_scope, self, scope)
       end
+
+      def current_scope_restoring_block(&block)
+        current_scope = self.current_scope(true)
+        -> *args do
+          self.current_scope = current_scope
+          yield(*args) if block_given?
+        end
+      end
     end
 
     def populate_with_current_scope_attributes # :nodoc:

[rafaelfranca]

That could be a nice refactoring

[kamipo]

Applied 4cb1438.

[kamipo]

Would not this cause breaking changes?

Before this PR #35186, the self.total_count = Bird.count in after_initialize and the Bird.find_by!(name: "canary") are affected by Bird.where(color: "green").scoping { } in Bird.where(color: "green").create, so parrot.total_count will be 0 and Bird.find_by!(name: "canary") will raise RecordNotFound.

canary = Bird.create!(color: "yellow", name: "canary")
parrot = Bird.where(color: "green").create do |bird|
  bird.name = "parrot"
  assert_equal canary, Bird.find_by!(name: "canary")
end
assert_equal 1, parrot.total_count

If anyone depends on the behavior, it is affected by this change.
But I think the leaking scope is just a bug, probably most people use a workaround Bird.unscoped.count and Bird.unscoped.find_by!(name: "canary") for a long time.

[rafaelfranca]

👍

[zarelit]

@kamipo Thank you for the fix :-)

[rafaelfranca]

I'm still concerned about this breaking change. If people is relying on this behavior they may start to leak information silently like happened some years ago in the GitHub incident.

Is there any way we can detect people is relying on this behavior and show them a warning?

@tenderlove @matthewd @jeremy what do you think?

[tenderlove]

Please correct me if I'm wrong, but if the scope is leaking, then the scope in after_initialize etc would depend on the context in which the object is created. People depending on this behavior would have to have exactly one code path for initialization, otherwise they would have to do unscoped because the scope would be different each time.

Could we set the scope object to something that raises an exception if unscoped isn't called?

[kamipo]

Since scoping is public API, people may have a usage like the case (1).
In the case (1), initialization block and callbacks are affected by the Bird.where(color: "yellow").scoping.
I made the case (2) working the same with the case (1) in this PR.

We could set the exception scope object for the case (2), it will make the case (2) working different back with the case (1) though...

canary = Bird.create!(color: "yellow", name: "canary")

# (1)
Bird.where(color: "yellow").scoping do
  parrot = Bird.create!(color: "green") do |bird|
    bird.name = "parrot"
    assert_equal canary, Bird.find_by!(name: "canary")
  end
  assert_equal 1, parrot.total_count
end

# (2)
Bird.where(color: "yellow").scoping do
  parrot = Bird.rewhere(color: "green").create! do |bird|
    bird.name = "parrot"
    assert_equal canary, Bird.find_by!(name: "canary")
  end
  assert_equal 1, parrot.total_count
end