CSNW / sql-bricks

Transparent, Schemaless SQL Generation
http://csnw.github.io/sql-bricks
MIT License
203 stars 25 forks source link

`sql()` overeagerly converts dollar characters into parameter placeholders #96

Closed sneakertack closed 7 years ago

sneakertack commented 7 years ago

@Suor (as the issue was discovered in the process of using PostgreSQL/sql-bricks-postgres - not sure where a fix if any ought to be applied)

sql(), recommended as a fallback for additional Postgres operations not covered by sql-bricks-postgres, will mangle values where an actual dollar character is desired.

In my case, I wanted to construct queries that do POSIX regular expression matching. The Postgres syntax looks something like:

SELECT * FROM table1 WHERE col1 ~* '.*someregex\d+$' (note the use of $ as an end-of-line anchor in the regex)

In such a case, doing sql.select().from('table1').where(sql("col1 ~* '.*someregex\d+$'")) will replace $ with $1 (in both toString() and toParams()).

Is there any way to escape the $-character to prevent it from being treated as a parameter placeholder? I looked at https://github.com/CSNW/sql-bricks/blob/master/sql-bricks.js#L69 and that line suggests that there isn't one at the moment.

sneakertack commented 7 years ago

I've since realized that a proper way to achieve the above could be to do sql.select().from('table1').where(sql("col1 ~* $", '.*someregex\d+$'))

It could help to have the features implemented by #61 and #74 explicitly described in the doc pages though... would it be worth updating them? I don't mind writing a PR with documentation updates, but the gh-pages branch seems to have been untouched in a while, so I'm not entirely sure about the docs' current status.

prust commented 7 years ago

@sneakertack:

the gh-pages branch seems to have been untouched in a while, so I'm not entirely sure about the docs' current status.

You're right, the gh-pages branch wasn't updated in 2016, but there were doc fixes in 2016. I just now updated it to the latest (2.0.2).

I don't mind writing a PR with documentation updates

That would be much appreciated.

prust commented 7 years ago

@sneakertack: As mentioned above, there were missing docs added in 2016 (180314be) that needed to be pushed to gh-pages -- that was done on June 30.

Today I added some more documentation in 46e5c7b89 and 45eef1c744, so I think this can be closed.

Regarding the request for a way to escape the '$' character... I suppose we could make '\$' and '\?' be an escape, via adding [^\\] to the beginning of all the regexes in toString()... but that would disallow a placeholder as the very first character passed to sql(). We might be able to allow start-of-string OR non-escape-character via ^|[^\\] -- but I haven't tested that to verify that it works. At this point, I'm hesitant to add the complexity unless there are more requests demonstrating the need for it.

Suor commented 7 years ago

@prust the usual thing to use is double char like $$ here not \$. In case if you ever going to implement this)

sneakertack commented 7 years ago

Actually $$ opens up another can of worms, because sometimes strings might be represented as $$a string$$ in Postgres :grimacing:. Maybe a 3rd option we could not-implement would be to preserve dollar-literals encountered within quotes.

But yes I no longer have that strong a need for the escaping originally raised, especially not if it makes things more difficult - after all the "workaround" of parametrizing your query and moving $ or ?literals into the parametrized value makes for a safer query.