Prevent mutation of arguments

[diegomansua]

1. Describe your new request in detail

I spent a couple of hours yesterday debugging an issue caused by the mutation of arguments, specifically the options one, here: https://github.com/oracle/node-oracledb/blob/v5.5.0/lib/connection.js#L587

My code looked like this:

const DEFAULT_OPTIONS: ExecuteOptions = {outFormat: oracledb.OUT_FORMAT_OBJECT};

export const runQuery = async <T>(query: string, params: BindParameters = {}): Promise<T[]> => {
  const connection = await getPool().getConnection();

  try {
    const {rows} = await connection.execute(query, params, DEFAULT_OPTIONS);

    return rows as T[];
  } finally {
    connection.close().catch(() => {});
  }
};

export const runQueryGenerator = async function* <T>(query: string, params: BindParameters = {}): AsyncGenerator<T> {
  const connection = await getPool().getConnection();

  try {
    const rowStream = connection.queryStream<T>(query, params, DEFAULT_OPTIONS);

    for await (const row of rowStream) {
      yield row as T;
    }
  } finally {
    connection.close().catch(() => {});
  }
};

So if I called my runQueryGenerator util function and then the runQuery one, the latter would fail as rows would be undefined and there would be a resultSet field in the result instead, as the call to connection.queryStream is mutating my DEFAULT_OPTIONS const and setting resultSet = true.

This causes an issue which is hard to debug and that's easily preventable. The queryStream function can simply use the spread operator or Object.assign or whatever to create its own copy of the options argument.

There's also linting rules to spot this. https://eslint.org/docs/latest/rules/no-param-reassign

2. Give supporting information about tools and operating systems. Give relevant product version numbers

I'm using 5.3.0 but the issue is present in the latest 5.5.0.

[VegarRingdalAibel]

querystream does not return promise last time I checked, prob more like this

const rowStream = connection.queryStream<T>(query, params, DEFAULT_OPTIONS);
rowStream.on("data", function (rowData) {
  //do something here with row data
 })

Also need event for end/error etc like docs says

[diegomansua]

@VegarRingdalAibelconnection.queryStream does indeed not return a promise but a readable stream. Readable streams are async iterable so they enable the use of for await like I do in my code. In any case that's not the issue I was trying to raise, actually whatever that function returns is irrelevant.

[VegarRingdalAibel]

@diegomansua

read it a bit too quick

[anthony-tuininga]

I noticed this recently while working on the new enhancements for version 6 and took the time to eliminate the mutation there. So this will be corrected in version 6 anyway. I'll let @cjbj decide whether this needs to be addressed sooner -- but considering you are the first to notice this in the entire lifetime of the driver I suspect it will wait until version 6 is released!

[sosoba]

Regardless of the queryStream problem, you might consider replacing your for-await-yield loop with the third example of chapter 17.1.2.

[diegomansua]

@sosoba thanks, can you please elaborate why the code above might not work well?

I was hoping that using queryStream in that way would save me the hassle of having to close the result set etc.

Also I see in those docs that in v. 5.5.0 the result sets are async iterable, but the TS definitions haven't been updated yet, but I guess those are maintained by someone else.

[cjbj]

• @dannyb648 has been kind enough to update the TS definitions in the past
• @diegomansua are there any other eslintrc rules you'd like us to use as well as no-param-reassign?

[diegomansua]

@cjbj no that's the only issue I've had so the no-param-reassign (with props option set to true to prevent mutation and not just reassignment) is my only suggestion, thank you.

[sharadraju]

Hi @diegomansua, now that version 6.0 is out, Can you check if the mutation of arguments is happening for your code with the latest version? Check out #1552 for node-oracledb 6.0 release details.

[diegomansua]

@sharadraju Tested right now and works great, thanks! Also the thin mode is really nice.

[sharadraju]

Thanks a lot, @diegomansua. Closing this issue now.