Parameter binding fails for Oracle query when parameter name is reused

[arabull]

Issue description

Parameter binding fails for Oracle query when parameter name is reused

Expected Behavior

Queries with duplicate parameter names should bind successfully.

Actual Behavior

An error is thrown:

QueryFailedError: NJS-098: 2 positional bind values are required but 1 were provided

Steps to reproduce

Try to execute a query that contains duplicate parameter names. For example:

const repo = module.get<Repository<SomeEntity>>(getRepositoryToken(SomeEntity));
const qb = repo.createQueryBuilder();
qb.where('id = :id', { id: 'foo' }).orWhere('id = :id', { id: 'foo' });
const result = await qb.getManyAndCount();

This is obviously a contrived example, but the point is that it has two parameters with the same name. I experienced the failure on a much more complex query, but I wanted to distill it down as simply as I could for reproduction.

My Environment

Dependency	Version
Operating System	macOS
Node.js version	16.19.0
Typescript version	4.8.4
TypeORM version	0.3.20
oracledb version	6.4.0

Additional Context

This was caused by a fix by @iifawzi that went in for version 0.3.18. It's a good fix, and it works fine against the Oracle thick driver, but it fails against the thin driver, which is now standard (v6+ of oracledb).

The problem arises when this code runs in the driver: https://github.com/oracle/node-oracledb/blob/v6.4.0/lib/thin/connection.js#L169-L173. Because of the optimization put in place by the previous fix, the binds list no longer contains the duplicate parameter. Unfortunately, the statement.bindInfoList does, which leads to a mismatch and a ERR_WRONG_NUMBER_OF_POSITIONAL_BINDS error.

@iifawzi, I didn't tag you to call you out. On the contrary, you clearly know this code based on your previous fix and I'm hopeful that you may know a way around this new issue!

Relevant Database Driver(s)

• aurora-mysql
• aurora-postgres
• better-sqlite3
• cockroachdb
• cordova
• expo
• mongodb
• mysql
• nativescript
• oracle
• postgres
• react-native
• sap
• spanner
• sqlite
• sqlite-abstract
• sqljs
• sqlserver

Are you willing to resolve this issue by submitting a Pull Request?

No, I don’t have the time, but I can support (using donations) development.

[iifawzi]

Hello, I apologize for the inconvenience you experienced.
I will take a look at it. Thank you for your patience.

[iifawzi]

Hi again,

I'd like to thank you for the context; it made the investigation easier. I believe this issue should be addressed on the driver's side, especially since it used to work with the thick driver.

After spending some time trying to pinpoint the exact cause, I believe it lies in the binding logic at Statement#_addBind

I couldn't fully grasp why, in that segment of code, we bind parameters again even if they already exist. I believe the condition should be an 'AND' instead of 'OR', or we need to push it to the list only if it doesn't exist before.

cc @sharadraju, your insights here would be invaluable, I will rectify the issue either within the driver or here, accordingly.

[sharadraju]

@iifawzi There is an ongoing discussion in node-oracledb, where we do not recommend reuse of the same parameter names in the same statement. See oracle/node-oracledb#1652 (comment).
Not sure, if it is related.

Regarding repeating parameter names, we have the following documentation:

https://node-oracledb.readthedocs.io/en/latest/user_guide/bind.html#bind-by-position
If a bind parameter name is repeated in the SQL string, then bind by name syntax should be used.

Our team (probably @sudarshan12s) can help you out.

[sudarshan12s]

Thanks @iifawzi for analysis.

If we generate a sql like this, we need to repeat the bind values as its treated as bind by position

const binds =  ["python", "other", "python", "python2"];
await conn.execute(`select :b, DUMP(:b), :humptydumpty, DUMP(:b) from dual`, binds);

Earlier in thick mode, even without repetition, it worked but it was causing confusion and was not very consistent. Hence in thin mode, if user passes less than 4 bind values above, we return an error as below and forcing to repeat the bind values.

NJS-098: 4 positional bind values are required but 3 were provided

Instead if we use bindByName like this , we can skip repetition since it is clear in this syntax.
await conn.execute(select :b, DUMP(:b), :humptydumpty, DUMP(:b) from dual, {b:"python", humptydumpty: "other"});

From the thin driver, we do send bind values on wire for duplicate binds too and hence we have list of duplicate binds.
I think duplicated bind names are not frequent..

I hope it makes sense..

[iifawzi]

Thanks, @sharadraju and @sudarshan12s for taking the time to elaborate on this.

Regarding repeating parameter names, we have the following documentation:

https://node-oracledb.readthedocs.io/en/latest/user_guide/bind.html#bind-by-position If a bind parameter name is repeated in the SQL string, then bind by name syntax should be used.

Thanks for bringing this up, now, given it's intended from the driver side, and that Typeorm also supports named parameters, which can be sufficient for solving the issue, I do not see any value in doing the remapping ( from position parameters to named parameters if duplicated names are used ) underneath in Typeorm.

Would it solve your issue? @arabull
I'm not the maintainer though, @pleerock might have another opinion.