add `Granite::ORM.migrator` to production codes #172

maiha · 2018-03-31T19:36:12Z

Dry up codes about creating tables in specs, and put it production code as Granite::ORM.migrator.
This helps users

to add tables in specs easily
to create tables in experiment and deployment

class User < Granite::ORM::Base
  adapter mysql
  field name : String
end

User.migrator.create

mysql> describe users;
+-------+--------------+------+-----+---------+----------------+
| Field | Type         | Null | Key | Default | Extra          |
+-------+--------------+------+-----+---------+----------------+
| id    | bigint(20)   | NO   | PRI | NULL    | auto_increment |
| name  | varchar(255) | YES  |     | NULL    |                |
+-------+--------------+------+-----+---------+----------------+

implementation

mapping rules from crystal class to database type is defined in adapters.
- Granite::Adapter::Base::Schema::Types
- Granite::Adapter::Mysql::Schema::Types
- ...

I just defined rules about basic classes used in current specs.

Bool, Float32, Float64, Int32, Int64, String, Time

If migrator is exected with undefined types, it raises a runtime error as below.

Migrator(Granite::Adapter::Mysql) doesn't support 'Time::Span' yet. (Exception)

Best regards,

drujensen

👍 Would like us to consider renaming up -> create and down -> drop to stay inline with drop_and_create. wdyt?

drujensen · 2018-04-01T03:33:03Z

src/granite_orm/migrator.cr

+      up
+    end      
+
+    def down


What about renaming this to drop?

drujensen · 2018-04-01T03:33:14Z

src/granite_orm/migrator.cr

+    def down
+    end
+
+    def up


and this to create?

Initially, I imagined cooperating with micrate as below, so I choose these names.

-- +micrate Up User.migrator.up -- +micrate Down User.migrator.down

But, I found that micrate can handle only SQL string. Therefore, since there are no constraints now, I think that it is better to change the name as you say. 😺

robacarp

Neat idea, and a good contribution. I like the way it cleans up the tests, but I also think it could be useful for removing micrate as a dependency in the future.

robacarp · 2018-04-02T16:33:17Z

src/adapter/mysql.cr

+  module Schema
+    TYPES = {
+      "AUTO_Int32" => "INT NOT NULL AUTO_INCREMENT PRIMARY KEY",
+      "AUTO_Int64" => "BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY",


The capitalization here is unexpected. Why AUTO_Int32?

Well, I'd like to resolve three kinds of crystal objects here.

Auto number (value + metadata like Int32 or Int64 + this is autonumber)

All crystal classes with module name (value like Int32, Foo::Data)

special field variables (value like created_at , updated_at)

Here, since they can be represented by strings itself for the case of 2 and 3, we can use simple Hash(String, String) for the mapping dictionary.
Then, I faced a problem of how to embed the meta information of autonumber in the payload for the case 1.

Although there are various correct and complicated implementations, I prioritized the simplicity of implementation, and I embedded the meta information as "uppercase letters + _" in the string for the time being. 😸

robacarp

The only thing I'd like to see is some test coverage. I know that this code will run as part of the test suite at this time, but it seems like the kind of code that will end up getting used elsewhere as well quickly so I'd like to have coverage over it.

robacarp · 2018-04-02T16:39:29Z

src/adapter/mysql.cr

+  module Schema
+    TYPES = {
+      "AUTO_Int32" => "INT NOT NULL AUTO_INCREMENT PRIMARY KEY",
+      "AUTO_Int64" => "BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY",


Can you add some quick tests for these in the adapter tests? I'd like to have them covered against accidental changes.

Yes, I think it would be better to have a test. But I can not image a simple test. But, even now, Int32 and Int64 are used as primary keys in other specs, so I think the code has been covered. 😺

drujensen · 2018-04-02T16:50:57Z

@robacarp we will not be replacing micrate with this anytime soon. I used to have this exact logic in granite to automatically perform migrations, but it was too difficult to guess the intent of the developer. For example, you didn't know what data to copy to which field if they renamed a column.

robacarp · 2018-04-02T17:38:46Z

@drujensen Sorry, I wasn't meaning to suggest we'd provide facility for automatic migrations. Only that I can see the maintenance overhead for yet another shard being abandoned by its creator getting adopted by Amber, and absorbing the idea of a migrator into granite doesn't seem far fetched. Should that happen, the namespace Migrator is probably where it'd live.

drujensen · 2018-04-02T20:56:21Z

@robacarp gotcha. Don't get me wrong, I loved it when the migrations were built in. It was additive so it never broke anything, but it couldn't do certain things and those limitations would require something like micrate anyway. This PR is a good compromise and really cleans up the code. I am very happy to see this solution. It fits in between the gap that I lost when moving to micrate.

Blacksmoke16 · 2018-04-08T20:41:00Z

README.md

+
+User.migrator.drop_and_create
+# => "DROP TABLE IF EXISTS `users`;"
+# => "CREATE TABLE `users` (id BIGSERIAL PRIMARY KEY, name VARCHAR(255));"


That should probably be id BIGINT or id INT since BIGSERIAL is PostgreSQL specific.

drujensen · 2018-04-22T17:54:38Z

@maiha Something is breaking the Migrator. I updated to the latest master branch and looks like I broke it:

Migrator(Granite::Adapter::Pg) doesn't support '{type: String}' yet. (Exception)
  from spec/spec_models.cr:255:3 in 'create'

Can you take a look when you get a chance?

maiha · 2018-04-26T03:45:51Z

I am sorry, my main business has been very busy since April, and I have not had time to contribute to OSS recently.
Unfortunately, this situation seems to last for half a year, so I will close this PR once. 😢

drujensen · 2018-04-27T13:33:58Z

@maiha Ok, i will fork this and fix it. Thanks for your contributions! Hope to see you back soon. 🥇

faustinoaq requested a review from a team March 31, 2018 22:59

faustinoaq added the pr:needs-review label Mar 31, 2018

drujensen approved these changes Apr 1, 2018

View reviewed changes

maiha force-pushed the migrator branch from 4d1c1e7 to 2640b18 Compare April 1, 2018 06:13

add Granite::ORM.migrator to production codes

a152eba

maiha force-pushed the migrator branch from 2640b18 to a152eba Compare April 1, 2018 06:15

robacarp reviewed Apr 2, 2018

View reviewed changes

robacarp requested changes Apr 2, 2018

View reviewed changes

Merge branch 'master' into migrator

86f65fa

Blacksmoke16 reviewed Apr 8, 2018

View reviewed changes

robacarp approved these changes Apr 9, 2018

View reviewed changes

Merge branch 'master' into migrator

aced2cf

eliasjpr mentioned this pull request Apr 19, 2018

Should Granite have a built in Migrator? #182

Closed

drujensen added 2 commits April 22, 2018 10:13

Merge branch 'master' into migrator

bdf8d85

Merge branch 'master' into migrator

413f51a

drujensen removed the pr:needs-review label Apr 22, 2018

maiha closed this Apr 26, 2018

drujensen reopened this Apr 27, 2018

faustinoaq added the pr:wip label Apr 27, 2018

drujensen closed this May 12, 2018

faustinoaq removed the pr:wip label May 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `Granite::ORM.migrator` to production codes #172

add `Granite::ORM.migrator` to production codes #172

maiha commented Mar 31, 2018 •

edited

Loading

drujensen left a comment

drujensen Apr 1, 2018

drujensen Apr 1, 2018

maiha Apr 1, 2018

robacarp left a comment

robacarp Apr 2, 2018

maiha Apr 3, 2018

robacarp left a comment

robacarp Apr 2, 2018

maiha Apr 3, 2018

drujensen commented Apr 2, 2018

robacarp commented Apr 2, 2018

drujensen commented Apr 2, 2018

Blacksmoke16 Apr 8, 2018

drujensen commented Apr 22, 2018

maiha commented Apr 26, 2018

drujensen commented Apr 27, 2018

add Granite::ORM.migrator to production codes #172

add Granite::ORM.migrator to production codes #172

Conversation

maiha commented Mar 31, 2018 • edited Loading

implementation

drujensen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robacarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robacarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drujensen commented Apr 2, 2018

robacarp commented Apr 2, 2018

drujensen commented Apr 2, 2018

Choose a reason for hiding this comment

drujensen commented Apr 22, 2018

maiha commented Apr 26, 2018

drujensen commented Apr 27, 2018

add `Granite::ORM.migrator` to production codes #172

add `Granite::ORM.migrator` to production codes #172

maiha commented Mar 31, 2018 •

edited

Loading