ForbesLindesay / atdatabases

TypeScript clients for databases that prevent SQL Injection
https://www.atdatabases.org
MIT License
609 stars 47 forks source link

Mysql queryNodeStream does't look to be correctly handling ending of stream results. #233

Open bjconlan opened 2 years ago

bjconlan commented 2 years ago

I'm currently dumping sql databases tables using the queryNodeStream and im seeing inconsistent results (perhaps the 'end' chunk sent by the mysql2 driver isn't correctly being handled?)

In anycase this is an example of the query (note the 'export_users' table a temporary table which just holds the ids of users who need to be dumped (ie single connection) NOTE: this is the second node pipeline call and non-streaming query just used to validate numbers

  console.log((await source.rhsso.query(sql`\
    SELECT identity_provider, REALM.name as realm_name, federated_user_id, federated_username, token, user_id
    FROM FEDERATED_IDENTITY
    LEFT JOIN REALM on REALM.id = FEDERATED_IDENTITY.realm_id
    WHERE EXISTS (SELECT * FROM export_users WHERE id = user_id)`)).length);

  await pipeline(
    source.rhsso.queryNodeStream(sql`
      SELECT identity_provider, REALM.name as realm_name, federated_user_id, federated_username, token, user_id
      FROM FEDERATED_IDENTITY
      LEFT JOIN REALM on REALM.id = FEDERATED_IDENTITY.realm_id
      WHERE EXISTS (SELECT * FROM export_users WHERE id = user_id)`),
    stringify({header: true}),
    createWriteStream(`${__dirname}/data/federated_identity.csv`)
  );
  console.log('Dumped federated identity table');

And the results reflect

1820 Dumped federated identity table Warning: got packets out of order. Expected 38 but received 0 (node_modules/mysql2/lib/connection.js:414)

And the dumped csv results only show 1792 result rows (1793 including csv headers) which exactly the number of results missing outlined form the 'query' based result.

NOTE the 1st pipeline dump executes successfully (only sequential seem to have this problem and looks like there are many open issues in mysql2 regarding this).

bjconlan commented 2 years ago

can also verify db.end() does resolve the issue on the mysql2 end (but ideally I'd like a single connection for this behaviour to prevent the temp table from being removed). In any case this looks to be an upstream related and not @database/mysql but perhaps there are workarounds that smarter minds can solve at the this library level.

I did just migrate this code over to use the mysql2 specific pool.query().streams() and am not seeing the issue. perhaps this is something in regards to the pool cleanup code? (after moving to the mysql2 query directly pool.end() seems to work as expected (replaced the @database/pg pool.dispose)

ForbesLindesay commented 2 years ago

If you're able to submit a PR with a failing test (or better yet, a newly passing test along with the bug fix). I'd be happy to have a look at this. I'm not sure I have the bandwidth to address this without that.