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

Make sure there are no duplicated nested records with create_with #33639

Merged
merged 1 commit into from Sep 11, 2018

Conversation

Projects
None yet
5 participants
@darwin67
Contributor

darwin67 commented Aug 17, 2018

Summary

Fixes #33610

Looks like the issue is happening at populate_with_current_scope_attributes where it shouldn't try to assign the scope_attributes on the initial evaluation, which results in a duplicate record.

e.g.) In the test example, when creating a Man record, the current behaviour with :create_with attribute registered in a Relation is that it will try to create an Interest record before the owner Man, instead of creating Man first, then build Interest based on Man.

making scope_attributes? truly returns boolean should fix it instead returning an empty relation that evaluates to true.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Aug 17, 2018

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

rails-bot commented Aug 17, 2018

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

@darwin67 darwin67 changed the title from 33610 make sure there are no duplicated nested records with create_with to Make sure there are no duplicated nested records with create_with Aug 17, 2018

@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Aug 17, 2018

Contributor

Looks like this is a very specific edge case, so I got it to work by deleting the create_with key in Relation.values when checking for current_scope, but it'll be great if you could provide me some advice for a cleaner / less hacky way to do it.

Contributor

darwin67 commented Aug 17, 2018

Looks like this is a very specific edge case, so I got it to work by deleting the create_with key in Relation.values when checking for current_scope, but it'll be great if you could provide me some advice for a cleaner / less hacky way to do it.

@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Aug 27, 2018

Contributor

Would you mind taking a look? @rafaelfranca

Contributor

darwin67 commented Aug 27, 2018

Would you mind taking a look? @rafaelfranca

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 27, 2018

Member

This fix seems indeed hacky. I think first we need to understand why it started to fail given it was working on 5.1. Can you try to bisect to see which commit introduced this bug?

Member

rafaelfranca commented Aug 27, 2018

This fix seems indeed hacky. I think first we need to understand why it started to fail given it was working on 5.1. Can you try to bisect to see which commit introduced this bug?

@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Aug 27, 2018

Contributor

Sure, how can I install arel 9.0.0.alpha?

I was 3 steps in bisect between v5.1.0 and master and now I'm seeing this when I try to run the reproduction script.
rails/arel doesn't seem to tag it's pre-releases and I don't have a git ref for the arel at the time so I'm not sure how I can get this working.

/var/lib/gems/2.5.0/gems/bundler-1.16.3/lib/bundler/resolver.rb:55:in `rescue in start': Bundler could not find compatible versions for gem "arel": (Bundler::VersionConflict)
  In Gemfile:
    activerecord was resolved to 5.2.0.alpha, which depends on
      arel (= 9.0.0.alpha)

Could not find gem 'arel (= 9.0.0.alpha)', which is required by gem 'activerecord', in any of the sources.

I'm currently on 3d3ec20a485a109bb772cb37382040e25f5d0ded for rails

Contributor

darwin67 commented Aug 27, 2018

Sure, how can I install arel 9.0.0.alpha?

I was 3 steps in bisect between v5.1.0 and master and now I'm seeing this when I try to run the reproduction script.
rails/arel doesn't seem to tag it's pre-releases and I don't have a git ref for the arel at the time so I'm not sure how I can get this working.

/var/lib/gems/2.5.0/gems/bundler-1.16.3/lib/bundler/resolver.rb:55:in `rescue in start': Bundler could not find compatible versions for gem "arel": (Bundler::VersionConflict)
  In Gemfile:
    activerecord was resolved to 5.2.0.alpha, which depends on
      arel (= 9.0.0.alpha)

Could not find gem 'arel (= 9.0.0.alpha)', which is required by gem 'activerecord', in any of the sources.

I'm currently on 3d3ec20a485a109bb772cb37382040e25f5d0ded for rails

@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Aug 27, 2018

Contributor

This is the script I'm running. I have the rails repo as a symlink inside the VM so I can just reference the specific version while bisecting.

#!/usr/bin/env ruby
# frozen_string_literal: true

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

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

  # gem 'rails', path: 'rails'
  gem 'activerecord', path: 'rails'
  gem "minitest"
  gem 'sqlite3'
  # gem 'mysql2'
  # gem "pg"
end

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

# sqlite3
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
# ActiveRecord::Base.establish_connection(adapter: 'mysql2', database: 'test', username: 'root', password: 'root')
# ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: 'test', username: 'vagrant')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :foos do |t|
    t.string :foo_name
  end

  create_table :bars do |t|
    t.string :bar_name
    t.references :foo
  end

  create_table :foobars do |t|
    t.string :foobar_name
    t.references :foo
  end
end

# Define models
class Foo < ActiveRecord::Base
  has_one :foobar
  has_many :bars
  accepts_nested_attributes_for :bars
  accepts_nested_attributes_for :foobar
end

class Bar < ActiveRecord::Base
  belongs_to :foo
end

class Foobar < ActiveRecord::Base
  belongs_to :foo
end

# Test the issue
class BugTest < ActiveSupport::TestCase
  teardown do
    Foo.delete_all
    Bar.delete_all
    Foobar.delete_all
  end

  def test_1
    assert_changes -> { Foo.count }, from: 0, to: 1 do
      assert_changes -> { Bar.count }, from: 0, to: 1 do
        assert_changes -> { Foobar.count }, from: 0, to: 1 do
          Foo.create_with(
            bars_attributes: [{ bar_name: 'bar_test' }],
            foobar_attributes: { foobar_name: 'foobar_test' }
          ).find_or_create_by!(
            foo_name: 'foo_test'
          )
          puts "Number of Bar: #{Bar.count}"
          puts "Number of Foobar: #{Foobar.count}"
        end
      end
    end
  end
end
Contributor

darwin67 commented Aug 27, 2018

This is the script I'm running. I have the rails repo as a symlink inside the VM so I can just reference the specific version while bisecting.

#!/usr/bin/env ruby
# frozen_string_literal: true

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

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

  # gem 'rails', path: 'rails'
  gem 'activerecord', path: 'rails'
  gem "minitest"
  gem 'sqlite3'
  # gem 'mysql2'
  # gem "pg"
end

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

# sqlite3
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
# ActiveRecord::Base.establish_connection(adapter: 'mysql2', database: 'test', username: 'root', password: 'root')
# ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: 'test', username: 'vagrant')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :foos do |t|
    t.string :foo_name
  end

  create_table :bars do |t|
    t.string :bar_name
    t.references :foo
  end

  create_table :foobars do |t|
    t.string :foobar_name
    t.references :foo
  end
end

# Define models
class Foo < ActiveRecord::Base
  has_one :foobar
  has_many :bars
  accepts_nested_attributes_for :bars
  accepts_nested_attributes_for :foobar
end

class Bar < ActiveRecord::Base
  belongs_to :foo
end

class Foobar < ActiveRecord::Base
  belongs_to :foo
end

# Test the issue
class BugTest < ActiveSupport::TestCase
  teardown do
    Foo.delete_all
    Bar.delete_all
    Foobar.delete_all
  end

  def test_1
    assert_changes -> { Foo.count }, from: 0, to: 1 do
      assert_changes -> { Bar.count }, from: 0, to: 1 do
        assert_changes -> { Foobar.count }, from: 0, to: 1 do
          Foo.create_with(
            bars_attributes: [{ bar_name: 'bar_test' }],
            foobar_attributes: { foobar_name: 'foobar_test' }
          ).find_or_create_by!(
            foo_name: 'foo_test'
          )
          puts "Number of Bar: #{Bar.count}"
          puts "Number of Foobar: #{Foobar.count}"
        end
      end
    end
  end
end
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 27, 2018

Member

Try to point to the 9.0.0 release.

Member

rafaelfranca commented Aug 27, 2018

Try to point to the 9.0.0 release.

@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Aug 27, 2018

Contributor

This seems to have introduced it. d4007d5

something to do with the change of scope_for_create

Contributor

darwin67 commented Aug 27, 2018

This seems to have introduced it. d4007d5

something to do with the change of scope_for_create

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 27, 2018

Member

Cool. That seems a better place to fix. cc @kamipo if you have pointers on how the best fix this.

Member

rafaelfranca commented Aug 27, 2018

Cool. That seems a better place to fix. cc @kamipo if you have pointers on how the best fix this.

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Aug 28, 2018

Member

We should not change the scope_attributes?, at least this PR would break the following create_with with scoping use case:

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

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

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

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

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

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :foos, force: true do |t|
    t.string :foo_name
  end

  create_table :bars, force: true do |t|
    t.string :bar_name
    t.references :foo
  end

  create_table :foobars, force: true do |t|
    t.string :foobar_name
    t.references :foo
  end
end

# Define models
class Foo < ActiveRecord::Base
  has_one :foobar
  has_many :bars
  accepts_nested_attributes_for :bars
  accepts_nested_attributes_for :foobar
end

class Bar < ActiveRecord::Base
  belongs_to :foo
end

class Foobar < ActiveRecord::Base
  belongs_to :foo
end

# Test the issue
class BugTest < ActiveSupport::TestCase
  teardown do
    Foo.delete_all
    Bar.delete_all
    Foobar.delete_all
  end

  def test_1
    assert_changes "Foo.count", from: 0, to: 1 do
      assert_changes "Bar.count", from: 0, to: 1 do
        assert_changes "Foobar.count", from: 0, to: 1 do
          Foo.create_with(
            bars_attributes: [{ bar_name: 'bar_test' }],
            foobar_attributes: { foobar_name: 'foobar_test' }
          ).scoping do
            Foo.create!(foo_name: 'foo_test')
          end
          puts "Number of Bar: #{Bar.count}"
          puts "Number of Foobar: #{Foobar.count}"
        end
      end
    end
  end
end
Member

kamipo commented Aug 28, 2018

We should not change the scope_attributes?, at least this PR would break the following create_with with scoping use case:

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

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

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

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

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

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :foos, force: true do |t|
    t.string :foo_name
  end

  create_table :bars, force: true do |t|
    t.string :bar_name
    t.references :foo
  end

  create_table :foobars, force: true do |t|
    t.string :foobar_name
    t.references :foo
  end
end

# Define models
class Foo < ActiveRecord::Base
  has_one :foobar
  has_many :bars
  accepts_nested_attributes_for :bars
  accepts_nested_attributes_for :foobar
end

class Bar < ActiveRecord::Base
  belongs_to :foo
end

class Foobar < ActiveRecord::Base
  belongs_to :foo
end

# Test the issue
class BugTest < ActiveSupport::TestCase
  teardown do
    Foo.delete_all
    Bar.delete_all
    Foobar.delete_all
  end

  def test_1
    assert_changes "Foo.count", from: 0, to: 1 do
      assert_changes "Bar.count", from: 0, to: 1 do
        assert_changes "Foobar.count", from: 0, to: 1 do
          Foo.create_with(
            bars_attributes: [{ bar_name: 'bar_test' }],
            foobar_attributes: { foobar_name: 'foobar_test' }
          ).scoping do
            Foo.create!(foo_name: 'foo_test')
          end
          puts "Number of Bar: #{Bar.count}"
          puts "Number of Foobar: #{Foobar.count}"
        end
      end
    end
  end
end
@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Aug 28, 2018

Contributor

Oh, thank you for pointing that out. I'll add that case to the test as well.
I guess I'll have to look into the create_with_scope and attributes being passed in then.

Contributor

darwin67 commented Aug 28, 2018

Oh, thank you for pointing that out. I'll add that case to the test as well.
I guess I'll have to look into the create_with_scope and attributes being passed in then.

@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Sep 8, 2018

Contributor

Okay, I finally see the issue here. My previous estimates were pretty off target.

Due to the change in d4007d5, attributes being passed into klass.new now includes the create_with_value too.

Which results in double assignment at 2 locations.

# activerecord/lib/active_record/core.rb

    def initialize(attributes = nil)
      self.class.define_attribute_methods
      @attributes = self.class._default_attributes.deep_dup

      init_internals
      initialize_internals_callback # <= here

      assign_attributes(attributes) if attributes # <= here

      yield self if block_given?
      _run_initialize_callbacks
    end

This is usually fine most of the time since double assignment of the same attributes on a record is idempotent. However, if the attributes includes nested attributes (only for has_many), this will result in trying to assign <child>_attributes twice, which then leads to the duplicate record in #33610 .

I've been trying a couple different things like adding an additional flag to scope_for_create, or create a different method values_for_create for create! to consume

    def scope_for_create
      where_values_hash.merge!(create_with_value.stringify_keys)
    end

    def values_for_create(attributes = nil)
      attributes ? scope_for_create.merge!(attributes) : scope_for_create
      # attributes ? where_values_hash.merge!(attributes) : where_values_hash
    end

but will always result in something else breaking.

Any better ideas of how to approach this? @kamipo

Contributor

darwin67 commented Sep 8, 2018

Okay, I finally see the issue here. My previous estimates were pretty off target.

Due to the change in d4007d5, attributes being passed into klass.new now includes the create_with_value too.

Which results in double assignment at 2 locations.

# activerecord/lib/active_record/core.rb

    def initialize(attributes = nil)
      self.class.define_attribute_methods
      @attributes = self.class._default_attributes.deep_dup

      init_internals
      initialize_internals_callback # <= here

      assign_attributes(attributes) if attributes # <= here

      yield self if block_given?
      _run_initialize_callbacks
    end

This is usually fine most of the time since double assignment of the same attributes on a record is idempotent. However, if the attributes includes nested attributes (only for has_many), this will result in trying to assign <child>_attributes twice, which then leads to the duplicate record in #33610 .

I've been trying a couple different things like adding an additional flag to scope_for_create, or create a different method values_for_create for create! to consume

    def scope_for_create
      where_values_hash.merge!(create_with_value.stringify_keys)
    end

    def values_for_create(attributes = nil)
      attributes ? scope_for_create.merge!(attributes) : scope_for_create
      # attributes ? where_values_hash.merge!(attributes) : where_values_hash
    end

but will always result in something else breaking.

Any better ideas of how to approach this? @kamipo

@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Sep 8, 2018

Contributor

Oh, actually it seems to work. But I'm not fully aware of the edge cases. Can you think of anything that this change could break? @kamipo

Contributor

darwin67 commented Sep 8, 2018

Oh, actually it seems to work. But I'm not fully aware of the edge cases. Can you think of anything that this change could break? @kamipo

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Sep 10, 2018

Member

Can you add an extra test like this?

diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index d03b412efb..c14dc23cf3 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -9,6 +9,7 @@
 require "models/author"
 require "models/entrant"
 require "models/developer"
+require "models/project"
 require "models/person"
 require "models/computer"
 require "models/reply"
@@ -1405,6 +1406,16 @@ def test_explicit_create_with
     assert_equal "cock", hens.new.name
   end
 
+  def test_create_with_nested_attributes
+    assert_difference("Project.count", 1) do
+      developers = Developer.where(name: "Aaron")
+      developers = developers.create_with(
+        projects_attributes: [{ name: "p1" }]
+      )
+      developers.create!
+    end
+  end
+
   def test_except
     relation = Post.where(author_id: 1).order("id ASC").limit(1)
     assert_equal [posts(:welcome)], relation.to_a
Member

kamipo commented Sep 10, 2018

Can you add an extra test like this?

diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index d03b412efb..c14dc23cf3 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -9,6 +9,7 @@
 require "models/author"
 require "models/entrant"
 require "models/developer"
+require "models/project"
 require "models/person"
 require "models/computer"
 require "models/reply"
@@ -1405,6 +1406,16 @@ def test_explicit_create_with
     assert_equal "cock", hens.new.name
   end
 
+  def test_create_with_nested_attributes
+    assert_difference("Project.count", 1) do
+      developers = Developer.where(name: "Aaron")
+      developers = developers.create_with(
+        projects_attributes: [{ name: "p1" }]
+      )
+      developers.create!
+    end
+  end
+
   def test_except
     relation = Post.where(author_id: 1).order("id ASC").limit(1)
     assert_equal [posts(:welcome)], relation.to_a
@darwin67

This comment has been minimized.

Show comment
Hide comment
@darwin67

darwin67 Sep 10, 2018

Contributor

Added the suggested test and great catch.
Unfortunately, this will require me to make an ugly change to get things to work. :(

Contributor

darwin67 commented Sep 10, 2018

Added the suggested test and great catch.
Unfortunately, this will require me to make an ugly change to get things to work. :(

@kamipo

kamipo approved these changes Sep 11, 2018

Looks good. Can you squash your commits into one?

Fixes #33610
In order to avoid double assignments of nested_attributes for `has_many`
relations during record initialization, nested_attributes in `create_with`
should not be passed into `klass.new` and have them populate during
`initialize_internals_callback` with scope attributes.

However, `create_with` keys should always have precedence over where
clauses, so if there are same keys in both `create_with` and
`where_values_hash`, the value in `create_with` should be the one that's
used.

@kamipo kamipo merged commit 7e167d0 into rails:master Sep 11, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Sep 11, 2018

Member

Thanks!

Member

kamipo commented Sep 11, 2018

Thanks!

@darwin67 darwin67 deleted the darwin67:33610-duplicated-nested-records-with-create_with branch Sep 11, 2018

kamipo added a commit that referenced this pull request Sep 11, 2018

Merge pull request #33639 from darwin67/33610-duplicated-nested-recor…
…ds-with-create_with

Make sure there are no duplicated nested records with create_with
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment