Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Add a transaction wrapper in add_to_target. This means that #build wi…

…ll now also use a transaction. IMO this is reasonable given that the before_add and after_add callbacks might do anything, and this great consistency allows us to abstract out the duplicate code from #build and #create.
  • Loading branch information...
commit b9ea751d0e56bd00d341766977a607ed3f7ddd0f 1 parent 5d6d669
@jonleighton jonleighton authored
Showing with 27 additions and 27 deletions.
  1. +27 −27 activerecord/lib/active_record/associations/association_collection.rb
View
54 activerecord/lib/active_record/associations/association_collection.rb
@@ -56,31 +56,15 @@ def reset
end
def build(attributes = {}, &block)
- if attributes.is_a?(Array)
- attributes.collect { |attr| build(attr, &block) }
- else
- add_to_target(build_record(attributes)) do |record|
- yield(record) if block_given?
- set_owner_attributes(record)
@josevalim Owner

The build_or_create code was not considering this line, added the regression described in #479.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- end
- end
+ build_or_create(attributes, :build, &block)
end
- def create(attributes = {})
+ def create(attributes = {}, &block)
unless @owner.persisted?
raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved"
end
- if attributes.is_a?(Array)
- attributes.collect { |attr| create(attr) }
- else
- transaction do
- add_to_target(build_record(attributes)) do |record|
- yield(record) if block_given?
- insert_record(record)
- end
- end
- end
+ build_or_create(attributes, :create, &block)
end
def create!(attrs = {}, &block)
@@ -354,17 +338,20 @@ def load_target
end
def add_to_target(record)
- callback(:before_add, record)
- yield(record) if block_given?
+ transaction do
+ callback(:before_add, record)
+ yield(record) if block_given?
- if @reflection.options[:uniq] && index = @target.index(record)
- @target[index] = record
- else
- @target << record
+ if @reflection.options[:uniq] && index = @target.index(record)
+ @target[index] = record
+ else
+ @target << record
+ end
+
+ callback(:after_add, record)
+ set_inverse_instance(record)
end
- callback(:after_add, record)
- set_inverse_instance(record)
record
end
@@ -425,6 +412,19 @@ def merge_target_lists(loaded, existing)
end + existing
end
+ def build_or_create(attributes, method)
+ records = Array.wrap(attributes).map do |attrs|
+ record = build_record(attrs)
+
+ add_to_target(record) do
+ yield(record) if block_given?
+ insert_record(record) if method == :create
+ end
+ end
+
+ attributes.is_a?(Array) ? records : records.first
+ end
+
# Do the relevant stuff to insert the given record into the association collection.
def insert_record(record, validate = true)
raise NotImplementedError

6 comments on commit b9ea751

@josevalim
Owner

This is very poorly optimized. It is very common to build an object at least two times before finally creating it and now we will be triggering transactions in the DB without no need at all.

@josevalim
Owner

Reverted here:

0ceb21e

@jonleighton
Collaborator

Sorry :(

@josevalim
Owner

Having one commit that does not work well for each hundred of your awesome commits is a price worth to pay. :D

@jonleighton
Collaborator

Bro, you gonna make me weep ;)

@josevalim
Owner

Yeah, I do this to people.

Please sign in to comment.
Something went wrong with that request. Please try again.