All Queries Default Scopes can lead to "ignored" updates

[pjambet]

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
    t.integer :sharding_key
    t.timestamps(null: false)
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base

  default_scope -> {
    if Current.sharding_key.present?
      where(sharding_key: Current.sharding_key)
    end
  }, all_queries: true

  belongs_to :post
end

class Current < ActiveSupport::CurrentAttributes
  attribute :sharding_key
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comments << Comment.create!(sharding_key: 1)
    comment = post.comments.first
    comment.reload
    assert comment

    Current.sharding_key = 2

    # a few checks to confirm things work as expected
    assert_equal 0, post.comments.count # 0 because the sharding key filter ignores the existing comment
    assert_equal 0, Comment.count # ditto
    assert_nil Comment.first # ditto
    assert_raises { comment.reload } # raises an exception, makes sense, no records match the conditions

    original_updated_at = comment.updated_at

    comment.update!(updated_at: Time.current) # silently does nothing

    # "Lies" to you, making it look like the record was updated, the in-memory value was changed on the instance but not in the DB
    refute comment.updated_at == original_updated_at

    # Check with the record from the DB, and fails, since nothing was updated
    refute Comment.unscoped { comment.reload.updated_at } == original_updated_at
  end
end

Expected behavior

Either an exception when calling .update! if the actual query ends up updating nothing and/or the values of the attributes that were expected to be changed to not be changed on the instance.

Actual behavior

The call to update! does not fail, the generated query looks like:

UPDATE "comments" SET "updated_at" = ? WHERE "comments"."id" = ? AND "comments"."sharding_key" = ?  [["updated_at", "2022-04-01 15:45:13.927742"], ["id", 1], ["sharding_key", 2]]

Which matches nothing. (Interestingly, using .touch calls a rollback, whereas update sends a commit, not sure why it's different: begin transaction; UPDATE ... ; commit transaction with .update! and begin transaction; UPDATE ...; rollback transaction with .touch

System configuration

Rails version: 7.1.0.alpha from https://github.com/rails/rails.git (at main@3b9be03)

Ruby version: ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [arm64-darwin21]

Final note

A related potential issue — which I'd be happy to create a separate ticket for if useful — is that .unscoped does not seem to work with updates, which feels unexpected, in the example above, I would have expected

Comment.unscoped do
  comment.update!(updated_at: Time.current)
end

to issue the following SQL query:

UPDATE "comments" SET "updated_at" = ? WHERE "comments"."id" = ?  [["updated_at", "2022-04-01 15:55:43.523284"], ["id", 1]]

But it seems like unscoped does not change the behavior of update

cc @eileencodes @kaiyannameighu

[eileencodes]

I've spent some time looking at this and I'm not quite sure what the right fix is. Here's the gist of the problem

The Comment record is already loaded in memory before we switch shards. When update! is called on a record no query is run to make sure the record still exists before attempting an update, Rails assume it must exist. The constraints are created from the original record ID and attributes passed in, plus the default scope if applicable. By the time the default scope is applied we've lost the in-memory record and only have the attributes. We could potentially do a find before the update but that would cause 2 queries to run and I don't think we should do that in the non-default scope case.

I tested out updating non-existent records (regardless of default_scope) to try to find a way to fix this but there's nothing there we can lean on. Here are some examples:

comment = Comment.new
comment.update!(updated_at: Time.now) => inserts the record into the db

comment = Comment.create!
comment.destroy
comment.update!(updated_at: Time.now) => raises cant modify frozen attributes error

The only way I can think to try to fix this is by storing whether the default scope changed (like how we store @destroyed when a record is deleted). But I'm not immediately sure if that can work or would be easy to track without making it super buggy. @destroyed works because destroy is only called from one place on the record itself. But the default_scope isn't stored on the record, it's stored on the class so tracking that it changed is harder.

[Erol]

It doesn't seem like this is only constrained to changing default scopes. If we try to update a record that has been deleted in the DB, this will also silently fail. For example:

topic = Topic.create(name: "New Title")

Topic.find(topic.id).delete

topic.update(title: "Another Title") #=> true
topic.update!(title: "Another New Title") #=> true

It seems like the check done on create_or_update might not necessarily be correct: https://github.com/rails/rails/blob/51b4370bb371a0f1d0823c57cad3a1557bf1e6fe/activerecord/lib/active_record/persistence.rb#L1086-L1091

_update_record returns the number of rows updated, but we're comparing it against false, which would theoretically always be true even if the number of updated rows is 0

I tried to tackle that in https://github.com/rails/rails/pull/44894, but seems like the PR just introduced regressions.

I think one way to work around this would be to use update_columns instead, since it properly checks the number of rows updated:

topic = Topic.create(name: "New Title")

Topic.find(topic.id).delete

topic.update_columns(title: "Another Title") #=> false

[Erol]

@eileencodes I've opened a new PR: https://github.com/rails/rails/pull/44902. May I know your thoughts on it?

[pjambet]

Given that changes the behavior of update doesn't seem obvious, I'm wondering what your thoughts are about my last point, shouldn't unscoped also apply to update calls?

The full example is below, but it's almost identical to the one above, with the exception that the .update call is wrapped in Comment.unscoped:

Full reproductions steps 👇

# frozen_string_literal: true

require "bundler/inline"
require "debug"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
    t.integer :sharding_key
    t.timestamps(null: false)
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base

  default_scope -> {
    if Current.sharding_key.present?
      where(sharding_key: Current.sharding_key)
    end
  }, all_queries: true

  belongs_to :post
end

class Current < ActiveSupport::CurrentAttributes
  attribute :sharding_key
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comments << Comment.create!(sharding_key: 1)
    comment = post.comments.first
    comment.reload
    assert comment

    Current.sharding_key = 2

    # a few checks to confirm most things
    assert_equal 0, post.comments.count # 0 because the sharding key filter ignores the existing comment
    assert_equal 0, Comment.count # ditto
    assert_nil Comment.first # ditto
    assert_raises { comment.reload } # raises an exception, makes sense, no records match the conditions

    original_updated_at = comment.updated_at

    Comment.unscoped do
      comment.update!(updated_at: Time.current) # silently does nothing
    end

    # "Lies" to you, making it look like the record was updated, the in-memory value was changed
    refute comment.updated_at == original_updated_at

    # Check with the record from the DB, and fails, since nothing was updated
    refute Comment.unscoped { comment.reload.updated_at } == original_updated_at
  end
end

[rails-bot[bot]]

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.