Ensure that semicolon is not appended to statements that already end with a semicolon

[guilhermedalleprane]

This solves #4045:

toQuery() & toSql() are returning duplicate semicolons for queries that pass through the method wrapSqlWithCatch()

Example:

var knex = require('knex')({
    client: 'oracledb',
    connection: {
        host: '127.0.0.1',
        user: 'your_database_user',
        password: 'your_database_password',
        database: 'myapp_test'
    }
});

var builder = knex.schema.dropTableIfExists('book');

console.log(builder.toQuery())

outputs:

begin execute immediate 'drop table "book"'; exception when others then if sqlcode != -942 then raise; end if; end;;
begin execute immediate 'drop sequence "book_seq"'; exception when others then if sqlcode != -2289 then raise; end if; end;

[kibertoad]

Thanks! Could you please add test for this? Also CI seems to be failing.

[guilhermedalleprane]

Of course, in reality this does not seem to be the solution, I will take a closer look.

[guilhermedalleprane]

Well, I believe it will now work as expected. But I don't know where to put the tests.

This specific case would be testing whether converting a set of queries, some ending with semicolons, would work as expected, with all of them ending with just one semicolon, except the last, where nothing would be appended.

The query builder already has tests to verify that most queries are converted correctly, the schema builder does not and that would require refactoring a lot for such a small adjustment I think.

It is not something strongly related to a dialect, nor to the query builder, nor to the schema builder and nor a specific statement.

What do you think?

[kibertoad]

@guicovred Is it possible to write an integration test that would have been failing prior to this change, but no longer does after it?

[guilhermedalleprane]

Ok, this is possible. I was thinking about something more generic before. I added the test. Thanks!

[guilhermedalleprane]

It seems that these new failed CI tests was introduced by nodejs v14.13 (20 hours ago).

esm-interop.spec.js#L164 and esm-interop.spec.js#L179

Apparently the exception is from here: lexer.js#L892

[kibertoad]

#4054

[kibertoad]

@guicovred Please pull from master, that should fix it.

[guilhermedalleprane]

@guicovred Please pull from master, that should fix it.

Done. Thanks!

[guilhermedalleprane]

@kibertoad Is there still anything I can do to get it merged?

[kibertoad]

Thanks a lot!

[guilhermedalleprane]

I thank you!