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 same length values #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sergeykish
Copy link
Contributor

ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  VALUES lists must all be the same length

@sergeykish
Copy link
Contributor Author

That's a sketch, I'll make a cleanup

@sergeykish
Copy link
Contributor Author

Disabled :disable_timestamps option

@sergeykish
Copy link
Contributor Author

Returned back :disable_timestamps

@sergeykish
Copy link
Contributor Author

@bjhaid I'm not sure how to deal with previous rails versions

def self._bulk_insert_quote(key, value)

@bjhaid
Copy link
Owner

bjhaid commented May 3, 2017

@sergeykish I am not sure I understand what problem you are attempting to solve with this PR.

@sergeykish
Copy link
Contributor Author

@bjhaid got it! I've filled in #12 and created spec

@bjhaid
Copy link
Owner

bjhaid commented May 4, 2017

Have you verified what you have here:

https://github.com/bjhaid/active_record_bulk_insert/pull/11/files#diff-8ac167c90faec485f35cdf33a5dfdba8R19

actually creates the records in the DB with the correct fields, I think this PR also introduces potential bugs, have you tried it with a hash that contains extraneous keys?

BTW I think ensuring that the hashes passed in contains the same set of keys and a default supplied to normalize hashes with fewer keys should be the job of whatever calls bulk_insert and not bulk_insert.

@sergeykish
Copy link
Contributor Author

Have you verified ... actually creates the records in the DB with the correct fields

Sure, I already use code in production

records = [ 
  { :name => "Foo", :age => 30 },
  { :name => "Bar" } 
]
SampleRecord.bulk_insert(records)

produces

  INSERT INTO "sample_records"
    (name, age, created_at, updated_at)
  VALUES
    ('Foo', 30, '2017-05-05 09:08:30.818160', '2017-05-05 09:08:30.818160'),('Bar', NULL, '2017-05-05 09:08:30.818297', '2017-05-05 09:08:30.818297

have you tried it with a hash that contains extraneous keys

they are not present in the columns and thus ignored

SampleRecord.bulk_insert([{:missing => "Bar"}])

produces

  INSERT INTO "sample_records"
    (name, age, created_at, updated_at)
  VALUES
    (NULL, NULL, '2017-05-05 09:05:31.646982', '2017-05-05 09:05:31.646982')

@bjhaid
Copy link
Owner

bjhaid commented May 5, 2017

If the column has not null constraints then the query blows up, which is why I think determining what default to use is not the job of bulk_insert, a more adequate solution might be to reject hashes with incomplete keys instead of generating a bad query

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

Successfully merging this pull request may close these issues.

None yet

2 participants