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

Add a string template tag handler for securely composing queries. #1926

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mikesamuel
Copy link

This is a rough draft. It is probably not suitable in its current
form.

https://nodesecroadmap.fyi/chapter-7/query-langs.html describes
this approach as part of a larger discussion about library support
for safe coding practices.

This enables

connection.query`SELECT * FROM T WHERE x = ${x}, y = ${y}, z = ${z}`(callback)

and similar idioms.

This is a rough draft.  It is probably not suitable in its current
form.

https://nodesecroadmap.fyi/chapter-7/query-langs.html describes
this approach as part of a larger discussion about library support
for safe coding practices.

This enables

    connection.query`SELECT * FROM T WHERE x = ${x}, y = ${y}, z = ${z}`(callback)

and similar idioms.
…t things when escaping JavaScript Date values
@mikesamuel
Copy link
Author

It looks like tests pass for node >= 6 and fail for node < 6. This is because of ES6 language features used. Working around those to be ES3.1-compatible shouldn't be a problem if this looks largely good.

@dougwilson
Copy link
Member

Hi @mikesamuel makes sense. Yes, ideally if this can work on all Node.js versions, that would be best, because we strive to support as many Node.js versions as possible since this is a very low-level driver module (as compared to a higher level ORM, etc.).

I would, though, say that the actual formatting code should not live in this module, otherwise it cannot be shared with the other MySQL modules like "mysql2" etc. The module https://github.com/mysqljs/sqlstring is dedicated to hosting the code for formatting.

@mikesamuel
Copy link
Author

Ok. So to move this forward, I could move Template.js and its tests into a separate npm module.
Could that live under the github.com/mysqljs org?

On failing gracefully on older node runtimes, I could define a utility file:

var calledAsTemplateTagQuick = (function () {
  try {
    return require('template-tag-common').calledAsTemplateTagQuick;
  } catch (ignored) {
    // Occurs if an ES6 parser is unavailable which is true for Node <= 6
    // String templates are not available there either, but might mismatch
    // if the calling code is transpiled, but mysql and its deps aren't.
    return function () { return false; };
  }
}());

If we don't attempt to load the interpolation handling code in branches that
don't pass that function, we should never observe that.

Test code would have to do an explicit version test when deciding whether to skip
tests and wrap template tag uses inside new Function("...").

@dougwilson
Copy link
Member

Why not into the sqlstring module?

@mikesamuel
Copy link
Author

Why not into the sqlstring module?

Sorry, didn't read closely enough. Will do.

mikesamuel added a commit to mikesamuel/sqlstring that referenced this pull request Jan 26, 2018
https://nodesecroadmap.fyi/chapter-7/query-langs.html describes
this approach as part of a larger discussion about library support
for safe coding practices.

This is one step in a larger effort to enable

connection.query`SELECT * FROM T WHERE x = ${x}, y = ${y}, z = ${z}`(callback)

and similar idioms.

This was broken out of mysqljs/mysql#1926
@mikesamuel
Copy link
Author

@dougwilson I filed mysqljs/sqlstring#29 there. If you want to close this, I can see what happens there and then come up with a new PR here, or if you want to leave this open, I can layer another commit on top once I'm ready to integrate a new version of sqlstring.

mikesamuel added a commit to mikesamuel/sqlstring that referenced this pull request Mar 7, 2018
https://nodesecroadmap.fyi/chapter-7/query-langs.html describes
this approach as part of a larger discussion about library support
for safe coding practices.

This is one step in a larger effort to enable

connection.query`SELECT * FROM T WHERE x = ${x}, y = ${y}, z = ${z}`(callback)

and similar idioms.

This was broken out of mysqljs/mysql#1926
@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 946727b to 37fbbdd Compare November 18, 2018 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants