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

Infix Query Columns need to match object or error occurs #1547

Open deusaquilus opened 5 years ago

deusaquilus commented 5 years ago

Version: (e.g. 3.4.2-SNAPSHOT) Module: (e.g. quill-jdbc) Database: (e.g. ALL)

Expected behavior

val ctx = new io.getquill.PostgresJdbcContext[SnakeCase](SnakeCase, ds)

// SELECT x._1, x._2 FROM (select table_name, table_schema from information_schema.tables) AS x
ctx.run(infix"select table_name, table_schema from information_schema.tables".as[Query[(String, String)]]).foreach(println(_))

This should correctly select and de-serialize tuples of the information_schema.

Note that the same problem occurs when you use a case class:

case class TwoValue(firstValue:String, secondValue:String)

ctx.run(infix"select table_name, table_schema from information_schema.tables".as[Query[TwoValue]]).foreach(println(_))
// SELECT x.first_value, x.second_value FROM (select table_name, table_schema from information_schema.tables) AS x

Actual behavior

The following query is produced:

SELECT x._1, x._2 FROM (select table_name, table_schema from information_schema.tables) AS x

For the case-class scenario:

SELECT x.first_value, x.second_value FROM (select table_name, table_schema from information_schema.tables) AS x

Quill which of course returns in an error:

org.postgresql.util.PSQLException: ERROR: column x._1 does not exist
  Position: 8
  org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2468)
  org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2211)
  org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:309)
  org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:446)
  org.postgresql.jdbc.PgStatement.execute(PgStatement.java:370)

... and the case class scenario:

org.postgresql.util.PSQLException: ERROR: window function first_value requires an OVER clause
  Position: 8
  org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2468)
  org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2211)
  org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:309)
  org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:446)
  org.postgresql.jdbc.PgStatement.execute(PgStatement.java:370)

Workaround

You need to label your columns in the inner query:

ctx.run(infix"select table_name as _1, table_schema as _2 from information_schema.tables".as[Query[(String, String)]]).foreach(println(_))

@getquill/maintainers

mdedetrich commented 5 years ago

So I have just hit this when converting from Doobie to Quill and using raw SQL as described here https://getquill.io/#extending-quill-infix-raw-sql-queries

What I don't understand is why is Quill even doing nested queries with x. In Doobie you can do this

sql"""SELECT * FROM user""".query[User]

And as long as the columns that you get from the result set is in the same order as the case class it works. If you are doing raw SQL you are in no mans land anyways

If we don't want to do this (i.e. we are selecting by field names in the SQL and matching those field names against the case class) we then have problems if you have custom prefix's. Normally you would define a schemaMeta[User] for this however infix appears to completely ignore it.

So in summary.

Selecting by case class

When using infix and mapping it to a case class, i.e. .as[User] this will break if you have a custom naming convention on field names, using a schemaMeta[User] will not fix this. You can theoretically use a custom NamingStrategy however this would get unwieldy pretty fast. Pretty sure this is a case of https://github.com/getquill/quill/issues/766

Selecting by tuple

When using infix and mapping it to a tuple, i.e. .as[String, String] assuming that User has 2 fields that are strings, we should just completely ignore and kind of nesting whatsoever. The quill docs state

Note that in this case the result query is nested. It’s required since Quill is not aware of a query tree and cannot safely unnest it. This is different from the example above because infix starts with the query infix"$q... where its tree is already compiled

I am not sure what this "cannot safely unnest" means, we are writing raw SQL here. There is no need for quill to inspect the query to figure out what is going on, the only thing it needs to take care of is interpolating $ values. Quill doesn't actually need to inspect the infix string at all, since in order to run an infix query we have to prescribe its return value with .as, i.e.

infix"...".as[Query[User]] // We know this is going to be select for a user
infix"...".as[Query[One, Two]] // We know this is going to match any select for any N number of fields as long as the types match up
infix"...".as[Insert[User]] //inserting some rows
infix"...".as[Delete[User]] //Deleting some rows
infix"...".as[Long] // Handle cases with a single row such as `COUNT *`
infix"...".as[ActionReturning[To,From]]` // We know this is going to be an insert/update on some table `To` returning a set of fields designated by `From`. We can even use tuples for `To`/`From`
mdedetrich commented 5 years ago

as @deusaquilus pointed out in gitter, there are cases where we need to do this nesting, i.e. if you mix raw infix and then do something like .take afterwards, i.e.

infix"select foo, bar from baz".as[Query[(Int, String)]].distinct.take(1)

A third option to advocate for is to introduce another way of defining SQL queries which really are raw and you can't compose on them using the quill dsl (i.e. basically the same as what Doobie does, you give it raw SQL and it doesn't do any magic).

Something like

sql"select foo, bar from baz".as[Query[(Int, String)]]

or

infix"select foo, bar from baz".raw.as[Query[(Int, String)]]

In this case, quill will not do any nesting at all. Its your responsibility that the columns match up (in cases of using tuple this will mean ordering, in cases of using case class this means field names will match).

In the short term, I think focusing on https://github.com/getquill/quill/issues/766 would be beneficial as it would mean that at least the case class variant would work in all cases.