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

Cleaner approach possible for append? #44

Open
rightaway opened this issue Jul 11, 2017 · 12 comments
Open

Cleaner approach possible for append? #44

rightaway opened this issue Jul 11, 2017 · 12 comments

Comments

@rightaway
Copy link

If I want to use append because I want to have a dynamic column name (e.g. othercolumn1, othercolumn2), it looks like this.

    return sqlTemplateString`
      update thetable set
        thecolumn1 = null,`
      .append(`othercolumn${version}`)
      .append(sql` = null
      where
        id = ${id}
    `

Is it possible to write it somehow like below? I find it looks cleaner for multi-line statements. Would the sqlvalue function I've used below make a good feature request, if it's even possible to work that way?

    return sqlTemplateString`
      update thetable set
        thecolumn1 = null,
        othercolumn${sqlvalue(version)} = null
      where
        id = ${id}
    `
@felixfbecker
Copy link
Owner

This was actually the API in 1.x, you had to use SQL.raw()

@rightaway
Copy link
Author

So instead of ${sqlvalue(version)} in my example above, ${SQL.raw(version)} would work?

Would you consider adding this syntax back to the 2.x API? I use mostly multi-line statements and SQL.raw would really help to preserve the indentation and make for easier to read statements, especially for larger or more complex ones.

@felixfbecker
Copy link
Owner

Hmm, apparently it would have complicated the implementation: https://github.com/felixfbecker/node-sql-template-strings/releases/tag/v2.0.0

@felixfbecker
Copy link
Owner

Honest question, do you really think this is that bad?

query(SQL`
  UPDATE thetable
  SET thecolumn1 = null, othercolumn`.append(version).append(SQL` = null
  WHERE id = ${id}
`)

@rightaway
Copy link
Author

rightaway commented Jul 11, 2017

I think that append and SQL.raw have different ways they can contribute to making SQL statements that are quite clear to understand at a glance.

The example you used in the README is a perfect case for append, where the query is being built dynamically from various clauses (e.g. also in conditionals or loops). The append makes it quite readable, and I use it throughout my code in these cases.

const query = SQL`SELECT * FROM books`
if (params.name) {
  query.append(SQL` WHERE name = ${params.name}`)
}
query.append(SQL` LIMIT 10 OFFSET ${params.offset || 0}`)

SQL.raw however would be useful when substituting a small part of a query within a clause, like a table or column name, or part of it (like the version example I used).

query(SQL`
  UPDATE thetable
  SET thecolumn1 = null, othercolumn${SQL.raw(version)} = null
  WHERE id = ${id}
`)

In these cases, I think readability suffers when using append instead of SQL.raw as I try to piece the query together in my mind.

It's the same way why when building strings, the first (contrived) example below is more readable than the second.

`person ${name} is ${age} years old and lives in ${country}`
'person '.concat(name).concat(' is ').concat(age).concat(' years old and lives in ').concat(country)

If I were able to use only one of append or SQL.raw for all my queries, I think the readability and clarity of the queries would be less than optimal, but having both would cover all cases I can think of (at least throughout the code I'm working on which is using sql-template-strings with hundreds of queries).

@rightaway
Copy link
Author

@felixfbecker Just wanted to follow up on this. It's still something that is a bit of a pain point in our projects and it would be great if the SQL.raw syntax were added back to the 2.x API, to get the benefits of clearer code I described in my post above.

@rightaway
Copy link
Author

rightaway commented Nov 9, 2017

@felixfbecker To give you an example of one of various inconveniences caused by the lack of SQL.raw, there's this query

SQL`update mytable set`
  .append(`column${version}`)
  .append(SQL` = now()
  where othercolumn = ${value}`

This fails because set and column get appended because there's no space in the middle, so the query starts update mytable setcolumn instead of update mytable set column.

One benefit of ES6 template strings was to remove errors like this in regular string concatenation. So 'before' + value + 'after' may look fine at first glance, but of course it becomes 'beforeVALUEafter'. Template strings solve this before ${value} after, which becomes 'before VALUE after' as expected because it's easy to see.

That query would be much clearer in my opinion if it were

SQL`update mytable set
  column${SQL.raw(version)} = now()
  where othercolumn = ${value}`;

@felixfbecker
Copy link
Owner

I reviewed #45

@skyjur
Copy link

skyjur commented Apr 29, 2018

I too think append is not pretty and goes against nature of this project: making sql query statements more readable. I think small downside of slightly more complex implementation required to support raw() ought-weights readability benefits by far.

Also I think nested queries would be very great addition, to completely remove need for .append():

master...skyjur:master

function authorFilter(author) {
  return SQL`author = ${author}`;
}
function nameFilter(name) {
  return SQL`name = ${name}`;
}
const query = SQL`SELECT author FROM books WHERE ${authorFilter(
  author
)} AND ${nameFilter(book)}`;
typeof query; // => 'object'
query.text; // => 'SELECT author FROM books WHERE name =  AND author = '
query.sql; // => 'SELECT author FROM books WHERE name = ? AND author = ?'
query.values; // => ['harry potter', 'J. K. Rowling']

WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
@WebReflection
Copy link
Contributor

@rightaway @skyjur

My current PR would allow the following transformation:

SQL`
  UPDATE thetable
  SET thecolumn1 = null, "${'othercolumn' + version}`" = null
  WHERE id = ${id}
`

will result into

UPDATE thetable
SET thecolumn1 = null, `othercolumn123` = null
WHERE id = ?

It will also accept nested statements:

function authorFilter(author) {
  return SQL`author = ${author}`;
}
function nameFilter(name) {
  return SQL`name = ${name}`;
}
SQL`SELECT author FROM books
  WHERE ${authorFilter(author)}
  AND ${nameFilter(book)}`;

aggregating automatically all the ? and the passed values.

Hope that works for this ticket too. 👋

WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
WebReflection pushed a commit to WebReflection/node-sql-template-strings that referenced this issue Mar 1, 2019
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
@msikma
Copy link

msikma commented Jul 13, 2019

I would also greatly welcome this change. It's the one point where this library makes SQL statements harder to read rather than easier. Unfortunately it seems there's been no progress on the PR #104 since March 1.

@ajwinkworth
Copy link

I also seem to have come here hoping to suggest/add something like

SQL`SELECT (${a(columnName}) FROM ${a(tableName)} WHERE firstName = ${name}`;

To find it did exist with SQL.raw() is slightly saddening.

let a = SQL.raw;
SQL`SELECT (${a(columnName}) FROM ${a(tableName)} WHERE firstName = ${name}`;

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

6 participants