Quick-fix for truncate() on sqlite3 dialect

[lehni]

As discussed in #2312 (comment), here the first commit as a quick fix restoring previous functionality. A full fix for the other observed issues will follow later.

[elhigu]

Also testcase for erroneus operation is missing.

[lehni]

I am not sure, no. I don't understand Knex enough, and I don't have enough time to get into it at the moment. I also couldn't get the tests to run on my machine, so unfortunately I wasn't able to create a test for it. As it says, this is a quick fix for something that was broken between 0.13 and 0.14 (and there wasn't a test for it then). This is the best I can do currently. It works for me, and it can't be more wrong than what's currently out. So I'd say it's an improvement.

[lehni]

Regarding your question about the quotes, I guess single quotes would be better? Both seem to work. I'll adjust it.

[lehni]

Regarding tests: If you can explain how to get them to run locally then I'll be happy to try and whip one up.

[elhigu]

Single quotes are better because it doesn't have double meaning, where string may be interpreted as identifier or literal. Sqlite's " quotes work really strange way.

I always run tests locally with npm run build; npm test command, but it requires that one has mysql and postgres running with DB called knex_test and to mysql one should be able log in with user root without any password... but if you like to run just sqlite tests you can pass DB list in env var like npm run build; DB="sqlite3" npm test.

Contributing document has all that info too:

https://github.com/tgriesser/knex/blob/master/CONTRIBUTING.md

[lehni]

When I tried to get the tests to run, I saw errors about docker, and couldn't find any information about that. Can I ignore that part?

[elhigu]

yep you should not worry about those tests just ignore them. If docker host is not running, then those tests should not be ran either (I always shut down my docker host when im working on knex).

[lehni]

I see. I misunderstood this error and though that was the reason why the tests weren't running. Getting into docker to fix a simple issue seemed rather daunting.

I just implemented a unit test for this now by adding to the existing test and checking that newly inserted data after truncate() restarts with id 1. I haven't tested this for anything else than sqlite. We'll see how this performs on Travis CI in a moment, I guess. The changed commit is up: 0ed3e65

[lehni]

BTW, it looks like tests are only running on Node up to v7. Shouldn't 8 and 9 be added to .travis.yml?

[lehni]

Looks like the empty insert isn't working on oracle:

 insert into "test_table_two" ("undefined") values (default) - ORA-00904: "undefined": invalid identifier

I'll try again with one value.

(but that may be another bug in itself on oracle. All other DBs were fine with that statement)

[elhigu]

BTW, it looks like tests are only running on Node up to v7. Shouldn't 8 and 9 be added to .travis.yml?

Last time I tried there was some problems with node 8... If it they work now indeed they should be added.

[lehni]

Ok, now the tests all run, but again on Oracle, the ids don't restart at 1 after truncate... I get 11 instead. Does that mean truncate() is broken there too, or is that just how it is? I don't know Oracle. Not sure what to do about this now?

[elhigu]

Sounds like oracle truncate implementation doesn't work correctly. There was also some other strange errors. my new connection pool tests might be random failing :| Gotte checkout oracledb code if its truncate seems reasonable, but now I need to go to sleep :) laters and thanks for putting time to this!

[lehni]

Oracle id reset sounds super complicated... I am excluding Oracle from the new test for now, as I have no idea how to implement it there, and no interest in fixing Oracle support:

https://stackoverflow.com/questions/51470/how-do-i-reset-a-sequence-in-oracle

[elhigu]

@lehni yeah fixing oracle is clearly out of scope of this PR. @atiertant do you know if oracle's truncate also truncates the sequence?

[elhigu]

Thank you @lehni !

[lehni]

Cool, thanks for merging!

[lehni]

Would this merit the release of 0.14.2?

[elhigu]

I'll do the release when I get spare moment.

[lehni]

Thanks! That way I can let go of my internal workarounds : )

[elhigu]

done

[lehni]

thanks!

[atiertant]

@elhigu need a test

[elhigu]

@atiertant You mean test like this? https://github.com/tgriesser/knex/pull/2348/files#diff-bfeb3d958a56cecbae4f5f8983fdf195R111

[elhigu]

oracle is disabled there, because it didn't seem to reset id sequence

[atiertant]

@elhigu ok for the test, i'll try to have a look a it later but i don't have time this days...