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

Conversation

berniechiu
Copy link
Contributor

@berniechiu berniechiu commented Jun 10, 2022

Summary

Currently the method insert_all! is missing the arguments to apply unique_by in cases when we need to use them, not sure why it has the inconsistent interface for calling with ! besides raising error, they should share the same argument interface.

Other Information

@berniechiu berniechiu changed the title Consistent insert_all interfaces Consistent ActiveRecord insert_all Interfaces Jun 10, 2022
@berniechiu berniechiu changed the title Consistent ActiveRecord insert_all Interfaces Consistent ActiveRecord insert_all! Interfaces Jun 10, 2022
@berniechiu berniechiu changed the title Consistent ActiveRecord insert_all! Interfaces Fix Inconsistent ActiveRecord insert_all! Interfaces Jun 10, 2022
@berniechiu berniechiu marked this pull request as ready for review June 10, 2022 10:30
@rails-bot rails-bot bot added the railties label Jun 10, 2022
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

I was reading the original PR and the current documentation and I think that this doesn't support unique_by on purpose. The reason insert_all! exists is to raise if any inserts would violate a unique constraint on the table. By accepting a unique_by option, it defeats the purpose of insert_all!. I think in this case you're supposed to use insert_all instead.

If we do decide to accept this PR it needs tests.

@@ -1,3 +1,7 @@
* Fix `ActiveRecord::Persistence::ClassMethods#insert_all!` not supporting `unique_by` option.
Copy link
Member

Choose a reason for hiding this comment

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

This changelog should be in activerecord/CHANGELOG.md not in Railties.

I also think it should say "ActiveRecord::Persistence::ClassMethods#insert_all! now supports the unique_by option" and include an example of usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up

@berniechiu berniechiu force-pushed the consistent-insert-all-call branch 5 times, most recently from a21f68f to b338aa8 Compare June 15, 2022 15:14
@berniechiu
Copy link
Contributor Author

Should be updated by now : )

{ 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

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

Successfully merging this pull request may close these issues.

None yet

2 participants