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

Handle sql comments inside the sql tagged template literal #107

Closed
paulovieira opened this issue Oct 3, 2019 · 4 comments
Closed

Handle sql comments inside the sql tagged template literal #107

paulovieira opened this issue Oct 3, 2019 · 4 comments

Comments

@paulovieira
Copy link
Contributor

Living up to the "Promotes writing raw SQL" motto, I was wondering how can we use sql comments (that is, -- a comment and /* a comment */) inside this library sql tagged template literal.

More concretely, using a simple table like

create table t_dummy(id int, name text, age int)

sometimes I would like to be able to turn this

let id = 1, name = 'abc', age = 50;
let query = sql`

INSERT INTO t_dummy(
  id
  ,name
  ,age
)
VALUES(
  ${id}
  ,${name}
  ,${age}
)

`;

await connection.query(query);

into this:

let id = 1, name = 'abc', age = 50;
let query = sql`

INSERT INTO t_dummy(
  id
  ,name
--  ,age
)
VALUES(
  ${id}
  ,${name}
--  ,${age}
)

`;

await connection.query(query);

The second js snippet above will fail because the query object created by slonik will still create 3 value bindings ($1, $2, $3):

{
    "sql": "\n\nINSERT INTO t_dummy(\n  id\n  ,name\n  --,age\n)\nVALUES(\n  $1\n  ,$2\n  ,$3\n)\n\n",
    "type": "SLONIK_TOKEN_SQL",
    "values": [
        1,
        "abc",
        50
    ]
}

However it will work if we delete the sql comment line in VALUES. In that case we get:

{
    "sql": "\n\nINSERT INTO t_dummy(\n  id\n  ,name\n  --,age\n)\nVALUES(\n  $1\n  ,$2\n\n)\n\n",
    "type": "SLONIK_TOKEN_SQL",
    "values": [
        1,
        "abc"
    ]
}

So the multiline javascript string given to the sql tagged template would have to be pre-processed somehow to have sql comments removed.

Would this be a useful addition to slonik? If so I'd like to contribute.

@gajus
Copy link
Owner

gajus commented Oct 8, 2019

I cannot think of a straightforward way to implement this.

If you are available, give it a go – it would be a valuable contribution.

@vitaly-t
Copy link

vitaly-t commented Oct 9, 2019

This is why within pg-promise I use another module that I wrote - pg-minify, that shrinks SQL, while removing all comments, so that such things would not happen.

I've said it before, and can only repeat it that any serious database module needs proper use of external SQL files. Writing gigantic SQL inside code is a terrible idea. Not only it is unmanageable that way, but you cannot even handle comments like that, or join multiple SQL statements independently.

@gajus
Copy link
Owner

gajus commented Oct 10, 2019

Eliminating the comments once you have the entire SQL is not the problem. The challenge is removing the associated values after they were bound. We cannot remove values before the query is ready to execute because there are no guarantees that the entire token is present, i.e. unsafe to make assumptions about start/ end of a comment.

@gajus gajus closed this as completed in 8c505b2 Aug 6, 2021
@gajus
Copy link
Owner

gajus commented Aug 9, 2021

🎉 This issue has been resolved in version 24.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants