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

Rewrite named-placeholders to support template function #4

Open
gajus opened this issue May 8, 2016 · 3 comments
Open

Rewrite named-placeholders to support template function #4

gajus opened this issue May 8, 2016 · 3 comments

Comments

@gajus
Copy link

gajus commented May 8, 2016

The current implementation of named-placeholders is flawed. It is assumes that parameter values is known in advance, i.e. the only possible use case is:

var mysql = require('mysql');
var toUnnamed = require('named-placeholders')();

var q = toUnnamed('select 1+:test', { test: 123});
mysl.createConnection().query(q[0], q[1]);

As a result, it is not possible to implement prepared statements using this implementation.

My suggestion is to reimplement named-placeholders API as:

const {
    parser,
    mapValues
} = require('named-placeholders');

// Used to control LRU.
const parseNamedPlaceholder = parser();

const inputQuery = 'SELECT :foo, :foo, :bar';

const queryTemplate = parseNamedPlaceholder(inputQuery);
// {
//     statement: 'SELECT ?, ?, ?',
//     parameterMap: {
//         0: 'foo',
//         1: 'foo',
//         2: 'bar'
//     }
// }

mapValues(queryTemplate.parameterMap, {
    foo: 'A',
    bar: 'B'
});
// {
//     0: 'A',
//     1: 'A',
//     2: 'B'
// }
@gajus
Copy link
Author

gajus commented May 9, 2016

@sidorares Thoughts?

@tomchiverton
Copy link

Progress ? Seems a fairly basic part of be missing from a key popular database implementation ?

@sidorares
Copy link
Member

@sidorares Thoughts?

@gajus sorry just found time to look at this. I really like your api, need to find a safe way to transition. It's quite likely mysql / mysql2 are the only consumers, so maybe major version + incompatible api change is enough

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

No branches or pull requests

3 participants