Improved append(...) and nested statements #104
Conversation
Codecov Report
@@ Coverage Diff @@
## master #104 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 36 62 +26
Branches 6 12 +6
=====================================
+ Hits 36 62 +26
|
WebReflection
force-pushed
the
master
branch
2 times, most recently
from
1f9b4df to
a594bfe
Compare
index.js
Outdated
| const current = tpl[i] | ||
| const value = val[i - 1] | ||
| if (/^('|")/.test(current) && RegExp.$1 === prev.slice(-1)) { | ||
| strings[j] = [strings[j].slice(0, -1), value, current.slice(1)].join('`') |
There was a problem hiding this comment.
This doesn't seem to escape the value (i.e. escape backticks inside the string). I also don't think it's easily possible because different dialects have different special chars.
There was a problem hiding this comment.
"I challenge you" finding anywhere someone using a name with a backtick in it for a table or a field name.
The being said, how about we make them configurable? backtick by default, setEscape('"') to change it as double quotes.
The escaping of the symbol is unnecessary at this point, nobody will have issues since this is a new feature, and if they want to use back ticks for table or field names, they are in charge of passing these already escaped.
YAGNI
There was a problem hiding this comment.
It's not about having table names with backticks or not, it's about security. If people expect this to escape the value then they might pass strings derived from user input. If it's not escaped properly, that would allow SQL injection.
There was a problem hiding this comment.
OK, current version exposes a method to define quotes:
SQL.withQuote('`')I could use that and fallback with a default based on back-ticks, or, if no quote is defined, simply throw an error.
I can escape automatically within the quotes, as long as escaping works with backslashes in every SQL implementation.
Is that the case?
There was a problem hiding this comment.
current version has a test too, with auto escape and error on failure
There was a problem hiding this comment.
off topic: I think SQL.withQuotes would feel more natural to devs ... since I usually refer to quotes in plural 🤔
There was a problem hiding this comment.
SQL.withQuotes it is 😅
WebReflection
force-pushed
the
master
branch
6 times, most recently
from
b3981f1 to
d9b1d42
Compare
This MR addresses all concerns and optimizations reised in felixfbecker#30 and felixfbecker#44 * SQLStatement can be used as value * raw names can be passed via `'${'table_' + name}'` with `'` or `"` transform * `.append(...)` doesn't need a space at the beginning, it's handled automatically
This MR addresses all concerns and optimizations reised in #30 and #44
'${'table_' + name}'with'or"transform.append(...)doesn't need a space at the beginning, it's handled automatically