-
Notifications
You must be signed in to change notification settings - Fork 60
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: migration generator indexes #625
fix: migration generator indexes #625
Conversation
…t, updated_at but separate indexes.
Will probably need to modify the migrations of the example applications too. But I'll do that tomorrow. |
Current coverage is 91.92% (diff: 100%)@@ master #625 diff @@
==========================================
Files 179 179
Lines 1981 1981
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 1821 1821
Misses 160 160
Partials 0 0
|
Ugh, this is still wrong. Only way to do it is to set the indexes separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I've been thinking about this and I think we're better off adding timestamp indexes individually.
table.timestamps();
table.index('created_at');
table.index('updated_at');
Also, we can remove the additional index on id
as it should already be covered as the primary key.
@@ -58,11 +58,11 @@ export default (name: string, attrs: Array<string> | string): string => { | |||
${body} | |||
table.timestamps(); | |||
|
|||
table.index([ | |||
table.index( | |||
'id', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have to create an additional index for id
as it will be the primary key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. That is covered. I noticed the need for separate indexes too in the comment before this. Knex has no API to add multiple indexes at once. I'll try to work on this a bit later tonight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Sounds good.
Don't generate index for id (covered by PRIMARY already)
👏 This looks good @nickschot. One last thing though. Can you update the migrations in the examples as well as the test-app? |
A single index was created over
id
,created_at
andupdated_at
in the default generator for migrations. This also gave problems with mysql (index too long).