4 ms·

Hey nice I actually looked at this library when I started. I really like the sql identifier escaping, where most just do literals. I'd thought about: "select

by qooleot 12y ago

Hey nice I actually looked at this library when I started. I really like the sql identifier escaping, where most just do literals. I'd thought about: "select * from %I${my_table} where ..." I think I'd probably feel more comfortable having a whitelist of allowed words rather than just a blacklist, as there are probably ways around it. Either new syntaxes in future Postgres versions, or mid-sql comments. Take this for example: Edit: sorry the * before the word "comment" below is being escaped by hacker news. There is probably some hackernews-injection to get around this:} select/comment/ * from foo; if "select/comment/" was the identifier, that's valid sql, but: function isReserved(value) { if (reservedMap[value.toUpperCase()]) { does a comparison on an exact match, not contains. https://github.com/datalanche/node-pg-format/blob/master/lib/index.js https://github.com/datalanche/node-pg-format/blob/master/lib...

rpedela 12y ago
I like the whitelist idea. If you have time, maybe you could create an issue or PR which goes into more detail on your thoughts. As for your specific example, I am not sure I understand the issue. The SQL keyword list (reserved list) is only used to determine if the identifier needs to be quoted. So if your identifier is 'select', the library will quote that because you can use SQL keywords as identifiers as long as they are quoted. Your specific example produces this: JS: format('%I * from foo;', 'select/comment/') SQL: "select/comment/" * from foo; Alternatively I could add an option to quote all identifiers regardless. I might do that. :) EDIT: I didn't see your comment edit before responding. I would appreciate it you created a Github issue where you can get the SQL formatting right. If the library is doing something wrong, I want to fix it.
- qooleot 12y ago
  An example of extending your reserved list would be the world "lateral" (new type of join in 9.4). I'll do a PR if I get that far:} RE: comments, yes it would be quoted and that would cause an error. select/comment/ 1; ?column? ---------- 1 (1 row) # "select/comment/" 1; ERROR: syntax error at or near ""select/comment/"" LINE 1: "select/comment/" 1; That's probably a silly example, and I was just trying to think through 'what could go wrong with this?' before implementing. Thinking about it further, thats probably not something you want the library to be responsible for handling. I'd want to try out newline chars too since thats more common.
  - rpedela 12y ago
    Yeah I do need to go through the list of supported SQL keywords for 9.4 and update the library. Thanks for the reminder. The main goal of the library is to behave like Postgres format() plus some JS sugar like handling an array of identifiers. If the library behaviour deviates from Postgres format() then it is a bug. BTW I personally like the %I${var} syntax you mentioned earlier.

by qooleot 12y ago

rpedela 12y ago

I like the whitelist idea. If you have time, maybe you could create an issue or PR which goes into more detail on your thoughts. As for your specific example, I am not sure I understand the issue. The SQL keyword list (reserved list) is only used to determine if the identifier needs to be quoted. So if your identifier is 'select', the library will quote that because you can use SQL keywords as identifiers as long as they are quoted. Your specific example produces this: JS: format('%I * from foo;', 'select/comment/') SQL: "select/comment/" * from foo; Alternatively I could add an option to quote all identifiers regardless. I might do that. :) EDIT: I didn't see your comment edit before responding. I would appreciate it you created a Github issue where you can get the SQL formatting right. If the library is doing something wrong, I want to fix it.

qooleot 12y ago
An example of extending your reserved list would be the world "lateral" (new type of join in 9.4). I'll do a PR if I get that far:} RE: comments, yes it would be quoted and that would cause an error. select/comment/ 1; ?column? ---------- 1 (1 row) # "select/comment/" 1; ERROR: syntax error at or near ""select/comment/"" LINE 1: "select/comment/" 1; That's probably a silly example, and I was just trying to think through 'what could go wrong with this?' before implementing. Thinking about it further, thats probably not something you want the library to be responsible for handling. I'd want to try out newline chars too since thats more common.
- rpedela 12y ago
  Yeah I do need to go through the list of supported SQL keywords for 9.4 and update the library. Thanks for the reminder. The main goal of the library is to behave like Postgres format() plus some JS sugar like handling an array of identifiers. If the library behaviour deviates from Postgres format() then it is a bug. BTW I personally like the %I${var} syntax you mentioned earlier.