Fix handling of multiline SQL in SQLite3 schema

[6uliver]

Bug:
Multiline SQL parsed incorrectly in SQLite3 schema when using renameColumn or dropColumn.

Knex version: 0.19.2
Sqlite3 version: 4.1.0
Node.js version: 10.15.3

Use case:
I have a legacy SQLite3 database in production which has been created manually with DB Browser for SQLite (https://sqlitebrowser.org/) so I decided to use Knex migration capabilities from now on.
The problem is that the create statement in sqlite_master table is a multiline SQL (which has been created by the DB Browser and is absolutely valid) but the SQL parser in DDL implementation does not expect for it.

Repro:

1. Create an SQLite3 database and a table having some columns with DB Browser so the create statement contains newlines and tabs:

CREATE TABLE "TestTable" (
	"id"	INTEGER,
	"description"	TEXT,
	PRIMARY KEY("id")
);

test.db.zip

2. Write a migration script which tries to rename a column or drop it.

20190824103207_add_and_rename_column.js.zip

ER:
Columns are renamed/dropped

AR:
Migration script failes with the following error:

TypeError: Cannot read property '1' of null
    at SQLite3_DDL._doReplace (***/node_modules/knex/lib/dialects/sqlite3/schema/ddl.js:134:30)
...

Should you have any question or suggestion don't hesitate to ask, I will make the necessary changes quickly, so that you can merge it as soon as possible.

[kibertoad]

How do you use it? By calling knex.raw?

[6uliver]

How do you use it? By calling knex.raw?

No, I'm not calling knex.raw, just using a simple migration script as you can see in the attached file:

exports.up = function(knex) {
  return knex.schema.table('TestTable', function (table) {
    table.dropColumn('description');
    table.renameColumn('id', 'testId');
  })
};

[kibertoad]

I'm really confused then. If your script is valid javascript, why any changes are needed? I don't see any multiline SQL logic in example you gave, and without full integration test I don't see the real use-case.

[6uliver]

I'm really confused then. If your script is valid javascript, why any changes are needed? I don't see any multiline SQL logic in example you gave, and without full integration test I don't see the real use-case.

The multiline SQL is coming from the database file since SQLite3 is storing schema as a create statement in sqlite_master table. The multiline SQL create statement added by the DB Browser tool because my database is a legacy one and have not been created with a migration script from the beginning but by hand. DDL class is finding table schema with the getTableSql method which is returning the multiline SQL.

I kindly ask you to read again my original comment carefully, try out the migration script and the DB attached, run and inspect the failing tests separately commited in 82a46a0

[kibertoad]

@6uliver I will. However, given that this is not a typical use-case and current unit test covers non-self-explanatory case, full integration test is definitely needed in this case that would show that solution is working end to end.

[6uliver]

@kibertoad ok, thanks for your quick response. I will add some end to end integration tests soon.

[6uliver]

@kibertoad I've added an end to end test.

At the start of the test run for SQLite3 we copy sample.sqlite3 to test.sqlite3 and it contains a test table which has been created with DB Browser for SQLite (this causes the bug because it contains the multiline expression). The new test case has two migration script, one for renaming a column and the other for dropping the column.

I've patched the double quote backward compatibility regex from /CREATE\sTABLE\s".*"\s\("/ to /CREATE\sTABLE\s".*"\s\(\s*"/ because after the parentheses the identifier starts in a newline so I added \s*.

[kibertoad]

Sample DB usage comments still need to be addressed

[6uliver]

I fixed the code related to your comments.

I had to change the identifier replacing logic too, because there were already some logic for backward compatibility related to double quotes and backsticks. I implemented this as a way to replace identifier style to the one which was given as the "to" parameter. The "from" parameter is stripped to get the identifier name without quotes and backsticks and replace it to "to" parameter as it received.

Examples:
in SQL, from, to, result
"id", "id", "newId" -> "newId"
"id", `id`, "newId" -> "newId"
"id", "id", `newId` -> `newId`
"id", `id`, `newId` -> `newId`

[6uliver]

Travis failed on Node.js 8 because two Oracle DB migration tests failed with a timeout error: TimeoutError: Knex: Timeout acquiring a connection. The pool is probably full.

I don't think it's related with this PR, can you restart the build?

[kibertoad]

what's the difference between the two files?

[6uliver]

I had to make a copy before the test run and delete it after because knex migration tables changes, so the DB file becomes changed in Git for every test run.

[kibertoad]

@kirrg001 Could you please check if changes in SQLite identifier name escaping is not going to break things for you?

[kibertoad]

Related: #2763

[6uliver]

@kibertoad do you have any update on this? @kirrg001 seems to be inactive for a while.

[kibertoad]

@6uliver I'm seeing this error on master now:

  1) Integration Tests
       "after all" hook in "Integration Tests":
     Error: EBUSY: resource busy or locked, unlink 'C:\sources\knex\test/test.sqlite3'

Do you think it is related to your changes?

[kibertoad]

Released in 0.19.5