Extract Migrator#create and #drop sql into separate methods

[Jens0512]

Fixes #319.

I haven't managed to run the specs (difficulties with docker), but the changes shouldn't change any behaviour, only expose functionality.

[Jens0512]

But alas, as per Murphy's Law, "If something can go wrong, it will." It did. Fix coming right up

[Jens0512]

There, obvious mistake on my end 😅. Fixed now

[robacarp]

@Jens0512 This is a nice added feature. What do you think about adding a test to show that these methods are working as expected?

[Jens0512]

Well, while that's a great idea, I'm not quite great enough at SQL to know what the should return...

[Blacksmoke16]

@Jens0512 Its pretty easy. It would however depend on what the current adapter is. Assuming for a model like:

class Post < Granite::Base
  adapter pg
  table_name posts

  field name : String
  field body : String
  timestamps
end

• MySQL
  • Drop - DROP TABLE IF EXISTS `posts`;
  • Create - CREATE TABLE `posts`( `id` BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY, `name` VARCHAR(255), `body` VARCHAR(255), `created_at` TIMESTAMP DEFAULT CURRENT_TIMESTAMP, `updated_at` TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP)
• PG
  • Drop - DROP TABLE IF EXISTS "posts";
  • Create - CREATE TABLE "posts"( "id" BIGSERIAL, "name" VARCHAR(255), "body" VARCHAR(255), "created_at" TIMESTAMP, "updated_at" TIMESTAMP)
• SqLite
  • Drop - DROP TABLE IF EXISTS "posts";
  • Create - CREATE TABLE "posts"( "id" INTEGER NOT NULL, "name" VARCHAR(255), "body" VARCHAR(255), "created_at" VARCHAR, "updated_at" VARCHAR)

Could pretty much get away with running it, seeing what the output is and use that as the expected string. We can review it afterwards so shouldn't be a problem.

Ideally we would use a model that has all data types in it to fully test the migrator (which i dont think it is atm?).

[robacarp]

@Blacksmoke16 is right about the technique "run the method, copy the output, and assert that the result is always the output".

Typically, I wouldn't say that's a good practice, but in this case we already know it's working (if only by experience). So asserting that the output is consistent will do the job of ensuring that no accidental changes to the output of the methods are introduced without raising a warning.

[Jens0512]

"run the method, copy the output, and assert that the result is always the output"

Sounds good to me, as long as we know the initial output is correct.

Thanks a ton for the help @Blacksmoke16.

Ideally we would use a model that has all data types in it to fully test the migrator.

Shall I create one and put it in spec/spec_models.cr then?

[Blacksmoke16]

@Jens0512 Yea that would be fine. Be sure to also test the PG array types. There is already a model that has them defined. Will have to scope it to run on PG only since the other DBs dont support it. Can reference https://github.com/amberframework/granite/blob/master/spec/granite_spec.cr#L261-L321

[Jens0512]

(Seems there was a model with all types already: Review)

[Jens0512]

The formatting of the generated SQL is terrible to behold haha

Did I miss anything?

[Jens0512]

Oh no, ameba has detected a terrible line of whitespace...

[Jens0512]

Maybe mention somewhere in the README that you should run ameba before you push. Just in case you put a 2-3 extra white spaces anywhere... :D

[Blacksmoke16]

@Jens0512 This is looking great thanks! I remembered there would be two other cases we should test.

1. Natural PK, can use https://github.com/amberframework/granite/blob/master/spec/spec_models.cr#L190
2. UUID PK, can use https://github.com/amberframework/granite/blob/master/spec/spec_models.cr#L351

Would you mind adding specs for those as well?

[Blacksmoke16]

Sorry Satisfactory sidetracked me this weekend. I'll get this reviewed tonight.

@robacarp @eliasjpr If you guys have any input as well, it would be nice.

[Jens0512]

Nothing to be sorry about :)

[Blacksmoke16]

This was good. Took the time to look through each driver's docs and found some discrepancies we should fix. The fixes for the comments are pretty simple, just have to add an override to the that driver's file's TYPES constant. So that when it goes to lookup the type, it would use the driver specific one versus the defaults we setup in base.cr.

Thanks for this!

[Jens0512]

Thanks for the feedback, anything else?

[Blacksmoke16]

Looks good as far as I can tell. Would like another reviewer tho. @drujensen @robacarp @amberframework/granite

[robacarp]

thanks @Jens0512