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

added pg functions keyword and identifier, updated README #2

Closed
wants to merge 2 commits into from

Conversation

fritzy
Copy link

@fritzy fritzy commented Sep 29, 2015

Not sure if you want the PG statements set in there, or how you wanted to deal with different dialects, but this is one way to do it. I wanted to be able to enforce some user input filtering on identifiers and keywords.

@fritzy
Copy link
Author

fritzy commented Sep 29, 2015

@nlf fixed that and replaced function in the examples (were erroneously keyword when they should have been identifier)

@felixfbecker
Copy link
Owner

Thanks for the PR, I don't know how I should feel about it though. The purpose of this module was to take a template string and transform it into an object with placeholder queries and values that both pg and mysql understand, and let them do the escaping. You can use SQL.raw() when the input can be trusted or use the database driver escaping functions (that's why I added an example in readme with mysql.escapeId()). pg provides something similar: brianc/node-postgres#396 (maybe I should add an example with these too). Adding specific pq functions kind of breaks with the dual-compatibility. I guess it is also generally better to validate stricter - i.e. if you insert a sort direction, check for asc or desc.
Do you have a specific use case?

@fritzy
Copy link
Author

fritzy commented Sep 29, 2015

Ah, I didn't realize the pg client had these already; these seem to be undocumented features.

I have to say that I don't like that it's bothering to produce 2 strings when all I need is one, but otherwise this little module is great.

@fritzy fritzy closed this Sep 29, 2015
@felixfbecker
Copy link
Owner

Yeah, I only found them by searching in the repo. Maybe someone should open a PR to add this to their docs?
And what do you think about adding an option to explicitly set the driver?

@fritzy
Copy link
Author

fritzy commented Sep 29, 2015

Yup, that's why I opened issue #3

@fritzy
Copy link
Author

fritzy commented Sep 29, 2015

@felixfbecker I was trying to avoid this:

SQL`... ORDER BY ${SQL.raw(pg.Client.prototype.escapeIdentifier(args.orderBy))} ${SQL.raw(validSort.has(args.sort.toUpperCase()) ? args.sort : 'DESC')}`

@nlf
Copy link
Contributor

nlf commented Sep 29, 2015

what if we just allow passing a second parameter to SQL.raw that can be an optional escaping function? then it would be more like

let escape = pg.Client.prototype.escapeIdentifier;
let query = SQL`... ORDER BY ${SQL.raw(args.orderBy, escape)}`

@nlf
Copy link
Contributor

nlf commented Sep 29, 2015

though i'm not sure that's really much better than just doing

let escape = pg.Client.prototype.escapeIdentifier;
let query = SQL`... ORDER BY ${SQL.raw(escape(args.orderBy))}`

maybe just a little easier to read since it has fewer parens

@felixfbecker
Copy link
Owner

Yeah, it's unfortunate that the escape functions sits on the client prototype. I would suggest doing

let escape = pg.Client.prototype.escapeIdentifier

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 this pull request may close these issues.

3 participants