Return primary keys when using blocks or values

[pomier]

In #32, @psloomis did a great job in adding the possibility to retrieve the primary keys inserted in PostGIS and PostgreSQL.

However, this requires to manupulate the Worker instance directly to retrieve the returned ids.

Besides, the provided example in the README.md was not working, as it assumes that the worker instance is returned by the bulk_insert function, which is not. When used with a block or the values argument, the function returns nil. In these cases there is no way of retrieving the inserted ids.

Book.bulk_insert(return_primary_keys: true) do |worker|
   ...
end # => nil
Book.bulk_insert(values: ..., return_primary_keys: true) # => nil

I submit here an improvement proposal which returns the list of inserted ids when the return_primary_keys: true option is provided with a block or the values argument.

Book.bulk_insert(return_primary_keys: true) { |worker| ... } # => [id1, id2, ...]
Book.bulk_insert(values: ..., return_primary_keys: true) # => [id1, id2, ...]

For retro-compatibility purpose, this function continues to return nil when return_primary_keys is false:

Book.bulk_insert { |worker| ... } # => nil
Book.bulk_insert(values: ...) # => nil

[pomier]

I have updated the Pull Request with new updates from master, it should be good to be merged.

[mberlanda]

hey @pomier I updated the test matrix to validate any future change against database instances. Could you please rebase on the master branch and submit a new revision of the PR ? Thanks in advance

[pomier]

hey @pomier I updated the test matrix to validate any future change against database instances. Could you please rebase on the master branch and submit a new revision of the PR ? Thanks in advance

I've just rebased the master branch!

[mberlanda]

The code looks clear, however the build is not passing yet:

https://github.com/jamis/bulk_insert/runs/2216453436

This is mainly due to:

• the lack of support of the feature is not explicit (e.g. I would suggest to raise an argument error when it is not supported)
• the tests on the postgresql adapter are expecting empty arrays instead of the primary keys

I put several comments to address these two points. Feel free to comment and ask questions if anything is not clear

[mberlanda]

This option on non postgresql adapters may lead to some errors like:

Old rails versions

NoMethodError: undefined method `rows' for nil:NilClass
    /home/travis/build/jamis/bulk_insert/lib/bulk_insert/worker.rb:40:in `map'
    /home/travis/build/jamis/bulk_insert/lib/bulk_insert/worker.rb:40:in `inserted_ids'
    /home/travis/build/jamis/bulk_insert/test/bulk_insert/worker_test.rb:163:in `block in <class:BulkInsertWorkerTest>'

Or it may return empty arrays in SQLite and Mysql (only if rails >=6).

In order to avoid confusion and future bugs, I would suggest to fail as soon as possible in case of misconfiguration.

E.g.

bulk_insert/lib/bulk_insert/worker.rb

Lines 22 to 23 in 4a69aff

	@return_primary_keys = return_primary_keys

# Add at line 23
validate_return_primary_keys!

# ---
def validate_return_primary_keys!
  if @return_primary_keys && !@statement_adapter.is_a?(StatementAdapters::PostgreSQLAdapter)
    raise ArgumentError, "Unsupported option 'return_primary_keys' for adapter #{@adapter_name}"
  end
end

[mberlanda]

Here the result_sets for non postgresql adapters would look like

• mysql rails < 6 [ nil ]
• sqlite and mysql rails > 6 [#<ActiveRecord::Result:0x0000563f19834d08 @columns=[], @rows=[], @hash_rows=nil, @column_types={}>]

[mberlanda]

On this test I would follow something similar to:

bulk_insert/test/bulk_insert_test.rb

Lines 34 to 43 in 4a69aff

	# return_primary_keys is not supported for mysql and rails < 5
	# this test ensures that the case is covered in the CI and handled as expected
	if ActiveRecord::VERSION::STRING < "5.0.0" && worker.adapter_name =~ /^mysql/i
	error = assert_raise(ArgumentError) { worker.save! }
	assert_equal error.message, "BulkInsert does not support @return_primary_keys for mysql and rails < 5"
	else
	worker.save!
	assert_equal 1, worker.result_sets.count
	end
	end

if Testing.connection.adapter_name =~ /\APost(?:greSQL|GIS)/i
      worker = BulkInsert::Worker.new(
      Testing.connection,
      Testing.table_name,
      'id',
      %w(greeting age happy created_at updated_at color),
      500,
      false,
      false,
      true
    )
    # put the rest of the test here
else
  assert_raise(ArgumentError) {     worker = BulkInsert::Worker.new(Testing.connection, Testing.table_name, 'id', %w(greeting age happy created_at updated_at color), 500, false, false, true) }

you can eventually also capture the error message as per the example linked and perform a partial matching on the error message

[mberlanda]

the expected result here should be

assert_equal Testing.last(2).map(&:id).sort, worker.inserted_ids.sort

[mberlanda]

same as per the previous comment on skip based on the adapter.

[mberlanda]

the expected result here should be

assert_equal [Testing.last(1).id], output

[mberlanda]

the expected result here should be

assert_equal [ Testing.last(1).id ], worker.inserted_ids