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

add node-oracledb support #990

Merged
merged 61 commits into from Jun 20, 2016

Conversation

Projects
None yet
@atiertant
Copy link
Contributor

atiertant commented Sep 22, 2015

this look like working,could someone test?
thanks @khalilTN

Alexandre Tiertant
@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Sep 22, 2015

this look like working

These files must be created in the /src directory and compiled to generate /lib.

could someone test?

I don't know much about oracle or oracledb, so hopefully someone more knowledgable can weigh in.

Alexandre Tiertant added some commits Sep 22, 2015

Alexandre Tiertant
Alexandre Tiertant
Alexandre Tiertant
correct pool release
correct date error

Alexandre Tiertant added some commits Nov 19, 2015

@jgoux

This comment has been minimized.

Copy link

jgoux commented Nov 26, 2015

Hello,
any chance this getting merged ?

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 26, 2015

There are basically no other tests, but only schema building for this. Maybe you could still add test oracledb client to test/unit/query/builder.js and make sure that queries are generated correctly, especially in those cases where oracledb driver works differently from base implementation.

Alexandre Tiertant added some commits Nov 26, 2015

@atiertant

This comment has been minimized.

Copy link
Contributor Author

atiertant commented Nov 26, 2015

@elhigu done ! i'm writting and sql adapter for waterline using this code for oracle,all test passed except blob because oracledb driver has a curious way to insert blob... they plan to simplify it and i'll update my code then.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 26, 2015

I think this looks fine (from a viewpoint not knowing anything about those dialects).

Only thing that is bothering me is that it looks mostly copy-paste of the old oracle client. There might be possibility to share codebase of the clients. e.g. websql is sqlite3 derivate so that could be used as an example.

However this is not breaking anything and I suppose it works... so it would be ok for me to merge this as is and do the refactoring later on :) @rhys-vdw what do you think?

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Nov 28, 2015

Hey @atiertant, thanks for the PR. It's challenging to review this PR because the changes are hidden in the duplicated code. Using inheritance here makes more sense than copy paste - not just for maintainability, but also for readability. The diff should show just the changes that pertain to OracleDB.

Ideally we'd do something like creating an AbstractOracleClient (supporting all the Oracle stuff) and then have each Oracle dialect inherit from this. Each inheriting Oracle dialect module should be quite thin and just override the sections pertaining to the API of its oracle module.

I understand the pressing need for this support - but I don't want to merge a PR containing wholesale duplication of files. There has to be clean up here before the merge. Do you think this will be possible, @atiertant?

@atiertant

This comment has been minimized.

Copy link
Contributor Author

atiertant commented Nov 29, 2015

@rhys-vdw hi, there is only one oracle database,and there were many drivers before oracle start writting their own driver for their database.
why would you like to maintain a dialect for an old and no more maintained driver (This library is not maintained. Oracle has made there own driver. see https://github.com/joeferner/node-oracle) ? i could move this changes directly in oracle dialect if you want... what do you think of it?

@vschoettke

This comment has been minimized.

Copy link
Collaborator

vschoettke commented Nov 29, 2015

@atiertant strong-oracle is except for the initialization identical to the old oracle driver.

@atiertant

This comment has been minimized.

Copy link
Contributor Author

atiertant commented Nov 30, 2015

so this should be correct ...

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Dec 2, 2015

Hey @atiertant, thanks for your patience.

why would you like to maintain a dialect for an old and no more maintained driver
I wasn't aware that the project was unmaintained until now - cheers. It's a good point.

i could move this changes directly in oracle dialect if you want... what do you think of it?

I would support using oracle as the client name for oracledb and renaming the unsupported driver to node-oracle - that way it seems more 'default'. But why remove the support? We have the module and there's no harm in keeping it (for now at least).

so this should be correct ...

Take a look. It inherits from the Oracle driver.

I can't see any benefit in introducing that much duplication into the database. It's a liability. It's not fair on future contributors to have to know to make fixes in two places - and it's more work for collaborators. (I'm not really sure what you've changed because it's just a huge copy+paste - but on my cursory reading it looked almost identical.)

I appreciate the work, but I can't merge it until it's DRY.

@atiertant

This comment has been minimized.

Copy link
Contributor Author

atiertant commented Dec 2, 2015

@rhys-vdw thanks for your review,

it's true that my first commit duplicated all dialect' code and that was not a good idea...
i removed all unnecessary duplicated code to make futur fixes at only one places.
of course the code staying is a copy paste of the original oracle with some little changes but
keep in mind all current oracle drivers are based on joeferner works and i woudn't like to reinvent the wheel but oracle made a lot of changes and api is now different.

this is the list of changes:

  • Client_Oracledb.prototype.prepBindings (binding methods are different)
  • Client_Oracledb.prototype._driver (driver is not the same)

the way to deal with connections is a bit different:

  • Client_Oracledb.prototype.acquireRawConnection
  • Client_Oracledb.prototype.destroyRawConnection

the results are returned into a different format:

  • Client_Oracledb.prototype._query
  • oracledb driver correct a bug in timestamp so we must use 'timestamp with local time zone' that affect time,datetime and timestamp of the ColumnCompiler.

made some change in oracle dialect to remove dupplicate code:

remove the parameter function (https://github.com/tgriesser/knex/blob/master/src/dialects/oracle/formatter.js#L18) of the formatter that did the same as prepBindings (https://github.com/tgriesser/knex/blob/master/src/dialects/oracle/index.js#L58)

add obj.rowsAffected to result of _query to share processResponse

now all this changes concert only the way the dialect deals with driver.

@epoberezkin

This comment has been minimized.

Copy link

epoberezkin commented Dec 7, 2015

👍

@atiertant

This comment has been minimized.

Copy link
Contributor Author

atiertant commented Dec 15, 2015

@rhys-vdw does this arguments convince you or should i emulate old driver functions? please let me know the way you want to do it...

Alexandre Tiertant and others added some commits Jun 13, 2016

@atiertant

This comment has been minimized.

Copy link
Contributor Author

atiertant commented Jun 13, 2016

@rhys-vdw ready to merge !

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Jun 14, 2016

Awesome. A lot of people excited about this. I'm still a bit intimidated by the support issue with the duplication, but I'm happy to put that aside in the interest of getting this popular feature merged.

So I've just checked out this branch and it's hitting over 100 linter errors. We need 0 errors, 0 warnings before merging. I used esnext, but it's mostly replacement of var with let/const as appropriate so it might be fine to do it by hand.

Then this branch should be rebased onto master. The commit history is also full of noise (merge commits etc, bad commit messages like 'fix bugs'), so it should be squashed into a single (or at least fewer) commits.

I have no access to Oracle, but thanks to @williamgueiros in #1491, we should have the ability to test on Travis. So we can pull in his PR and see those tests run live.

So, steps in summary (anybody is able to do this btw, not just @atiertant):

  1. Create a new PR with a single (or at least fewer) commits rebased onto current master HEAD.
  2. Correct for linter warnings.
  3. Merge in williamgueiros/knex#1
  4. Ensure tests run successfully
  5. Merge
@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Jun 14, 2016

PS. If somebody is planning to tackle the above task - please make sure to comment here first to avoid double handling.

@ramirezd42

This comment has been minimized.

Copy link

ramirezd42 commented Jun 14, 2016

I'd be happy to clean up the linter errors if someone else isn't already
doing so.

On Tuesday, June 14, 2016, Rhys van der Waerden notifications@github.com
wrote:

PS. If somebody is planning to tackle the above task - please make sure to
comment here first to avoid double handling.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#990 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACGSZkW2mLmvOhPqkY7q2am8AWhTQz3Mks5qLoRMgaJpZM4GBijT
.

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Jun 14, 2016

@ramirezd42 Great. I think we can safely assume that nobody is doing it at this stage. Perhaps just fork from atiertant's branch and merge in master from knex. We can sort out squashing etc later.

@ramirezd42

This comment has been minimized.

Copy link

ramirezd42 commented Jun 15, 2016

I fixed all of the linter issues in a PR to @atiertant 's fork here: https://github.com/Atlantis-Software/knex/pull/11.

I can make a separate PR if necessary, but it seemed easier to just wait for @atiertant to accept changes into that PR to keep the conversation in this thread.

All of the tests pass except for one mariadb test which didn't pass for me on tgriesser/knex master either so I can't imagine it was related:

1) Integration Tests mariadb | mariasql "before all" hook: Error: Pool is destroyed at Pool.acquire (node_modules/pool2/lib/pool.js:166:12) at ../src/client.js:2:32 at Client.acquireConnection (../src/client.js:2:32) at ../src/runner.js:1:41 From previous event: at ../src/runner.js:1:41 From previous event: at Runner.ensureConnection (../src/runner.js:1:41) at Runner.run (../src/runner.js:1:41) at SchemaBuilder.Target.then (../src/interface.js:2:20) at Migrator.forceFreeMigrationsLock (lib/migrate/index.js:9:6636) at Context.<anonymous> (test/integration/migrate/index.js:15:25)

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Jun 15, 2016

I fixed all of the linter issues in a PR to @atiertant 's fork

Excellent. Nice one.

All of the tests pass except for one mariadb test which didn't pass for me on tgriesser/knex master either so I can't imagine it was related

All good. I think that test will pass if we keep re-running the tests. (That's a problem, but not related to this PR).

We're at a point now where I could just pull all this stuff in manually and merge in the CI test branch. But I'll give @atiertant a chance to respond to your PR first.

atiertant and others added some commits Jun 15, 2016

Merge pull request #11 from ramirezd42/master
Fixed linter issues and merged in upstream changes
Merge pull request #12 from CorvusCorrax/master
Add .completed to transaction acquireConnection()
@atiertant

This comment has been minimized.

Copy link
Contributor Author

atiertant commented Jun 15, 2016

@rhys-vdw so what's next? whould you like to merge in the CI test branch?

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Jun 18, 2016

Hey @atiertant. I want to get #1503 sorted so we know everything's functioning as expected. Hopefully we'll have an elegant solution for testing Oracle soon.

@elhigu elhigu merged commit 53d089b into tgriesser:master Jun 20, 2016

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jun 20, 2016

Merged this branch as were discussed on contributor chat... If there are issues to be fixed in tests when oracle CI is setup those will be fixed as part of CI enabling pull request. Thanks @atiertant and everyone else 👍

@mrodrig

This comment has been minimized.

Copy link

mrodrig commented Jul 20, 2016

Any idea when the next release that will include this will be? Would like to use knex for Oracle, but can't until this is released.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jul 21, 2016

@mrodrig Looks like we can still do minor release 0.11.8 to release this. I skimmed through latest merges and there didn't seem to be incompatible changes, except bug fixes. Just need to write changelog, bump the version and publish. If I don't have anything important this night I can do it... not making promises though 👍

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jul 21, 2016

@mrodrig 0.11.8 is now released with this included

@mrodrig

This comment has been minimized.

Copy link

mrodrig commented Jul 22, 2016

Awesome- Thank you @elhigu!

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