[SQLite] dropForeign is silently ignored

[rijkvanzanten]

Environment

Knex version: 0.95.2
Database + version: SQLite (any)
OS: macOS

Bug

1. Explain what kind of behaviour you are getting and how you think it should do

When using .dropForeign() in SQLite, it's silently ignored. When inspecting the queries made, it correctly creates a new temp table for the alterTable call, but fails to strip out the FOREIGN KEY line in the create table sql.

2. Error message

n/a

3. Reduced test code

require('sqlite3');
const Knex = require('knex');

const sqlite = Knex({
  client: 'sqlite',
  connection: ':memory:',
});

(async function run() {
  await sqlite.schema.createTable('test', (t) => {
    t.increments('id').primary();
  });
  
  await sqlite.schema.createTable('related', (t) => {
    t.increments('id').primary();
    t.integer('fk').references('id').inTable('test');
  });
  
  console.log(await getCreateTableSQL('related'));
  // Contains the `FOREIGN KEY` as expected
  
  await sqlite.schema.alterTable('related', (t) => {
    t.dropForeign(['fk']);
  });
  
  console.log(await getCreateTableSQL('related'));
  // Still contains the `FOREIGN KEY`. Should've been removed.
})();

async function getCreateTableSQL(table) {
    const result = await sqlite
        .select('sql')
        .from('sqlite_master')
        .where({ name: table })
        .first();
    
    return result && result.sql;
}

https://runkit.com/rijkvanzanten/knex-sqlite-drop-foreign-example

(@nickrum, before I start chopping away in your amazing work from the other day, do you have any instructions on how to (re-)implement dropForeign using the new tokenized approach you set up?)

[rijkvanzanten]

I'm also a little confused why the tests haven't caught this 🤔

There is an integration test for foreign keys that does seem to test for this:

knex/test/integration/schema/foreign-keys.js

Lines 80 to 91 in 2bce36e

	await knex.schema.alterTable('foreign_keys_table_one', (table) => {
	table.dropForeign(['fkey_two']);
	table.dropForeign([], 'fk_fkey_threeee');
	table.dropForeign(['fkey_four_part1', 'fkey_four_part2']);
	});
	await knex('foreign_keys_table_one').insert({
	fkey_two: 999,
	fkey_three: 999,
	fkey_four_part1: 'e',
	fkey_four_part2: 'f',
	});

but it still passes in SQLite..

I did see that foreign_keys is enabled in the pool options in the test-runner:

knex/test/knexfile.js

Lines 24 to 32 in 2bce36e

	const poolSqlite = {
	min: 0,
	max: 1,
	acquireTimeoutMillis: 1000,
	afterCreate: function (connection, callback) {
	assert.ok(typeof connection.__knexUid !== 'undefined');
	connection.run('PRAGMA foreign_keys = ON', callback);
	},
	};

The only thing I can think of is that this option somehow isn't actually used? That would explain SQLite silently ignoring foreign key constraints (@kibertoad)

[nickrum]

I think the issue with the integration test is that it doesn't actually check if the foreign key constraints have been deleted.

[rijkvanzanten]

I'd expected that insertion on 86 to throw an invalid key constraint error though, seeing it didn't delete the foreign key constraint 🤔

[nickrum]

Yeah right, but the test would pass anyway without an expect assertion, wouldn't it? 🤔

[rijkvanzanten]

Hmm I was under the impression that an unexpected error (in this case the throw from insert) would mark the test as failed. You might be right tho!

[nickrum]

I just checked it and it seems like tests indeed pass even if there was an exception. Mocha even says No unhandled exceptions. Maybe this is due to the test being asynchronous.

But in this case the insert doesn't throw an exception because apparently if you drop a composite foreign key by specifying the individual columns, all foreign keys are dropped.

[rijkvanzanten]

@kibertoad I hate to be that guy, but when is the next patch release scheduled? 😁
This fix is a pretty massive deal for us, as our SQLite migrations are totally bust 👀

[kibertoad]

@rijkvanzanten Apologies for the delay, another swamp surge came in and consumed me entirely. Appreciate the ping, will do the release tonight.

[rijkvanzanten]

No worries! I totally understand the business and life getting in the way 😄

[kibertoad]

Released in 0.95.3.