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

Accept an object as first parameter in `execute` et al. #1629

Closed PhilippSalvisberg closed 6 months ago

PhilippSalvisberg commented 10 months ago

The popular npm module sql-template-tag supports oracledb in the latest version 5.2.0. So we can write code like this:

const query = sql`select * from emp where deptno = ${deptno} order by sal desc`;
const result = session.execute(query.statement, query.values);

However, I'd like to make the code more compact. Similar to MySQL and PostgreSQL. Something like this:

const result = session.execute(sql`select * from emp where deptno = ${deptno} order by sal desc`);

To make it work, the execute function and related functions need to accept a JSON object as first parameter. Similar to the implementation in MySQL for query. The object contains a statement field with positional bind parameters for oracledb. The bind values are provided in the values field.

IMO this would improve the usability in a backward-compatible way. Making the use of bind parameters as simple as in MySQL and PostgreSQL.

Using the sql-template-tag makes template literals containing large SQL statements easy to read, even with positional bind parameters. It's similar to static SQL in PL/SQL. This makes the use of named bind parameters in a lot of cases unnecessary.

sharadraju commented 10 months ago

HI @PhilippSalvisberg Thank you for using node-oracledb. I assume the npm module sql-template-tag supports the latest node-oracledb version of 6.2, not 5.2 :) Thank you for suggesting this feature. Rest assured, we are evaluating this enhancement as it looks interesting!

PhilippSalvisberg commented 10 months ago

I assume the npm module sql-template-tag supports the latest node-oracledb version of 6.2, not 5.2 :)

I was referring to the sql-template-tag version. ;-)

Rest assured, we are evaluating this enhancement as it looks interesting!

Sounds promising. Thank you.

sudarshan12s commented 8 months ago

Hi @PhilippSalvisberg , Can you try this patch which enables execute API to take an object. It will be included officially in upcoming 6.4 release.

diff --git a/lib/connection.js b/lib/connection.js
index 551a2d45..5f1b348d 100644
--- a/lib/connection.js
+++ b/lib/connection.js
@@ -847,13 +847,24 @@ class Connection extends EventEmitter {
     let options = {};

     // process arguments
-    errors.assertArgCount(arguments, 1, 3);
-    errors.assertParamValue(typeof sql === 'string', 1);
-    if (arguments.length >= 2) {
-      binds = await this._processExecuteBinds(a2);
-    }
-    if (arguments.length == 3) {
-      options = this._verifyExecOpts(a3, false);
+    if (nodbUtil.isObject(sql) && typeof sql.statement === 'string') {
+      errors.assertArgCount(arguments, 1, 2);
+      if (sql.values) {
+        binds = await this._processExecuteBinds(sql.values);
+      }
+      sql = sql.statement;
+      if (arguments.length == 2) {
+        options = this._verifyExecOpts(a2, false);
+      }
+    } else {
+      errors.assertArgCount(arguments, 1, 3);
+      errors.assertParamValue(typeof sql === 'string', 1);
+      if (arguments.length >= 2) {
+        binds = await this._processExecuteBinds(a2);
+      }
+      if (arguments.length == 3) {
+        options = this._verifyExecOpts(a3, false);
+      }
     }
     this._addDefaultsToExecOpts(options);
     errors.assert(this._impl, errors.ERR_INVALID_CONNECTION);

I noted the following while i was testing the same with the sql-template-tag module

  1. bulk insert or executeMany doesn't work as the expected positional binds, values differ in syntax from what the sql function returns..
  2. RETURNING Clause with out binds for INTO clause don't work as the sql function expects the binds to have input values given.
PhilippSalvisberg commented 8 months ago

Hi @sudarshan12s,

If you can provide me with a compiled version that runs on macOS Silicon, I'd be happy to try it out. No problem, If this is not possible/too complicated. I will test it once 6.4 is released.

Thank you for including this change.

sharadraju commented 8 months ago

Hi @PhilippSalvisberg I have added the latest change with the sql object support to the GitHub repo here.

All you need to do is the following:

Since this is a JavaScript change, there is no need for any compile.

If it is still too complicated or if there are other issues arising from integrating this file, it will be available in the 6.4 release.

PhilippSalvisberg commented 8 months ago

Works, but I had to change the signature for execute in @types/oracledb/index.d.ts

from

        execute<T>(sql: string): Promise<Result<T>>;

to

        execute<T>(sql): Promise<Result<T>>;

or

        execute<T>(sql: string|unknown): Promise<Result<T>>;

to avoid an Argument of type 'Sql' is not assignable to parameter of type 'string'.ts(2345) error in my test.

PhilippSalvisberg commented 8 months ago

So, besides the change of the driver, this file/line needs to be patched as well when working with TypeScript.

anthony-tuininga commented 8 months ago

Looks to me like a new Interface needs to be added and another option for execute() added instead. Your solution works but leaves the interface undefined -- a good stopgap solution but not a good final one!

sudarshan12s commented 7 months ago

Thanks @PhilippSalvisberg for pointing it out. We will check on updating the types definition..

PhilippSalvisberg commented 7 months ago

Thanks, @sudarshan12s for taking care of the TypeScript types definitions. Very much appreciated.

sharadraju commented 6 months ago

@PhilippSalvisberg This is now available in node-oracledb 6.4. We have raised a PR for the TypeScript update

sudarshan12s commented 6 months ago

The type definition change PR is here.

lucasbraun commented 1 month ago

By the way, this is now also part of mle-js-oracledb, i.e. for In-Database JavaScript in Oracle 23.5:

PhilippSalvisberg commented 1 month ago

Perfect. Thanks @lucasbraun