Fix Bulk Imports

[Blacksmoke16]

This PR addresses some issues i uncovered recently, removing too much when implementing the batch size, while the tests passed.

1. Re add the extra params for ignore_on_duplicate and update_on_duplicate
2. Add Granite::ORM::Collection(self) typing to model_array param so that records can be fetched from DB, iterated upon, and reimported easily. See This Spec
3. Greatly expands the test suite of bulk imports to cover standard usages, batch_size, ignore_on_duplicate and update_on_duplicate for each of the following:
   • Non AUTO INCREMENT default name PK
   • AUTO INCREMENT default name PK
   • Non AUTO INCREMENT custom name PK
   • AUTO INCREMENT custom name PK

[drujensen]

@Blacksmoke16 can you add tests that verify the timestamps are set? We should verify that created_at and updated_at are properly set when imported.

[Blacksmoke16]

Oo good call, yea can do.

[Blacksmoke16]

@drujensen That ought to be good.

[drujensen]

Can you confirm that on_duplicate_key_update does not change the created_at? If so, this is approved.

[Blacksmoke16]

I'll look into it. Will add a spec for this as well.

[Blacksmoke16]

@drujensen How would i see what sqlite version the tests use?

[drujensen]

good question. In the Docker file, we include the library here:

https://github.com/amberframework/granite/blob/master/Dockerfile#L3

You can run docker-compose run spec bash to get a bash shell inside the docker image. Not sure what command to use to get the sqlite version from there though. Maybe write a small crystal test to run select sqlite_version();?

[Blacksmoke16]

Am a total noob when it comes to sqlite, best i could do was add sqlite3 package to Dockerfile then run sqlite3 --version and seems it installs 3.11.0.

Anyway, reason i ask is https://www.sqlite.org/draft/releaselog/3_24_0.html. Once that gets released, if we use it, will allow sqlite to support updates like PG does. This'll prob allow the timestamps to get updated in sqlite, so could add sqlite to timestamps spec.

Regardless, we can worry about that later when it actually is released. This PR should be good to go. Got a spec added that checks created_at and updated_at when using bulk imports.

[drujensen]

ok, this needs a second reviewer and then we can merge.

[Blacksmoke16]

I'll update this with new spec stuff, sec.

EDIT: Some things broke, i'll look into it.

[drujensen]

@Blacksmoke16 I resolved some of the conflicts in the spec_models.cr. @maiha added a migrator module that allows you to drop and create databases without specifying the schema.

[Blacksmoke16]

Rgr, seems some of the specs fail now, ill see what's wrong.

[Blacksmoke16]

There is a bug with the migrator that doesn't apply PRIMARY KEY to column that is not AUTO INCREMENT.

I logged out the table_name k/value of the PK by doing
puts "#{@quoted_table_name} - #{k} #{v}"

"kvss" - "k" VARCHAR(255)
"people" - "id" INTEGER NOT NULL PRIMARY KEY
"companies" - "id" INTEGER NOT NULL PRIMARY KEY
"books" - "id" INTEGER NOT NULL PRIMARY KEY
"book_reviews" - "id" INTEGER NOT NULL PRIMARY KEY
"items" - "item_id" VARCHAR(255)
"non_auto_default_pk" - "id" INTEGER
"non_auto_custom_pk" - "custom_id" INTEGER

Notice how all the models with auto: false do not have PRIMARY KEY

[drujensen]

@Blacksmoke16 ahhh, yup. Let me push a fix for that. thanks!

[drujensen]

Here #203 is a fix for the PRIMARY KEY auto:false

[drujensen]

@Blacksmoke16 go ahead and rebase. Let's get this merged.