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

Add Oracle support #419

Merged
merged 4 commits into from Aug 14, 2014

Conversation

Projects
None yet
5 participants
@vschoettke
Collaborator

vschoettke commented Aug 11, 2014

Added Oracle support. Tested with Oracle 11g. It requires the oracle npm module, but I refrained to add it to the package.json to avoid problems for other developers.

There is also a new enviroment variable "KNEX_TEST_INTEGRATION_DIALECTS" to limit the integration tests to specified dialects. e.g.:
export KNEX_TEST_INTEGRATION_DIALECTS="oracle sqlite3"

I had to work around the limitation of missing auto_increment by automatically generating a per table sequence and trigger to add the auto incremented value for insert statements if somebody used increment() in their table. Oracle deletes triggers implicitly but no sequences, so dropTable() and dropTableIfExists() also delete the sequences if they exist.

A word of caution. Node-oracle module together with the Oracle 11g I tested does not seem to like how the cleanup of the default pool is done. It may lead to false test errors sometimes (e.g.ORA-01007: variable not in select list). Implementing my own pool and waiting for some time after the db connection is closed fixed the problem for me, but it is not part of this pull request.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Aug 12, 2014

This is awesome, thanks @vschoettke!

Any ideas if the pool will cause issues for people outside of testing if they don't supply their own?

@vschoettke

This comment has been minimized.

Collaborator

vschoettke commented Aug 12, 2014

I had no problems with normal usage and the default pool, so maybe this problem is only test related. Do you make sure that all connections are always closed/pool is drained when testing? On the node-oracle page it's especially mentioned that not closing all connections may lead to false errors.

tgriesser added a commit that referenced this pull request Aug 14, 2014

@tgriesser tgriesser merged commit b83a0af into tgriesser:master Aug 14, 2014

1 check passed

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

This comment has been minimized.

Owner

tgriesser commented Aug 14, 2014

So I don't have a way of testing it yet, but I'll merge it and take your word that it works so we can at least have it out there as "generally working / experimental"

@vschoettke vschoettke deleted the vschoettke:oracle-support branch Aug 14, 2014

@vschoettke

This comment has been minimized.

Collaborator

vschoettke commented Aug 14, 2014

Thanks for merging. I test with a Ubuntu VM on a Mac and Linux/Windows with dedicated server. A Travis CI integration would be nice though.

@maxl0rd

This comment has been minimized.

maxl0rd commented Aug 14, 2014

This is awesome, @vschoettke. +1 +1

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Aug 14, 2014

Oracle support for Travis was discussed and rejected (travis-ci/travis-ci#689) because of:

  1. Licensing issues
  2. Requires manual install

Would anyone using Oracle be willing to set up a Digital Ocean box (or any other server for that matter)? Would require some extra test code vs a fresh local DB in CI and run slower, but it'd be nice to have parity.

@tgriesser tgriesser referenced this pull request Sep 2, 2014

Closed

Changelog for 0.7 #460

@llambda

This comment has been minimized.

Contributor

llambda commented Nov 19, 2014

Nice work! Is it possible to have support for streaming queries from Oracle?

@vschoettke

This comment has been minimized.

Collaborator

vschoettke commented Nov 19, 2014

@llambda Thanks. I don't see a direct support for Query Stream in node-oracle.
Using a stream for query results (without loading all data at once) is already implemented.

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