oracle / node-oracledb

Oracle Database driver for Node.js maintained by Oracle Corp.
http://oracle.github.io/node-oracledb/
Other
2.24k stars 1.07k forks source link

Relax error to warning for "getting the Oracle Client version" for Knex #1582

Closed robmcguinness closed 10 months ago

robmcguinness commented 1 year ago

First, thanks for thin client. Really improves the developer experience when leveraging node-oracledb in Nodejs.

Problem Statement

Trying to leverage node-oracledb thin mode with knex and running into issues with migrations. Knex makes a clone of the Oracle client during the migration initialization phase which leverages lodash merge underneath. Lodash merge iterates through the Oracle client properties which eventually triggers a call to the getter for oracleClientVersion https://github.com/oracle/node-oracledb/blob/eadafe2818ba083c32bc29210128188729761f8f/lib/oracledb.js#L1055

Feature Request

Can this error be converted to a warning instead for lib compatibility? Knex is one of the few query builders in the Nodejs ecosystem that supports Oracle. Fixing the upstream lib would take more work.

Commenting lines below resolves issue with Knex migrations.

get oracleClientVersion() {
  if (_initOracleClientArgs === undefined) {
    // errors.throwNotImplemented("getting the Oracle Client version");
  }
  return settings.oracleClientVersion;
},

get oracleClientVersionString() {
  if (_initOracleClientArgs === undefined) {
    // errors.throwNotImplemented("getting the Oracle Client version");
  }
  return settings.oracleClientVersionString;
},

Besides this issue, thin mode appears to work with current version of Knex. For reference, Knex feature request for node-oracledb v6.0 compatibility https://github.com/knex/knex/issues/5615

cjbj commented 1 year ago

Thanks for your comments.

What exact behavior do you suggest? Can you submit a PR?

robmcguinness commented 1 year ago

I used pnpm to patch oracledb and simply used console.warn as a workaround.

   get oracleClientVersion() {
     if (_initOracleClientArgs === undefined) {
-      errors.throwNotImplemented("getting the Oracle Client version");
+      console.warn('node-oracledb: getting the Oracle Client version is not supported in thin mode');
+      return undefined;
     }
     return settings.oracleClientVersion;
   },

   get oracleClientVersionString() {
     if (_initOracleClientArgs === undefined) {
-      errors.throwNotImplemented("getting the Oracle Client version");
+      console.warn('node-oracledb: getting the Oracle Client version is not supported in thin mode');
+      return undefined;
     }
     return settings.oracleClientVersionString;
   },

I didn't see any methods to emit warnings from this lib. I could submit a PR if the above workaround is sufficient.

cjbj commented 1 year ago

We use errors.throwNotImplemented() in a lot of places, but being inconsistent for oracleClientVersion() does have merit. We'll have to analyze all the pros & cons to check the impacts - and determine what version such a change could land in.

sosoba commented 1 year ago

These functions should return thin string, not an exception

anthony-tuininga commented 1 year ago

Or just the value undefined in both cases? The string thin would make sense for the second case but not the first one! Whereas undefined makes sense in both cases.

sharadraju commented 10 months ago

This enhancement has been added to the 6.1 release.