Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

oracle: new connector #437

Merged
merged 5 commits into from
May 25, 2018
Merged

oracle: new connector #437

merged 5 commits into from
May 25, 2018

Conversation

n-riesco
Copy link
Contributor

  • Implemented connector to Oracle based on node-oracledb.

  • Implemented a pool of clients to ensure all the request from a
    connector share the same client. For more details, see
    backend/persistent/datastores/pool.js.

  • Upgraded to electron@2.0 and node@8, so that:

    • both node and electron have ABI 57 (for which node-oracledb provides
      binaries)
    • unable to use electron@1.8 due to issue resulting in a empty window
      at some startups
  • Added Dockerfile and npm scripts to build and launch a container with
    an Oracle Express database setup for testing.

  • Ensure connector failures are logged.

  • Added npm script test-unit-oracle for testing. See CONTRIBUTING.md
    for the requirements to run these tests.

  • Updated documentation in CONTRIBUTING.md.

Closes #322

@n-riesco
Copy link
Contributor Author

n-riesco commented May 17, 2018

TODO:

@n-riesco n-riesco force-pushed the oracle branch 2 times, most recently from ab3b642 to bf60ecc Compare May 18, 2018 18:50
Copy link
Contributor

@shannonlal shannonlal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need an oracle.LICENSE file as well?

@@ -3,7 +3,7 @@ matrix:
- os: osx
osx_image: xcode9.0
language: node_js
node_js: "6"
node_js: "8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm. Once this is moved in we will need to use NodeJS 8? Will earlier versions work? Not an issue just want to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The oracle connector won't work in electron and yarn will refuse to install.

If you force yarn to ignore the node version (yarn install --ignore-engines), then Falcon seems to work (I mean everything I tried, except the Oracle connector).

@shannonlal
Copy link
Contributor

Looks good. No major issues. Just had a couple of quick questions for you on but should not block merging this in💃

@n-riesco
Copy link
Contributor Author

@shannonlal

Do you need an oracle.LICENSE file as well?

We need written permission to use Oracle's logo. For the time being, the icon for Oracle's connector is just text (similar to what I've used for DB2 and CSV).

@n-riesco
Copy link
Contributor Author

n-riesco commented May 23, 2018

I've tested this branch on windows:

  • the web app works (i.e. oracledb works in Node)
  • the desktop app doesn't (i.e. oracledb fails when required from Electron)

The issue I have with the electron app has already been reported: oracle/node-oracledb#917

Running .\node_modules\.bin\electron-rebuild.cmd --version=2.0.1 --arch=x64 --only=oracledb as suggested in oracle/node-oracledb#881 (comment) fails with NAN-related errors (like these reported in oracle/node-oracledb#862 (comment))


Potential leads:

n-riesco added 5 commits May 24, 2018 20:22
* Implemented connector to Oracle based on node-oracledb.

* Implemented a pool of clients to ensure all the request from a
  connector share the same client. For more details, see
  `backend/persistent/datastores/pool.js`.

* Upgraded to electron@2.0 and node@8, so that:
  - both node and electron have ABI 57 (for which node-oracledb provides
    binaries)
  - unable to use electron@1.8 due to issue resulting in a empty window
    at some startups

* Added Dockerfile and npm scripts to build and launch a container with
  an Oracle Express database setup for testing.

* Ensure connector failures are logged.

* Added npm script `test-unit-oracle` for testing. See `CONTRIBUTING.md`
  for the requirements to run these tests.

* Updated documentation in `CONTRIBUTING.md`.

Closes #322
* See nodejs/node#16196 for further details
  about the cause.
* Ensure error response is formatted as {error: message: 'xxxx'}.
* Fixed the code parsing a connection error message.

* Ensure error message is a string.

* Ensure isConnected() and isSaved() returns false when status code is
  500.

* Added tests.

Closes #342
* Use source package from github to prevent the use of prebuilt
  binaries and force the rebuild of the native module.
@n-riesco
Copy link
Contributor Author

3c13700 fixes the issue with the windows installer.

I've tested both, the linux and the windows installer.

@tarzzz, could you test the mac installer here, please?

To test the connector for oracle, you'll need to:

  • Install the free Oracle Instant Client for mac, as described here
  • Build the docker image with the oracle database for testing: yarn docker:oracle:build
  • Launch the docker image: yarn docker:oracle:start
  • Wait until the docker container prints out Ready
  • Install falcon using the installer here
  • Open Falcon and create an connection to oracle using the sample credentials.

@tarzzz
Copy link
Contributor

tarzzz commented May 25, 2018

Getting this while trying to connect:

image

It seems an issue with my Oracle client installation.. !!

@tarzzz
Copy link
Contributor

tarzzz commented May 25, 2018

Okay, I was able to make it work by correctly installing clients:

image

Everything looks good to me from testing point of view.. 💃

@n-riesco n-riesco merged commit 125ddb1 into master May 25, 2018
@n-riesco
Copy link
Contributor Author

@tarzzz I'm planning to update README.md with the instructions for installing Oracle Install Client.
For mac, did you have to do anything else other what is described here?

@n-riesco n-riesco mentioned this pull request May 25, 2018
10 tasks
@n-riesco n-riesco deleted the oracle branch May 28, 2018 16:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants