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

Fix Inconsistent ActiveRecord insert_all! Interfaces #45317

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,21 @@
* `ActiveRecord::Persistence::ClassMethods#insert_all!` now supports `unique_by` option.

```ruby
books = [
{ name: 'A', author_id: 1, price: 100 },
{ name: 'B', author_id: 1, price: 200 },
{ name: 'A', author_id: 1, price: 300 },
]

# Raises ActiveRecord::RecordNotUnique
Book.insert_all!(books, unique_by: [:name, :author_id])

# Returns ActiveRecord::Result
Book.insert_all(books, unique_by: [:name, :author_id])
```

*Bernie Chiu*

* Update `db:prepare` task to load schema when an uninitialized database exists, and dump schema after migrations.

*Ben Sheldon*
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/persistence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,8 @@ def insert!(attributes, returning: nil, record_timestamps: nil)
# { id: 1, title: "Rework", author: "David" },
# { id: 1, title: "Eloquent Ruby", author: "Russ" }
# ])
def insert_all!(attributes, returning: nil, record_timestamps: nil)
InsertAll.new(self, attributes, on_duplicate: :raise, returning: returning, record_timestamps: record_timestamps).execute
def insert_all!(attributes, returning: nil, unique_by: nil, record_timestamps: nil)
InsertAll.new(self, attributes, on_duplicate: :raise, returning: returning, unique_by: unique_by, record_timestamps: record_timestamps).execute
end

# Updates or inserts (upserts) a single record into the database in a
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/insert_all_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ def test_insert_all_raises_on_duplicate_records
end
end

def test_insert_all_raises_on_duplicate_records_when_unqiue_by_is_provided
assert_raise ActiveRecord::RecordNotUnique do
Cart.insert_all! [
{ id: 1, shop_id: 1, title: "My cart" },
{ id: 2, shop_id: 1, title: "My another cart" },
{ id: 1, shop_id: 1, title: "My another cart" },
], unique_by: [:shop_id, :id]
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The counterparts are here that don't raise any error

But actually, it's supported and we're actually logging our custom message in my company.

def test_insert_all_can_skip_duplicate_records
skip unless supports_insert_on_duplicate_skip?
assert_no_difference "Book.count" do
Book.insert_all [{ id: 1, name: "Agile Web Development with Rails" }]
end
end
def test_insert_all_with_skip_duplicates_and_autonumber_id_not_given
skip unless supports_insert_on_duplicate_skip?
assert_difference "Book.count", 1 do
# These two books are duplicates according to an index on %i[author_id name]
# but their IDs are not specified so they will be assigned different IDs
# by autonumber. We will get an exception from MySQL if we attempt to skip
# one of these records by assigning its ID.
Book.insert_all [
{ author_id: 8, name: "Refactoring" },
{ author_id: 8, name: "Refactoring" }
]
end
end


def test_insert_all_returns_ActiveRecord_Result
result = Book.insert_all! [{ name: "Rework", author_id: 1 }]
assert_kind_of ActiveRecord::Result, result
Expand Down