zio / zio-quill

Compile-time Language Integrated Queries for Scala
https://zio.dev/zio-quill
Apache License 2.0
2.15k stars 348 forks source link

sql keywords are not sanitized #405

Closed cambridgemike closed 7 years ago

cambridgemike commented 8 years ago
  val parent_metric_group_q = quote {
    (metric: Metric) => {
      MetricGroup.schema.filter(group => metric.parentGroup == group.name)
    }
  }

caused a runtime MySQL error. It took quite a while to debug, but it turned out it was because the word 'group' was being used in the sql query. I can see why this happened now, but maybe a better error message or at least a warning in the docs could have been helpful.

Version: (e.g. 0.4.1-SNAPSHOT) 0.6.0 Module: (e.g. quill-jdbc)

Expected behavior

Actual behavior

Steps to reproduce the behavior

@getquill/maintainers

jilen commented 8 years ago

possibly a duplicate of #365

fwbrasil commented 8 years ago

@cambridgemike @mbardea have you tried using the naming strategy Escape as pointed out by @jilen? I've been thinking about this issue and I don't think we want to maintain a list of reserved words for each database dialect. Also, checking that the sql doesn't have reserved words would be expensive at run time for dynamic quotations.

Maybe we just need to improve the documentation on this topic?

jilen commented 8 years ago

@fwbrasil I have misunderstand this issue previously, it did no help to use Escape, because alias are not escaped currently

cambridgemike commented 8 years ago

Yeah I don't think it makes sense for quill to do any sanitizing. I also like that the output sql uses the variable names because it makes it easy to debug. It just felt like this was a gotcha so improved documentation would be very helpful.

On Monday, July 4, 2016, jilen notifications@github.com wrote:

@fwbrasil https://github.com/fwbrasil I have misunderstand this issue previously, it did no help to use Escape, because alias are not escaped currently

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/getquill/quill/issues/405#issuecomment-230367148, or mute the thread https://github.com/notifications/unsubscribe/AAJ-Zew7INswyEEWgAocYFozpwdVc6NBks5qSag1gaJpZM4I7Bcp .

megri commented 8 years ago

Maybe it would make sense to simply quote all names in the sql-query, much like other frameworks do?

In essence, for every flavour of SQL, assure there is a escapeName(…). MySQL uses backticks in mysql-mode, or double quotes in ANSI mode. PostgreSQL would probably just use double quotes.

Example: SELECT * FROM group becomes SELECT * FROM "group".

fdietze commented 7 years ago

I just ran into this same issue and it took a long time to debug. For me it was using a variable called user in an anonymous function for a JOIN. (PostgreSQL)

Any news on this?

fwbrasil commented 7 years ago

Issue is not valid anymore. The Escape naming strategy now escapes identifiers: https://scalafiddle.io/sf/lsmX57J/128

danieltoomey commented 6 years ago

Found this error where we used the alias 'user' for a table name. version 2.3.2

fdietze commented 6 years ago

I can confirm this.