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

Quick-fix for truncate() on sqlite3 dialect #2348

Merged
merged 1 commit into from Nov 24, 2017

Conversation

Projects
None yet
3 participants
@lehni
Contributor

lehni commented Nov 23, 2017

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

This comment has been minimized.

Collaborator

elhigu commented Nov 23, 2017

Also testcase for erroneus operation is missing.

@lehni

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

@lehni

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

This comment has been minimized.

Collaborator

elhigu commented Nov 23, 2017

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

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

This comment has been minimized.

Collaborator

elhigu commented Nov 23, 2017

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

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

@lehni

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

This comment has been minimized.

Collaborator

elhigu commented Nov 23, 2017

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

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

This comment has been minimized.

Collaborator

elhigu commented Nov 23, 2017

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

This comment has been minimized.

Contributor

lehni commented Nov 23, 2017

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

Fix truncate() on sqlite3 dialect
Relates to #2312: ids were not correctly reset anymore
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 24, 2017

@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 elhigu merged commit b5ba51a into tgriesser:master Nov 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 24, 2017

Thank you @lehni !

@lehni

This comment has been minimized.

Contributor

lehni commented Nov 24, 2017

Cool, thanks for merging!

@lehni

This comment has been minimized.

Contributor

lehni commented Nov 24, 2017

Would this merit the release of 0.14.2?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 24, 2017

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

@lehni

This comment has been minimized.

Contributor

lehni commented Nov 24, 2017

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 24, 2017

done

@lehni

This comment has been minimized.

Contributor

lehni commented Nov 25, 2017

thanks!

@atiertant

This comment has been minimized.

Contributor

atiertant commented Dec 8, 2017

@elhigu need a test

@elhigu

This comment has been minimized.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 8, 2017

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

@atiertant

This comment has been minimized.

Contributor

atiertant commented Dec 8, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment