bulk_insert does not return a worker object

[fredwillmore]

I am using version 1.8.1, ruby 2.3.8, rails 4.2.5
I am trying to use non-block mode, passing an array of attribute hashes. My model is named Gap:

worker = Gap.bulk_insert(return_primary_keys: true)

however, bulk_insert returns nil. (block mode also returns nil).
Looking at the code it looks like it only returns a worker if bulk_insert is only called without values or a block:

    def bulk_insert(*columns, values: nil, set_size:500, ignore: false, update_duplicates: false, return_primary_keys: false)
      columns = default_bulk_columns if columns.empty?
      worker = BulkInsert::Worker.new(connection, table_name, primary_key, columns, set_size, ignore, update_duplicates, return_primary_keys)

      if values.present?
        transaction do
          worker.add_all(values)
          worker.save!
        end
        nil
      elsif block_given?
        transaction do
          yield worker
          worker.save!
        end
        nil
      else
        worker
      end
    end

How would I access the worker object in order to look at result_sets?
thanks, fw

[mberlanda]

hi @fredwillmore , it looks like the behaviour you are describing is covered by unit tests https://github.com/jamis/bulk_insert/blob/master/test/bulk_insert_test.rb . Can you confirm you can reproduce it consistently?

[travismiller]

I was surprised by this as well.

The test sets an outer result variable from inside the block. So this is the way to access the worker and result_sets when using values or a block with the existing code.

bulk_insert/test/bulk_insert_test.rb

Lines 9 to 13 in ab5db08

	test "bulk_insert with block should yield worker" do
	result = nil
	Testing.bulk_insert { |worker| result = worker }
	assert_kind_of BulkInsert::Worker, result
	end

However, the README indicates that a worker would be returned when using a block.

bulk_insert/README.md

Lines 176 to 192 in ab5db08

	### Return Primary Keys (PostgreSQL, PostGIS)
	If you want the worker to store primary keys of inserted records, then you can
	use the _return_primary_keys_ option. The worker will store a `result_sets`
	array of `ActiveRecord::Result` objects. Each `ActiveRecord::Result` object
	will contain the primary keys of a batch of inserted records.
	```ruby
	worker = Book.bulk_insert(*destination_columns, return_primary_keys: true) do
	|worker|
	worker.add(...)
	worker.add(...)
	# ...
	end
	worker.result_sets
	```

As mentioned in the original issue comment, the worker is only returned if no values are present and no block is given.

bulk_insert/lib/bulk_insert.rb

Lines 11 to 25 in ab5db08

	if values.present?
	transaction do
	worker.add_all(values)
	worker.save!
	end
	nil
	elsif block_given?
	transaction do
	yield worker
	worker.save!
	end
	nil
	else
	worker
	end

I think it should be appropriate to always return the worker. Maybe something like this?

def bulk_insert #…
  # …

  if values.present?
    transaction do
      worker.add_all(values)
      worker.save!
    end
  elsif block_given?
    transaction do
      yield worker
      worker.save!
    end
  else

  worker
end

[mberlanda]

@jamis do you remember why you choose to return nil when implementing this feature here? 95a0c43

[jamis]

I don't remember exactly. :/ However, looking at the diff, I suspect it was an attempt to make the return value consistent, so that it always returns either a worker object, or nil. I vaguely remember being worried about the consequences of someone trusting the method to return self, and introducing a bug when they got a worker object back instead.