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

Handling infileStreamFactory at connection-level #2151

Closed
loozhengyuan opened this issue Jul 28, 2023 · 3 comments · Fixed by #2159
Closed

Handling infileStreamFactory at connection-level #2151

loozhengyuan opened this issue Jul 28, 2023 · 3 comments · Fixed by #2159

Comments

@loozhengyuan
Copy link
Contributor

The infileStreamFactory option only works when calling connection.query() or connection.execute(), which is as documented.

However, if I set it on a connection-level as suggested in #1080, the configuration is currently not propagated when calling connection.query() or connection.execute(). Since I am using Sequelize, which only exposes the connection configuration via dialectOptions, there is no other way I can set the infileStreamFactory option.

To help move this forward, I am keen to know where the change should be made:

  1. To support the infileStreamFactory connection option in mysql2 and pass it when calling Commands.Execute()/Commands.Query(); or
  2. To make the change in Sequelize instead to expose these options at a query-level.

Any comments/feedback is deeply appreciated, thanks!

@sidorares
Copy link
Owner

hi @loozhengyuan I'm keen to have this fixed on mysql2 side, can we start with a simple repro example?
We might to change api slightly, if infileStreamFactory is set on a connection/pool level it probably needs to see the query text to be able to decide which file to return, maybe pass sql as a second parameter: (path, fullQuery) => ReadableStream

@loozhengyuan
Copy link
Contributor Author

Sure, I have created a repro of the issue, using mysql2 and sequelize to depict my current situation. Feel free to try it out.

Separately, I also created a draft version of the patch and managed to get it working in the feat/connection-level-infilestreamfactory branch. Not too familiar with the code base so happy to hear any feedback that you have. I can also submit the PR for review if the changes are in the right direction.

@sidorares
Copy link
Owner

the change looks ok. We might actually not need extra sql parameter. My thinking was more of a "virtual file system": given query X and a file name Y (from that query), return a way to read the data. But in reality most use cases is just a security gate / allowlist: an INFILE query wants a file XYZ, if it's a safe path read the file under the same name from fs

Feel free to open a PR. It's probably a minor version bump ( not patch ): clients using updated mysql2 library might not work with the version before your fix.

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

Successfully merging a pull request may close this issue.

2 participants