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

Association reset does not work as expected when you then call save #42637

Open
ollieh-m opened this issue Jun 29, 2021 · 2 comments
Open

Association reset does not work as expected when you then call save #42637

ollieh-m opened this issue Jun 29, 2021 · 2 comments

Comments

@ollieh-m
Copy link

ollieh-m commented Jun 29, 2021

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 :publishers, force: true do |t|
    t.integer :post_id
    t.datetime :expires_at
  end
end

class Post < ActiveRecord::Base
  has_one :active_publisher, -> { active }, class_name: "Publisher"
end

class Publisher < ActiveRecord::Base
  def self.active
    where(expires_at: nil).or(expiring)
  end

  def self.expiring
    where("expires_at > ?", Time.current)
  end
end

class BugTest < Minitest::Test
  # this fails
  def test_association_reset_then_save
    post = Post.create
    post.create_active_publisher
    post.active_publisher.update(expires_at: Time.current)
    post.association(:active_publisher).reset

    # this loads the `active_publisher` association using the already-set `@association_scope`,
    # which now has a stale timestamp - the publisher has already expired
    # but its expiry is in the future according to the timestamp set in `@association_scope`
    post.save

    assert_nil post.active_publisher
  end

  # this passes
  def test_association_reset
    post = Post.create
    post.create_active_publisher
    post.active_publisher.update(expires_at: Time.current)

    post.association(:active_publisher).reset

    assert_nil post.active_publisher
  end
end

Expected behavior

I think both the tests in the above test case should pass, but only one does.

When we load an association (active_publisher) on an object (post), then reset that association (post.association(:active_publisher).reset), the next time the association is loaded should trigger a fresh query to find any associated record. That fresh query should invoke the association's scope realtime, so if the scope refers to Time.current, that should evaluate to the time when the association is loaded (not the time when the association was first loaded).

Actual behavior

After loading an association on an object then resetting the association, calling save on the object has the effect of loading the association again (as part of AutosaveAssocation#save_has_one_association). When the association is loaded in this way, the query to find the associated record is based on the scope as it was evaluated originally. The conditions are not re-evaluated, meaning they can be stale. In this example, an expired publisher is returned as if its expiry is still in the future, because the timestamp in the scope is now out-of-date.

The implication is (as an example) that if we do the following:

  • post.create_active_publisher
  • post.active_publisher.update(expires_at: Time.current)
  • post.assocation(:active_publisher).reset
  • post.save
  • post.create_active_publisher

the original publisher will be deleted or its foreign key set to nil, because it will be handled as if it is still an active_publisher the second time post.create_active_publisher is called.

A workaround is to call post.association(:active_publisher).reset_scope as well as post.association(:active_publisher).reset, but would it be less surprising for reset to include reset_scope automatically?

System configuration

Rails version: 7.0.0.alpha

Ruby version: 2.7.0

@ghiculescu
Copy link
Member

but would it be less surprising for reset to include reset_scope automatically?

I think so, but it's hard to know for sure without trying it out. Do you want to have a go at that and see if it causes any issues?

@ollieh-m
Copy link
Author

I think so, but it's hard to know for sure without trying it out. Do you want to have a go at that and see if it causes any issues?

Thanks @ghiculescu. I gave it a go and didn't break any specs, so have opened #42643...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants