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

When skipping duplicates in bulk insert on MySQL, avoid assigning an AUTONUMBER column when not specified #35854

Merged
merged 1 commit into from Apr 8, 2019

Conversation

Projects
None yet
2 participants
@boblail
Copy link
Contributor

commented Apr 4, 2019

Summary

If id is an AUTONUMBER column, then my former strategy of assigning no_op_column to an arbitrary column would fail in this specific scenario:

  1. model.columns.first is an AUTONUMBER column
  2. model.columns.first is not assigned in the insert attributes

I added three tests: the first test covers the actual error; the second test documents that this isn't a problem when a value is given for the AUTONUMBER column and the third test ensures that this no-op strategy to achieve skip duplicates for MySQL isn't secretly doing an UPSERT.

Reproduce Locally

To see the exception, you can run this script:

# 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: "master"
  gem "mysql2"
end

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

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

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.string :title, null: false
    t.string :slug, null: false
    t.string :author, null: false

    t.index :slug, unique: true
  end
end

class Article < ActiveRecord::Base; end

class BugTest < Minitest::Test
  def test_upsert_all
    Article.insert_all(
      [
        { title: "Article 1", slug: "article", author: "Jane" },
        { title: "Article 2", slug: "article", author: "John" }
      ]
    )

    assert_equal 1, Article.count
  end
end

@rails-bot rails-bot bot added the activerecord label Apr 4, 2019

@boblail boblail force-pushed the boblail:fix-bug-with-insert_all-on-mysql branch from eca4010 to a12cd32 Apr 4, 2019

@boblail boblail force-pushed the boblail:fix-bug-with-insert_all-on-mysql branch from a12cd32 to 64af662 Apr 5, 2019

end
end

def test_mysql_skip_duplicates_strategy_does_not_secretly_upsert

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 5, 2019

Member

Let's remove mysql_.

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 5, 2019

Member

Also skip unless supports_insert_on_duplicate_skip? is needed.

https://travis-ci.org/rails/rails/jobs/515997440#L1214

This comment has been minimized.

Copy link
@boblail

boblail Apr 5, 2019

Author Contributor

No problem! Change made.

I just wanted to confirm, though — these tests all specifically cover MySQL's strategy for skipping duplicates — its strategy of attempting a no-op upsert.

And this one — which asserts that "skip_duplicates_strategy_does_not_secretly_upsert" — is kind of a confusing thing to assert of the other connection adapters.

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 5, 2019

Member

Yeah, whatever the strategy we should test just behavior, better naming is welcome.

@boblail boblail force-pushed the boblail:fix-bug-with-insert_all-on-mysql branch from 64af662 to 307b256 Apr 5, 2019

Show resolved Hide resolved activerecord/test/cases/insert_all_test.rb Outdated
Show resolved Hide resolved activerecord/test/cases/insert_all_test.rb Outdated
When skipping duplicates in bulk insert on MySQL, avoid assigning id …
…when not specified

If `id` is an `AUTONUMBER` column, then my former strategy here of assigning `no_op_column` to an arbitrary column would fail in this specific scenario:

1. `model.columns.first` is an AUTONUMBER column
2. `model.columns.first` is not assigned in the insert attributes

I added three tests: the first test covers the actual error; the second test documents that this _isn't_ a problem when a value is given for the AUTONUMBER column and the third test ensures that this no-op strategy isn't secretly doing an UPSERT.

@boblail boblail force-pushed the boblail:fix-bug-with-insert_all-on-mysql branch from 7277ee6 to c24efdb Apr 8, 2019

@kamipo kamipo merged commit 652ce83 into rails:master Apr 8, 2019

3 checks passed

buildkite/rails Build #60216 passed (15 minutes, 40 seconds)
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.