Skip to content
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 handling of multiline SQL in SQLite3 schema #3411

Merged
merged 7 commits into from Oct 6, 2019

Conversation

@6uliver
Copy link
Contributor

6uliver commented Aug 24, 2019

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:

Screenshot 2019-08-24 at 10 29 21

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

test.db.zip

  1. 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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 24, 2019

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

@6uliver

This comment has been minimized.

Copy link
Contributor Author

6uliver commented Aug 24, 2019

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 24, 2019

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

This comment has been minimized.

Copy link
Contributor Author

6uliver commented Aug 24, 2019

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 24, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

6uliver commented Aug 24, 2019

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

test/integration/index.js Outdated Show resolved Hide resolved
test/integration/index.js Outdated Show resolved Hide resolved
@6uliver

This comment has been minimized.

Copy link
Contributor Author

6uliver commented Aug 24, 2019

@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*.

test/integration/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

kibertoad left a comment

Sample DB usage comments still need to be addressed

@6uliver

This comment has been minimized.

Copy link
Contributor Author

6uliver commented Aug 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

6uliver commented Aug 26, 2019

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?

before(function() {
if (config.sqlite3 && config.sqlite3.connection.filename !== ':memory:') {
fs.copyFileSync(
__dirname + '/../multilineCreateMasterSample.sqlite3',

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 26, 2019

Collaborator

what's the difference between the two files?

This comment has been minimized.

Copy link
@6uliver

6uliver Aug 26, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 26, 2019

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 26, 2019

Related: #2763

@6uliver

This comment has been minimized.

Copy link
Contributor Author

6uliver commented Oct 6, 2019

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

@kibertoad kibertoad merged commit c1d2027 into knex:master Oct 6, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 87.928%
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 6, 2019

@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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 6, 2019

Released in 0.19.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.