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

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

Closed
PhilippSalvisberg opened this issue Nov 28, 2023 · 14 comments
Closed

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

PhilippSalvisberg opened this issue Nov 28, 2023 · 14 comments

Comments

@PhilippSalvisberg
Copy link

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.

@PhilippSalvisberg PhilippSalvisberg changed the title Enhancement Request: Accept an object as first parameter in execute et. al. Accept an object as first parameter in execute et al. Nov 28, 2023
@sharadraju
Copy link
Member

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
Copy link
Author

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
Copy link

sudarshan12s commented Jan 31, 2024

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
Copy link
Author

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
Copy link
Member

sharadraju commented Feb 5, 2024

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:

  • Take the latest lib/connection.js file and replace it in your local NODE-ORACLEDB home directory (i.e., the $NODE_ORACLEDB/lib/connection.js file)
  • Run your test case.

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
Copy link
Author

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
Copy link
Author

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

@anthony-tuininga
Copy link
Member

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
Copy link

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

@PhilippSalvisberg
Copy link
Author

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

@sharadraju
Copy link
Member

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

@sudarshan12s
Copy link

The type definition change PR is here.

@lucasbraun
Copy link
Member

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

@PhilippSalvisberg
Copy link
Author

Perfect. Thanks @lucasbraun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants