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.

Sign up for GitHub

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

Jump to bottom

WIP: SQLite3 support for migrations #221

Closed
wants to merge 1 commit into from
Closed

WIP: SQLite3 support for migrations #221

wants to merge 1 commit into from

Conversation

erikh
Copy link

@erikh erikh commented Dec 5, 2019

This is a basic start, a lot of cut and paste but it does pass some very basic tests now. I still have more work to do but I was hoping we could discuss some of it as it arrives, so here's the PR.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 5, 2019
@erikh erikh mentioned this pull request Dec 5, 2019
Closed
@erikh erikh force-pushed the sqlite3-migration-support branch 4 times, most recently from 5eb1f21 to fe7eecd Compare December 6, 2019 04:23
@erikh
Copy link
Author

erikh commented Dec 6, 2019

@a8m at the risk of spamming you, here are some highlights in the existing work you should take note of. I have the FK work to do still and a few tests, but otherwise I think everything is functioning.

Note that due to how sqlite3 lib handles parameters, I don't think using a ? or binding syntax will work in the pragmas. I think since we're working totally on trusted input that a sprintf is safe, but I would like to hear your input.

https://github.com/facebookincubator/ent/pull/221/files#diff-39e4b302014280cdc713d8c1a32acb2eR140 -- this tinyint(1) special case for mysql is perpetuated here, do we want that?

https://github.com/facebookincubator/ent/pull/221/files#diff-56a4aef6402a64fbe636f02a23d8fa57R22 better place for this test?

also the following refactors were made:

https://github.com/facebookincubator/ent/pull/221/files#diff-932a340b0ff6f22c5b5a59ce1e8a4972R434 - first function is taken from mysql driver and the second is taken from pg, these are just utility functions to perform standard operations I needed. I hope the moves are ok.

@erikh
Copy link
Author

erikh commented Dec 6, 2019

Additionally, it doesn't seem like constraints are uniquely named throughout sqlite3, leading to an issue i've pushed here:

you can see there is no globally unique identifier. what matches against the name provided to fkExist?

sqlite> create table foo (bar int primary key);
sqlite> create table quux (baz int references foo (bar));
sqlite> .schema
CREATE TABLE foo (bar int primary key);
CREATE TABLE quux (baz int references foo (bar));
sqlite> pragma foreign_key_list('quux');
0|0|foo|baz|bar|NO ACTION|NO ACTION|NONE
sqlite> .headers on
sqlite> pragma foreign_key_list('quux');
id|seq|table|from|to|on_update|on_delete|match
0|0|foo|baz|bar|NO ACTION|NO ACTION|NONE

I'm committing some code now that shows my problem; read the fkexist method.

Thanks again for your patience with me. :)

@erikh erikh force-pushed the sqlite3-migration-support branch 2 times, most recently from 7403e91 to 8ac4326 Compare December 6, 2019 07:38
@a8m
Copy link
Member

a8m commented Dec 6, 2019

Hey @erikh, thanks for working on this!
I'll review this on Sunday (weekend in Israel is Friday–Saturday).

@erikh
Copy link
Author

erikh commented Dec 6, 2019 via email

@a8m
Copy link
Member

a8m commented Dec 8, 2019

@a8m at the risk of spamming you, here are some highlights in the existing work you should take note of. I have the FK work to do still and a few tests, but otherwise I think everything is functioning.

You're not spamming at all. Thanks for working on this.
If you prefer to work with smaller PRs (like, only table scanning and them reading indexes, etc..) that's fine too.

Note that due to how sqlite3 lib handles parameters, I don't think using a ? or binding syntax will work in the pragmas. I think since we're working totally on trusted input that a sprintf is safe, but I would like to hear your input.

I think Sprintf is fine here.

https://github.com/facebookincubator/ent/pull/221/files#diff-39e4b302014280cdc713d8c1a32acb2eR140 -- this tinyint(1) special case for mysql is perpetuated here, do we want that?

I don't think so. I think it's OK to support only bool and boolean, because these are the common types in the ORMs outside (feel free to correct me if I have a wrong assumption).

https://github.com/facebookincubator/ent/pull/221/files#diff-56a4aef6402a64fbe636f02a23d8fa57R22 better place for this test?

entc/integration/migrate/migrate_test.go

you can see there is no globally unique identifier. what matches against the name provided to fkExist?

Let's change the method then to:

fkExist(ctx context.Context, tx dialect.Tx, table, fk string) (bool, error)

Let me know if you plan to break it to smaller PRs, or I'll just review it.
Thanks again for your contribution.

@erikh
Copy link
Author

erikh commented Dec 8, 2019 via email

@erikh
Copy link
Author

erikh commented Dec 9, 2019

I'm in process of porting the postgres table migration tests over to sqlite3 but your other change suggestions have been implemented.

@erikh
Copy link
Author

erikh commented Dec 9, 2019

ok the migration tests are up as well as some related fixes. I'll fix up any tests next, but what should I be doing with this patch after this? writing more tests?

@erikh
Copy link
Author

erikh commented Dec 9, 2019

how do I safely run the integrations? is there a doc for it?

@erikh
Copy link
Author

erikh commented Dec 9, 2019

nvm found it

@erikh
Copy link
Author

erikh commented Dec 10, 2019

https://www.sqlite.org/lang_altertable.html#otheralter is going to be necessary for the FK moves and other operations on constraints such as changing the on delete or update triggers.

Where would this code best live?

@a8m
Copy link
Member

a8m commented Dec 11, 2019

https://www.sqlite.org/lang_altertable.html#otheralter is going to be necessary for the FK moves and other operations on constraints such as changing the on delete or update triggers.

Maybe I miss something, but why do we need to create a temporary table if we can create FKs as mentioned in #200 (comment)?

@erikh
Copy link
Author

erikh commented Dec 11, 2019 via email

@a8m
Copy link
Member

a8m commented Dec 11, 2019

for example adding a foreign key to an existing column -- you have to perform this operation.

Currently, a FK is added with its column to the schema file, when a new edge is added to the ent.Schema. Meaning, we don't have situations that a FK is added to a column that already exists in the table.

We can add support for this (I don't really mind), but I think it adds much more complexity for a feature/functionality that we don't really need.

Let me know what do you think.

@erikh
Copy link
Author

erikh commented Dec 11, 2019 via email

@erikh
Copy link
Author

erikh commented Dec 14, 2019

Sorry, I haven't had time to get to this over the week. I'll try to polish this off next week.

@erikh erikh force-pushed the sqlite3-migration-support branch 2 times, most recently from 519175d to 7d06b42 Compare December 19, 2019 10:31
@erikh
Copy link
Author

erikh commented Dec 19, 2019

This should pass all tests now; I've squashed the patch and I think it's ready for at least an initial review. I'm sure there are still some things outstanding.

@erikh
Copy link
Author

erikh commented Dec 19, 2019

some notes:

  • we now pass *Table and *ForeignKey objects directly to the fkExist interface method; it's simpler this way. let me know if you object.
  • since there is no symbol in sqlite for FK columns, we simply match the parameters. I may be missing a few that are necessary for ent, please advise.

@erikh
Copy link
Author

erikh commented Dec 19, 2019

one last thing; I just don't know if this is going to behave in the following scenario:

  • you define a schema with two tables, each with a Node and no edges
  • migrate
  • define an edge between the two existing nodes

This is probably not a problem for very simple situations but could be in the future.

@erikh
Copy link
Author

erikh commented Dec 24, 2019

@a8m what should I do with this patch?

@a8m
Copy link
Member

a8m commented Dec 24, 2019

Hey @erikh, sorry for the delay. I missed the previous messages.

I'll review this later today (or tomorrow).
Thanks!

a8m
a8m reviewed Dec 24, 2019
View reviewed changes
Copy link
Member

@a8m a8m left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @erikh!
I've added a few comments.

Most of them are minor, but the major one should be handled (or we can discuss about it).

Update: "major" => first comment below.

dialect/sql/builder.go
Comment on lines +257 to +261
if t.Dialect() == dialect.SQLite {
t.Queries = append(t.Queries, &Wrapper{"ADD COLUMN %s", fk})
} else {
t.Queries = append(t.Queries, &Wrapper{"ADD CONSTRAINT %s", fk})
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this logic should reside in the schema migration package.

I'm thinking about 2 options (but you're welcome to suggest other alternatives):

  1. Add an addFK method to the sqlDialect interface that generates a different command for the sqlite3 dialect.
  2. Change fkExist logic in sqlite3 to always returns true (as it was before), and append the foreign-key-clause to the new column we create. For example: 
    ALTER table t1 ADD COLUMN c1 int REFERENCES t2(c2) ON DELETE SET NULL;

Let me know what do you think about it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, that makes sense. I think I get the general gist of what your'e suggesting:

func (s *SQLite) addFK(t *Table, *fk ForeignKey) string {
   // construct string to add foreign key
}

func (s *SQLite) fkExist(t *Table, *fk ForeignKey) bool {
   return true
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@a8m ok after some noodling on this (and re-reading what you said, multiple times, sorry about missing the message there!):

  • addFK leads to import cycles best I can see without creating a third package, but I'm still playing around with this approach and it might prove fruitful. I can't share the migrate package with the builder directly because of the relationship that already exists. I'm guessing I'm missing something here and will report back after more searching around.

  • setting things to true works fine, but then the alter cannot work at all unless I am still misunderstanding? I'm not sure is the best approach to adding edges to existing tables. I've implemented most of this and the clear problem with it is that it will never work on the second migration to the same table as the user probably would expect (with fks populated).

I think what might make sense here (but need to examine a little more) is altering the addColumn interface method to accept a foreignkey struct, or otherwise relate the foreign keys to the columns being currently built (which is true for FKs but not the columns that they are related to). This way I can deliberately say "I want to ensure my foreign key relationships are always added as a part of my column mutations to tables, no other way". And I think, but am not 100% certain, this way the work in fk.DSL() can either be simplified or removed for SQLite's case and we can just match/query the foreign keys directly in the Column portion of the migration in the tBuilder method and simplify the building process here.

WDYT?

Also sincerely sorry for the uber-high latency on this patch, I'll try to button things up this week.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what might make sense here (but need to examine a little more) is altering the addColumn interface method to accept a foreignkey struct, or otherwise relate the foreign keys to the columns being currently built (which is true for FKs but not the columns that they are related to).

I think that's fine (as I mentioned in #221 (comment)). If you want to go with this approach, I can help with that and add the code that links the columns with their foreign-keys.

WDYT?

dialect/sql/builder.go
Comment on lines +344 to +358
if fk.Dialect() == dialect.SQLite {
fk.Pad().Ident(fk.columns[0])
} else {
fk.WriteString("FOREIGN KEY")
fk.Nested(func(b *Builder) {
b.IdentComma(fk.columns...)
})
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

dialect/sql/schema/migrate.go

if new == nil {
return nil, errors.New("determined state could not be determined during change set generation")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this format: sql/schema: determined state could ....

Also, maybe add the table name as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Would you like wrappers for these package name identifiers usable for github.com/pkg/errors.Wrap? Could do this in a later PR.

dialect/sql/schema/mysql.go
return r == '(' || r == ')' || r == ' ' || r == ','
}); parts[0] {

switch parts := typeFields(c.typ); parts[0] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

dialect/sql/schema/sqlite.go
c.Type = field.TypeJSON
case "uuid":
c.Type = field.TypeUUID
case "enum":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this clause is needed.

dialect/sql/schema/sqlite_test.go
mock.ExpectQuery(escape("pragma foreign_key_list('users')")).
WillReturnRows(sqlmock.NewRows([]string{"id", "seq", "table", "from", "to", "on_update", "on_delete", "match"}))
mock.ExpectExec("ALTER TABLE `users` ADD COLUMN `spouse_id` REFERENCES `users`\\(`id`\\) ON DELETE CASCADE").
WillReturnResult(sqlmock.NewResult(0, 1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great testing work!

dialect/sql/schema/mysql.go Show resolved Hide resolved
dialect/sql/schema/migrate.go Show resolved Hide resolved
dialect/sql/schema/sqlite.go Show resolved Hide resolved
dialect/sql/schema/sqlite.go
}
}

return false, rows.Close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@erikh
Copy link
Author

erikh commented Dec 24, 2019 via email

@erikh
Copy link
Author

erikh commented Dec 28, 2019

Thanks for the review. I'll try to turn this around over the weekend. I assume I probably won't hear from you until after the new year. ;) Enjoy your time.

@erikh
Copy link
Author

erikh commented Jan 1, 2020

I haven't forgotten about this; just haven't had much time this week. Thanks and sorry.

@a8m
Copy link
Member

a8m commented Jan 6, 2020

I haven't forgotten about this; just haven't had much time this week. Thanks and sorry.

I can help you with that if you want. Just split it to multiple PRs, so we can merge all the ready parts.

SQLite3 support for migrations
5b3a31c 
Work in progress until this message is removed

Signed-off-by: Erik Hollensbe <github@hollensbe.org>
@erikh
Copy link
Author

erikh commented Jan 19, 2020

Between a variety of personal conflicts I can't really spend too much more time on this, and I feel bad just leaving it here. Happy to split it up over the next week if you want to take pieces of it, or you can cherry-pick what you would like to keep. I really do need this behavior though, so I'd encourage you to persist with the features this patch was intended to deliver.

Sorry man. I hate doing this to people but life is just getting too busy.

@erikh
Copy link
Author

erikh commented Jan 19, 2020

Let me know how you want it split up and I will do the work.

@Sloaix
Copy link

Sloaix commented Feb 11, 2020

How much time will it take to merge this pr?

This feature is important for me, i'm using ent instead of xorm.

@vtolstov
Copy link

now this pr needs to be fixed as it have merge conflicts, but yes, how we can help to get this merged?

@a8m
Copy link
Member

a8m commented Feb 11, 2020

Thanks for the great work @erikh, and sorry for my delayed response.

I'll check if I can split (and edit) it by myself and merge parts of this to ent. I want to make sure you get the credit for your work.

Thanks again and don't feel bad about it!

@erikh
Copy link
Author

erikh commented Feb 14, 2020 via email

a8m added a commit that referenced this pull request Apr 10, 2020
@a8m
dialect/sql/schema: initial work for incremental migration
2a3b13f 
This is a WIP PR and should be ignored this moment.
It's based on PR #221 created by Erik Hollensbe (He should
get his credit for his work before we land this).
a8m added a commit that referenced this pull request Apr 11, 2020
@a8m
dialect/sql/schema: initial work for incremental migration
1bf2416 
This is a WIP PR and should be ignored this moment.
It's based on PR #221 created by Erik Hollensbe (He should
get his credit for his work before we land this).
a8m added a commit that referenced this pull request Apr 12, 2020
@a8m
dialect/sql/schema: initial work for incremental migration
54bdaf8 
This is a WIP PR and should be ignored this moment.
It's based on PR #221 created by Erik Hollensbe (He should
get his credit for his work before we land this).
@a8m a8m mentioned this pull request Apr 12, 2020
Merged
a8m added a commit that referenced this pull request Apr 12, 2020
@a8m
dialect/sql/schema: initial work for incremental migration
77a5672 
This is a WIP PR and should be ignored this moment.
It's based on PR #221 created by Erik Hollensbe (He should
get his credit for his work before we land this).
a8m added a commit that referenced this pull request Apr 12, 2020
@a8m
dialect/sql/schema: initial work for incremental migration
210cd1d 
This is a WIP PR and should be ignored this moment.
It's based on PR #221 created by Erik Hollensbe (He should
get his credit for his work before we land this).
a8m added a commit that referenced this pull request Apr 12, 2020
@a8m
dialect/sql/schema: initial work for incremental migration
e87e1ce 
This is a WIP PR and should be ignored this moment.
It's based on PR #221 created by Erik Hollensbe (He should
get his credit for his work before we land this).
a8m added a commit that referenced this pull request Apr 12, 2020
@a8m
dialect/sql/schema: initial work for incremental migration (#428)
2208b24 
This is a WIP PR and should be ignored this moment.
It's based on PR #221 created by Erik Hollensbe (He should
get his credit for his work before we land this).
@a8m
Copy link
Member

a8m commented Apr 12, 2020

#428 was merged and it adds the first basic migration functionality for SQLite (adding resources).
Stuff like dropping/modifying columns or constraints are currently disabled in SQLite and will be added in the future.

I want to thank you @erikh for this amazing work and for being active and involved in this project!

@a8m a8m closed this Apr 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a8m a8m a8m left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@erikh @a8m @Sloaix @vtolstov @facebook-github-bot