oracle / node-oracledb

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

Using the v4.0.0, when the `Object.prototype` had been chained a method, the `Connection.executeMany` goes error with `NJS-060` prompt. #1129

Closed TheNorthMemory closed 5 years ago

TheNorthMemory commented 5 years ago

The behavior is similar to node-modules/urllib#312, the sample code as below:

const data = [
    {"rate_user":"Bob","rate_time":"2019-07-26 19:42:36","item_price":338,"rating":"I like it"},
    {"rate_user":"Alice","rate_time":"2019-07-26 19:42:36","item_price":338,"rating":"I like it too"}
];

const cfg = require('./dbConfig');
const oracledb = require('oracledb');

Object.prototype.noop = () => {}

(async () => {

  const options = {
    autoCommit: true,
    bindDefs: {
      item_price : { type: oracledb.NUMBER },
      rate_time  : { type: oracledb.STRING, maxSize: 32 },
      rate_user  : { type: oracledb.STRING, maxSize: 128 },
      rating     : { type: oracledb.STRING, maxSize: 4000 },
    }
  };

  const sql = `MERGE INTO RATING R USING ( SELECT
      :item_price as itemPrice,
      :rate_time as rateTime,
      :rate_user as rateUser,
      :rating as rating
  FROM DUAL ) T ON (
      R.RATE_TIME     = T.rateTime
      AND R.RATE_USER = T.rateUser
  ) WHEN MATCHED THEN UPDATE SET
      R.ITEM_PRICE = T.itemPrice
      , R.RATING   = T.rating
  WHEN NOT MATCHED THEN INSERT (
      RATE_TIME
      , RATE_USER
      , ITEM_PRICE
      , RATING
  ) VALUES (
      T.rateTime
      , T.rateUser
      , T.itemPrice
      , T.rating
  )`;

  const conn = await oracledb.getConnection(cfg);
  try {
    await conn.executeMany(sql, data, options);
  } catch (e) {
    console.error(e);
  }
  await conn.close();

})();
  1. Is it an error or a hang or a crash?

crashed with this message:Error: NJS-060: type must be specified for bind "noop"

  1. What error(s) you are seeing?
Error: NJS-060: type must be specified for bind "noop"
    at Connection.executeMany (/Users/james/data.git/node_modules/oracledb/lib/connection.js:207:21)
    at /Users/james/data.git/node_modules/oracledb/lib/util.js:180:16
    at new Promise (<anonymous>)
    at Connection.executeMany (/Users/james/data.git/node_modules/oracledb/lib/util.js:168:14)
    at /Users/james/data.git/test/rate.executeMany.test.js:48:18
  1. Run node and show the output of:
Welcome to Node.js v12.6.0.
Type ".help" for more information.
> process.platform
'darwin'
>
> process.version
'v12.6.0'
> process.arch
'x64'
> require('oracledb').versionString
'4.0.0'
>
cjbj commented 5 years ago

Thanks for the report. Keep testing and let us know what else you find.

Did this work in in node-oracledb 3.1.2?

Can you share the SQL to create the table?

TheNorthMemory commented 5 years ago

it works well under node-oracledb@3.1.2 and node@11.12.0.

table script as below:

CREATE TABLE "DATAHOUSE"."RATING" 
   (    "RATE_TIME" VARCHAR2(32), 
    "RATE_USER" VARCHAR2(128), 
    "ITEM_PRICE" NUMBER, 
    "RATING" VARCHAR2(4000)
   )
cjbj commented 5 years ago

Thank you. We'll review it.

anthony-tuininga commented 5 years ago

In discussion with @dmcghan I have confirmed that the v3 code used a method that only returned its "own" property names. v4 uses N-API which does not provide that directly, so some additional effort will need to be made to filter out properties that are not "owned" by the object. A patch will follow in due time. Thanks for letting us know about this!

anthony-tuininga commented 5 years ago

Here is a preliminary patch:

diff --git a/src/njsBaton.c b/src/njsBaton.c
index 3137446d..699bb7fb 100644
--- a/src/njsBaton.c
+++ b/src/njsBaton.c
@@ -452,7 +452,8 @@ bool njsBaton_getFetchInfoFromArg(njsBaton *baton, napi_env env,
         return true;

     // extract the property names from the object
-    NJS_CHECK_NAPI(env, napi_get_property_names(env, value, &keys))
+    if (!njsUtils_getOwnPropertyNames(env, value, &keys))
+        return false;

     // allocate space for the fetchInfo based on the number of keys
     NJS_CHECK_NAPI(env, napi_get_array_length(env, keys, &numElements))
diff --git a/src/njsConnection.c b/src/njsConnection.c
index 6826ee02..5e2d6488 100644
--- a/src/njsConnection.c
+++ b/src/njsConnection.c
@@ -1982,9 +1982,8 @@ static bool njsConnection_processExecuteBinds(njsBaton *baton,

     // if binding by name, get the list of bind names
     bindNames = NULL;
-    if (!isArray) {
-        NJS_CHECK_NAPI(env, napi_get_property_names(env, binds, &bindNames))
-    }
+    if (!isArray && !njsUtils_getOwnPropertyNames(env, binds, &bindNames))
+        return false;

     // initialize variables; if there are no variables, nothing further to do!
     baton->bindArraySize = 1;
@@ -2057,9 +2056,8 @@ static bool njsConnection_processExecuteManyBinds(njsBaton *baton,

     // get the list of bind names, if binding by name
     bindNames = NULL;
-    if (!bindByPos) {
-        NJS_CHECK_NAPI(env, napi_get_property_names(env, bindDefs, &bindNames))
-    }
+    if (!bindByPos && !njsUtils_getOwnPropertyNames(env, bindDefs, &bindNames))
+        return false;

     // initialize variables; if there are no variables, nothing further to do!
     if (!njsConnection_initBindVars(baton, env, bindDefs, bindNames))
diff --git a/src/njsModule.h b/src/njsModule.h
index d712baf2..f8867b6a 100644
--- a/src/njsModule.h
+++ b/src/njsModule.h
@@ -943,6 +943,8 @@ bool njsUtils_getError(napi_env env, dpiErrorInfo *errorInfo,
 bool njsUtils_getIntArg(napi_env env, napi_value *args, int index,
         int32_t *result);
 napi_value njsUtils_getNull(napi_env env);
+bool njsUtils_getOwnPropertyNames(napi_env env, napi_value value,
+        napi_value *names);
 bool njsUtils_getStringArg(napi_env env, napi_value *args, int index,
         char **result, size_t *resultLength);
 bool njsUtils_getStringFromArg(napi_env env, napi_value *args,
diff --git a/src/njsUtils.c b/src/njsUtils.c
index ea8b57b8..0a7755c0 100644
--- a/src/njsUtils.c
+++ b/src/njsUtils.c
@@ -508,6 +508,27 @@ napi_value njsUtils_getNull(napi_env env)
 }

+//-----------------------------------------------------------------------------
+// njsUtils_getOwnPropertyNames()
+//   Returns an array of property names owned specifically by the given object.
+//-----------------------------------------------------------------------------
+bool njsUtils_getOwnPropertyNames(napi_env env, napi_value value,
+        napi_value *names)
+{
+    napi_value global, globalObject, method;
+
+    NJS_CHECK_NAPI(env, napi_get_global(env, &global))
+    NJS_CHECK_NAPI(env, napi_get_named_property(env, global, "Object",
+            &globalObject))
+    NJS_CHECK_NAPI(env, napi_get_named_property(env, globalObject,
+            "getOwnPropertyNames", &method))
+    NJS_CHECK_NAPI(env, napi_call_function(env, globalObject, method, 1,
+            &value, names))
+
+    return true;
+}
+
+
 //-----------------------------------------------------------------------------
 // njsUtils_getStringArg()
 //   Gets a string from the specified parameter. If the value is not a string,
cjbj commented 5 years ago

@TheNorthMemory have you tested the proposed patch? We'd like confirmation before we merge it. You will need to build the source code with the patch applied: https://oracle.github.io/node-oracledb/INSTALL.html#github

TheNorthMemory commented 5 years ago

@cjbj before we were only fetch the package from npmjs.com onto the docker container. Let me see how to build from the source code with this patch. feedback later.

cjbj commented 5 years ago

@TheNorthMemory thank you.

Are there any other issues you're seeing with 4.0.0?

TheNorthMemory commented 5 years ago

@cjbj @anthony-tuininga great thx the patch, it works well.

just a little bit problem that hard to build the custom package :(.

here's my env & steps:

uname -a && env
Linux e783b2147f67 3.10.0-862.9.1.el7.x86_64 #1 SMP Mon Jul 16 16:29:36 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

NODE_VERSION=12.7.0
HOSTNAME=e783b2147f67
LD_LIBRARY_PATH=/usr/lib/oracle/11.2/client64/lib
PATH=/usr/lib/oracle/11.2/client64/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/app
LANG=en_US.UTF-8
SHLVL=1
HOME=/root
YARN_VERSION=1.17.3
mkdir -p /app
yum install -y gcc-c++ make
npm install node-gyp --verbose -g
git clone -b v4.0.0 --depth 1 --single-branch https://github.com/oracle/node-oracledb.git /app/node-oracledb
cd /app/node-oracledb
git submodule init
git submodule update
git apply /tmp/v4.0.0.patch
cd /app
npm install /app/node-oracledb

and then, i'd run the first floor javascript, it works.

i'd also try to build a fresh npm-style-package, unfortunately, i didnot got the package, steps as below:

cd /app/node-oracledb
node-gyp clean
npm build .
npm WARN lifecycle oracledb@4.0.0~install: cannot run in wd oracledb@4.0.0 node-gyp rebuild (wd=/app/node-oracledb)

and retried following steps, also failed:

cd /app/node-oracledb
node-gyp clean
node-gyp rebuild
npm run buildbinary
> oracledb@4.0.0 buildbinary /app/node-oracledb
> node package/buildbinary.js

Building binary oracledb-4.0.0-linux-x64.node using Node.js v12.7.0
npm WARN lifecycle oracledb@4.0.0~install: cannot run in wd oracledb@4.0.0 node-gyp rebuild (wd=/app/node-oracledb)
ENOENT: no such file or directory, rename 'build/Release/oracledb.node' -> 'package/Staging/oracledb-4.0.0-linux-x64.node'

i think that the patch solution is only verified work, but not product ready, waiting for the official npm package.

cjbj commented 5 years ago

@TheNorthMemory thanks for the confirmation and detail.

I'm not totally surprised that buildbinary doesn't work with the Node rpms. Those packages are a little different to the binaries I am using from nodejs.org. It would be great if you submit a PR to make it work.

PS, Oracle Client 19.3 connects to Oracle DB 11.2 or later and can be installed from yum.oracle.com without needing a clickthrough, e.g.: yum -y install oracle-release-el7 && yum -y install oracle-instantclient19.3-basic

TheNorthMemory commented 5 years ago

@cjbj patching here

diff --git a/package/buildbinary.js b/package/buildbinary.js
index 5d6e9ce6..010be3d7 100644
--- a/package/buildbinary.js
+++ b/package/buildbinary.js
@@ -66,7 +66,7 @@ function buildBinary() {
     fs.unlink(buildBinaryFile, function(err) {if (err && !err.message.match(/ENOENT/)) throw(err);});
     fs.unlink(binaryStagingFile, function(err) {if (err && !err.message.match(/ENOENT/)) throw(err);});
     fs.unlink(binaryStagingInfoFile, function(err) {if (err && !err.message.match(/ENOENT/)) throw(err);});
-    execSync('npm install');
+    execSync('node-gyp rebuild');
     fs.renameSync(buildBinaryFile, binaryStagingFile);
     fs.appendFileSync(binaryStagingInfoFile, buildInfo + "\n");
   } catch(err) {

and then, the full working steps as below:

mkdir -p /app
yum install -y gcc-c++ make
npm install node-gyp --verbose -g
git clone -b v4.0.0 --depth 1 --single-branch https://github.com/oracle/node-oracledb.git /app/node-oracledb
cd /app/node-oracledb
git submodule init
git submodule update
git apply /tmp/v4.0.0.patch
npm run buildbinary --verbose
npm run buildpackage --verbose
cd /app
npm install /app/node-oracledb/oracledb-4.0.0.tgz --no-save

tested under mac node@12.6.0&npm@6.10.2 and centos7 node@12.7.0&npm@6.10.0, both are working expectedly.

cjbj commented 5 years ago

That works in my macOS and Linux environments; I'll get Windows checked and see about merging. Thanks!

cjbj commented 5 years ago

@TheNorthMemory node-oracledb 4.0.1 has the fix for adding enumerable properties to Object.prototype. Thanks for reporting the problem.