node-mysql2 icon indicating copy to clipboard operation
node-mysql2 copied to clipboard

Adapt to Cloudflare Workers environment

Open Mini256 opened this issue 7 months ago • 23 comments

try to close #2179

Summary

  • To fix the error window is not found, replace node Timers with web timers
  • To fix the error Code generation from strings disallowed for this context, using the static parser in PR #2099
  • To fix the error crypto.hash() is not a function, polyfill node crypto withwebcrypto, and modify the functions which directly/indirectly called the crypto library as async function.

TODOs:

  • [x] mysql_native_password auth plugin
  • [x] caching_sha2_password auth plugin
  • [x] sha256_password auth plugin
  • [x] static_text_parser (will be finished in PR #2099)
  • [ ] static_binary_parser (will be finished in PR #2099)
  • [ ] add integration tests
  • [ ] repalce pg-cloudflare with mysql2-cloudflare?

Usage

For users, they need to:

  1. Add node_compat = true flag to polyfill the missing node modules
  2. Add useStaticParser flag to force mysql2 to use static parser.

For example: https://github.com/Mini256/mysql2-cloudflare-test/blob/1f339a85f4457b696f122503051991326dbc23dc/src/index.ts#L30

Mini256 avatar Nov 21 '23 08:11 Mini256

This PR can run on Cloudflare Workers with the following database:

  • MySQL 5
  • MySQL 8
  • TiDB Serverless Cluster
  • PlanetScale

@sidorares could you please approve to run the GitHub action?

shiyuhang0 avatar Dec 25 '23 07:12 shiyuhang0

@shiyuhang0 @Mini256 thanks for working on this! Could you document simple steps I can follow to try it myself?

sidorares avatar Dec 25 '23 09:12 sidorares

@shiyuhang0 @Mini256 thanks for working on this! Could you document simple steps I can follow to try it myself?

https://github.com/sidorares/node-mysql2/pull/2289/commits/cc3458f23b222e49856d5acc14faa6f99f0671ee

This commit adds a step-by-step tutorial. Since it's user-facing, it needs a new release of MySQL2 including this PR. it's better to follow the develop guide in the doc to test locally without a new release.

shiyuhang0 avatar Dec 25 '23 11:12 shiyuhang0

CI is green. Maybe we can ignore the failure of CodeQL action, because the error is reported in the code that originally existed (just the location has changed)

As for the static parser. We'd like to merge your PR https://github.com/sidorares/node-mysql2/pull/2099 rather than cherry-pick it into this PR. What's your opinion? @sidorares The static binary parser is also needed to make things work. Code generation from strings disallowed for this context still occurs when code goes through the binary parser.

shiyuhang0 avatar Dec 27 '23 03:12 shiyuhang0

As for the static parser. We'd like to merge your PR #2099 rather than cherry-pick it into this PR. What's your opinion? @sidorares The static binary parser is also needed to make things work. Code generation from strings disallowed for this context still occurs when code goes through the binary parser.

I can probably merge it first, together with added binary protocol non-jit parser. I don't like the flag name "useStaticParser", it reflects more implementation details rather than intention, but don't have better ideas. Can you think of a better flag name? "disableEvals" ? "strictMode" ( could be an umbrella flag, also prohibits cleartext plugin etc ). disableGenFunction ( again, too focused on implementation ). Naming things is hard 🤷‍♂️

re testing - thanks a lot for dev documentation. Are you aware of any way to test workers locally or in actions runtime? something similar to localstack. Ideally with the same checks, e.i errors when code tries to eval. Any container we can use for that?

sidorares avatar Dec 28 '23 01:12 sidorares

Are you aware of any way to test workers locally or in actions runtime?

Cloudflare Workers provide an unstable_dev API for E2E / integration tests:

https://developers.cloudflare.com/workers/wrangler/api/#unstable_dev

Test code in node-postgres:

https://github.com/brianc/node-postgres/blob/6cd0aeb212d1672edd33499b2f4f858cf7ed9a79/packages/pg/test/worker/src/index.test.js#L13

Mini256 avatar Dec 28 '23 03:12 Mini256

I can probably merge it first, together with added binary protocol non-jit parser. I don't like the flag name "useStaticParser", it reflects more implementation details rather than intention, but don't have better ideas. Can you think of a better flag name? "disableEvals" ? "strictMode" ( could be an umbrella flag, also prohibits cleartext plugin etc ). disableGenFunction ( again, too focused on implementation ). Naming things is hard 🤷‍♂️

I prefer disableEvals among these names. You can also use other names, not good at naming for me :(

shiyuhang0 avatar Dec 28 '23 03:12 shiyuhang0

I prefer disableEvals among these names. You can also use other names, not good at naming for me :(

I like it more than useStaticParser, I think I'll update in my branch ( together with a version for no evals binary parser ) and comment here when ready

sidorares avatar Dec 28 '23 05:12 sidorares

I have tested this pr on multiple MySQL server ranging from old 5.7 to more recent MySQL and MariaDB version locally and on deployed workers so far it works flawlessly. The only issue is having to set node_compat = true instead of compatibility_flags = [ "nodejs_compat" ] but that's a small price to pay. Also it's currently not possible to use drizzle with msyql driver but kysely works ok.

advancedtw avatar Jan 23 '24 17:01 advancedtw

FYI:

  • Other drivers (like node-postgres and Postgres.js) also require node_compat = true today
  • We have plans to allow nodejs_compat to work: this will likely require us to better audit specific packages and conditionally load/exclude Node.js APIs that we don't have support for in Workers, or that don't make sense in a serverless environment like Workers.

Otherwise, we'd love to see this merged + we're bringing MySQL support to Hyperdrive: https://developers.cloudflare.com/hyperdrive/

elithrar avatar Jan 24 '24 13:01 elithrar

I have resolved the conflict with master branch. @sidorares Hi, what's the process of no evals parser? I'd like to remove the related codes in the PR after that.

shiyuhang0 avatar Mar 06 '24 03:03 shiyuhang0

Hi, @shiyuhang0 🙋🏻‍♂️

If it's okay, I'd like to make a few simple suggestions in the documentation:

  • Change from .md to .mdx
  • Change the title from Run on Cloudflare Workers to Cloudflare Workers
  • I think it fits in the Examples section, what do you think?

wellwelwel avatar Mar 06 '24 03:03 wellwelwel

Hi, @shiyuhang0 🙋🏻‍♂️

If it's okay, I'd like to make a few simple suggestions in the documentation:

  • Change from .md to .mdx
  • Change the title from Run on Cloudflare Workers to Cloudflare Workers
  • I think it fits in the Examples section, what do you think?

already doing this, will push a commit soon

shiyuhang0 avatar Mar 06 '24 04:03 shiyuhang0

@shiyuhang0 what do you think about useStaticParser flag name? Should we rename it to disableEval before its too late and somebody else depends on this name?

sidorares avatar Mar 06 '24 11:03 sidorares

@shiyuhang0 what do you think about useStaticParser flag name? Should we rename it to disableEval before its too late and somebody else depends on this name?

Both it is fine for me. But we shouldn't worry about modifying parameter names in an unmerged PR. For me, I think it can even be modified before release.

I will adjust this PR after the name is determined

shiyuhang0 avatar Mar 07 '24 05:03 shiyuhang0

I really need this feature. I need to use the node-mysql2 driver with Drizzle in Cloudflare Workers. If everything is confirmed to be correct, could you please merge this PR as soon as possible? Thanks

@sidorares

m430 avatar Mar 27 '24 03:03 m430

@m430 last time I checked Drizzle/Mysql was not compatible with cloudflare even using this updated driver. That is because of some node compatibility issue. If you really need a mysql driver for cf today! you can use a git dependecy or @nora-soderlund cloudflare-mysql

@shiyuhang0 disableEval makes more sense to me

advancedtw avatar Mar 27 '24 08:03 advancedtw

@shiyuhang0 missed your message, I also prefer disableEval. Are you able to continue or you need someone to take over your work?

sidorares avatar Mar 29 '24 00:03 sidorares

@shiyuhang0 missed your message, I also prefer disableEval. Are you able to continue or you need someone to take over your work?

@sidorares I have pushed a new commit to use disableEval instead of useStaticParser . Is there anything else I need to do?

shiyuhang0 avatar May 06 '24 02:05 shiyuhang0

Codecov Report

Attention: Patch coverage is 55.14706% with 183 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (master@41d21eb). Click here to learn what that means.

Files Patch % Lines
lib/parsers/static_text_parser.js 11.45% 116 Missing :warning:
lib/utils/nodecrypto.js 0.00% 33 Missing :warning:
lib/stream.js 61.36% 17 Missing :warning:
lib/auth_plugins/sha256_password.js 12.50% 7 Missing :warning:
lib/commands/client_handshake.js 70.00% 3 Missing :warning:
lib/connection.js 84.61% 2 Missing :warning:
lib/utils/crypto.js 77.77% 2 Missing :warning:
lib/auth_plugins/caching_sha2_password.js 92.30% 1 Missing :warning:
lib/commands/change_user.js 80.00% 1 Missing :warning:
lib/commands/query.js 88.88% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #2289   +/-   ##
=========================================
  Coverage          ?   89.47%           
=========================================
  Files             ?       76           
  Lines             ?    16021           
  Branches          ?     1363           
=========================================
  Hits              ?    14334           
  Misses            ?     1687           
  Partials          ?        0           
Flag Coverage Δ
compression-0 89.47% <55.14%> (?)
compression-1 89.47% <55.14%> (?)
tls-0 88.96% <52.45%> (?)
tls-1 89.08% <46.07%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 06 '24 03:05 codecov[bot]

Add node_compat = true flag to polyfill the missing node modules

Is there a path to instead supporting our modern nodejs_compat mode by updating imports (conditionally) vs using the legacy polyfill mode?

https://developers.cloudflare.com/workers/runtime-apis/nodejs/

elithrar avatar May 06 '24 21:05 elithrar

I have pushed a new commit to use disableEval instead of useStaticParser . Is there anything else I need to do?

Just a note, while in the docs it says disableEval, in code it's useStaticParser 🙋🏻‍♂️

wellwelwel avatar May 06 '24 21:05 wellwelwel

I have pushed a new commit to use disableEval instead of useStaticParser . Is there anything else I need to do?

Just a note, while in the docs it says disableEval, in code it's useStaticParser 🙋🏻‍♂️

I forget push the change in code

shiyuhang0 avatar May 07 '24 08:05 shiyuhang0