-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fixed primary keys being created separately on MySQL #5017
Conversation
refs knex#4141 - for the MySQL dialect within `knex`, creating a table with a primary key will produce 2 queries - one to create the table and another to alter the table with a primary key on the column specified - this is causing problems when trying to create tables on MySQL instances that have `sql_require_primary_key` enabled (the default on some managed DB providers) - this commit adds a `primaryKeys` function to the table compiler that is called upon table creation, and inserts the primary keys into the `create` statement - it also prevents the original `alter table ...` being called if we're creating a table - the tests have been updated to reflect the fact we no longer have a separate query upon creation, and I've added a test to check the inline primary key is added - the approach was mostly ripped from the same section in the SQLite driver - https://github.com/knex/knex/blob/47b96344c21beb3f8a2beb7aee6c99798a1ad318/lib/dialects/sqlite3/schema/sqlite-tablecompiler.js#L257-L274
Alternatively, bookshelf can be updated to use 1.x :) |
@kibertoad 😁😁 Still, we'd need something like this in knex 1.x to fix our issue 🙏🏻 |
Bleugh - fixing |
Can you also add an integration test for this that would actually execute the SQL? |
@kibertoad Sure thing 👍🏻 I'm currently stuck fixing the integration test because |
@daniellockyer Do you not have enough builder data at your step to do this check? |
@kibertoad I just found what I need 😁 Sorry - new to the knex code 🙂 |
no worries! knex code is fairly tangled :D |
@kibertoad That last commit should fix all the tests (it does locally), and I've added another unit test and replaced the lodash filter import. How would you expect the integration test to work? I thought I could try something like it.only('creates a primary key inline with the table creation', async () => {
await knex.schema.dropTableIfExists('table_primary_key');
await knex.raw(`set global sql_require_primary_key=ON;`);
await knex.schema.createTable('table_primary_key', (table) => {
table.string('id', 24).primary();
});
}); but I get permission errors from MySQL when running that and I'm not sure how else to test this. My patch changes the table creation SQL, which the unit tests cover, and the integration tests still pass for primary key creation so hopefully that covers everything 🙂 |
GitHub sure likes to re-request approval to run CI 😁 |
fair point, integration test probably too expensive to setup, scratch that |
Thank you! |
Will try to publish new version tonight |
Thank you for merging! ❤️ I'll get a 0.21 PR opened tomorrow - should be quite simple now 🙂 |
refs #4141
knex
, creating a table with a primarykey will produce 2 queries - one to create the table and another to
alter the table with a primary key on the column specified
instances that have
sql_require_primary_key
enabled (the default onsome managed DB providers)
primaryKeys
function to the table compiler thatis called upon table creation, and inserts the primary keys into the
create
statementalter table ...
being called if we'recreating a table
separate query upon creation, and I've added a test to check the
inline primary key is added
driver -
https://github.com/knex/knex/blob/47b96344c21beb3f8a2beb7aee6c99798a1ad318/lib/dialects/sqlite3/schema/sqlite-tablecompiler.js#L257-L274
Note: I originally looked into this to fix the problem for Ghost, which is pinned to knex 0.21 because we use Bookshelf. If we manage to get this merged, I'd request the patch also be backported to the 0.21 branch (happy to do so). 🙂