Fixed primary keys being created separately on MySQL

[daniellockyer]

refs #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

---

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). 🙂

[kibertoad]

Alternatively, bookshelf can be updated to use 1.x :)

[daniellockyer]

@kibertoad 😁😁

Still, we'd need something like this in knex 1.x to fix our issue 🙏🏻

[daniellockyer]

Bleugh - fixing

[kibertoad]

Can you also add an integration test for this that would actually execute the SQL?

[daniellockyer]

@kibertoad Sure thing 👍🏻

I'm currently stuck fixing the integration test because increments should add an auto-incrementing primary column, but doesn't because other primary keys are present, and my code here can't detect the increments 🤔

[kibertoad]

@daniellockyer Do you not have enough builder data at your step to do this check?

[daniellockyer]

@kibertoad I just found what I need 😁 Sorry - new to the knex code 🙂

[kibertoad]

no worries! knex code is fairly tangled :D

[daniellockyer]

@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 🙂

[daniellockyer]

GitHub sure likes to re-request approval to run CI 😁

[kibertoad]

fair point, integration test probably too expensive to setup, scratch that

[kibertoad]

Thank you!

[kibertoad]

Will try to publish new version tonight

[daniellockyer]

Thank you for merging! ❤️

I'll get a 0.21 PR opened tomorrow - should be quite simple now 🙂