groovy

Sql DataSet fails to work with non-literals in queries (fix error message/doco)

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 2.0-beta-3
  • Fix Version/s: 2.0-beta-3, 1.8.7
  • Component/s: None
  • Labels:
    None
  • Number of attachments :
    0

Description

All the examples of using findAll in the Sql DataSet class use literals for the search values of queries. Using free variables causes failure as Groovy does not implement lexical closure automatically. However this can be realized using the Closure delegate field. I therefore believe that the following example fails because the Sql.SqlWhereVisitor fails to lookup variables but assumes that all query values are literals.

import groovy.sql.DataSet
import groovy.sql.Sql

@Grapes ( [    
            @Grab ( 'org.xerial:sqlite-jdbc:3.7.2' ),
            @GrabConfig ( systemClassLoader = true )
          ] )
def database
final words = [ ]
try {
  database = Sql.newInstance ( 'jdbc:sqlite:database.db' , 'org.sqlite.JDBC')
  final wordsTable = new DataSet ( database , 'words' )
  ( 0 ..< 4 ).each { i ->
    final query = { item -> item.id == i }
    query.delegate = { i : i }
    query.resolveStrategy = Closure.DELEGATE_FIRST 
    words << wordsTable.findAll ( query ).firstRow ( ).word
  }
}
finally {
  database?.close ( )
}
println words.join ( '' )

Issue Links

relates to

New Feature - A new feature of the product, which has yet to be developed. GROOVY-5373 Sql DataSet fails to work with non-literals in queries (enhancement required)

  • Major - Major loss of function.
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Paul King added a comment -

Just a small aside: You don't need the @Grapes annotation in your example above - just the @Grab and @GrabConfig should suffice.

Show
Paul King added a comment - Just a small aside: You don't need the @Grapes annotation in your example above - just the @Grab and @GrabConfig should suffice.
Hide
Permalink
Paul King added a comment -

I think this issue conflates two aspects. It seems to me that I can "resolve" the issue by detecting when non-literals are used in the right-hand side of DataSet findAll statements and generate an appropriate warning indicating that such expressions aren't supported. If you are trying to solve the bigger issue about Closures, then I would rename the issue and give other simpler examples which illustrate the point. Given that DataSet has limitations, it doesn't really help much as an example in this discussion.

Show
Paul King added a comment - I think this issue conflates two aspects. It seems to me that I can "resolve" the issue by detecting when non-literals are used in the right-hand side of DataSet findAll statements and generate an appropriate warning indicating that such expressions aren't supported. If you are trying to solve the bigger issue about Closures, then I would rename the issue and give other simpler examples which illustrate the point. Given that DataSet has limitations, it doesn't really help much as an example in this discussion.
Hide
Permalink
Russel Winder added a comment -

So when is @Grapes needed?

Show
Russel Winder added a comment - So when is @Grapes needed?
Hide
Permalink
Russel Winder added a comment -

I am not sure right-hand and left-hand is important here – "it.id == x" and "x == it.id" should behave the same.

I think the above example is independent of the general Closure debate, since it provides a delegate that should resolve the "free variable". I think the point here is that the findAll evaluator doesn't do resolution at all, it assumes that the value is a literal. Isn't this just wrong?

Show
Russel Winder added a comment - I am not sure right-hand and left-hand is important here – "it.id == x" and "x == it.id" should behave the same. I think the above example is independent of the general Closure debate, since it provides a delegate that should resolve the "free variable". I think the point here is that the findAll evaluator doesn't do resolution at all, it assumes that the value is a literal. Isn't this just wrong?
Hide
Permalink
Paul King added a comment -

Re: "@Grapes": I would like to say @Grapes is never needed now but that isn't quite true. There are two reasons for its existence.

One is to do with JVM limitations regarding having multiple Grab annotations of the same name, e.g. in java.lang.Class#getAnnotation(Class) a single annotation is returned. We have worked around that limitation for now.

The second reason is to do with eventually supporting fine-grained control over Grab settings, e.g. you might want GrabResolver or GrabConfig to apply to just one of your Grab's not all of them. This is where Grapes would come in, however at the moment, all of those settings are global across all Grab's.

Show
Paul King added a comment - Re: " @Grapes ": I would like to say @Grapes is never needed now but that isn't quite true. There are two reasons for its existence. One is to do with JVM limitations regarding having multiple Grab annotations of the same name, e.g. in java.lang.Class#getAnnotation(Class) a single annotation is returned. We have worked around that limitation for now. The second reason is to do with eventually supporting fine-grained control over Grab settings, e.g. you might want GrabResolver or GrabConfig to apply to just one of your Grab 's not all of them. This is where Grapes would come in, however at the moment, all of those settings are global across all Grab 's.
Hide
Permalink
blackdrag blackdrag added a comment -

As for the query. Russel, how we construct this atm is by getting the AST from source for this, evaluate the ast to a sql query and execute that query. This translation cannot and will probably never handle complex expressions. The case at hand here though is about a local variable being used. And such a case should be supported. But that doesn't make this a critical bug, but a RFE. If anything here is a bug, than that unhandled cases are silently accepted. imho that should cause an exception.

Show
blackdrag blackdrag added a comment - As for the query. Russel, how we construct this atm is by getting the AST from source for this, evaluate the ast to a sql query and execute that query. This translation cannot and will probably never handle complex expressions. The case at hand here though is about a local variable being used. And such a case should be supported. But that doesn't make this a critical bug, but a RFE. If anything here is a bug, than that unhandled cases are silently accepted. imho that should cause an exception.
Hide
Permalink
Paul King added a comment -

Re: "right-hand vs left-hand":

You are correct, it shouldn't be important though the current implementation makes a distinction - don't have time to try it tonight - I suspect it may just produce an expression with an unneeded set of brackets in one case.

Re: "I think the point here is that the findAll evaluator doesn't do resolution at all, it assumes that the value is a literal. Isn't this just wrong?"

I guess I am just saying "wrong" is in the eye of the beholder. Groovy can provide a limited functionality DataSet if it wants. It will probably still cover many of the typical queries that people do but obviously not all. What we currently have though is a limited functionality mechanism with some secondary issues (poor error messages and inadequate documentation). I tend to agree with you that we do want to fix this properly but am reminding myself that perhaps removing the secondary issues in the meantime might also be a useful step forward.

Show
Paul King added a comment - Re: "right-hand vs left-hand": You are correct, it shouldn't be important though the current implementation makes a distinction - don't have time to try it tonight - I suspect it may just produce an expression with an unneeded set of brackets in one case. Re: "I think the point here is that the findAll evaluator doesn't do resolution at all, it assumes that the value is a literal. Isn't this just wrong?" I guess I am just saying "wrong" is in the eye of the beholder. Groovy can provide a limited functionality DataSet if it wants. It will probably still cover many of the typical queries that people do but obviously not all. What we currently have though is a limited functionality mechanism with some secondary issues (poor error messages and inadequate documentation). I tend to agree with you that we do want to fix this properly but am reminding myself that perhaps removing the secondary issues in the meantime might also be a useful step forward.
Hide
Permalink
Paul King added a comment -

OK, just confirmed my suspicions, there is a slight different treatment of left- and right-hand sided literals but it makes no practical difference. Here is an example:

def found1 = projects.findAll{ it.id < 35 && it.id > 15 }
println found1.sql // => select * from project where (id < ? and id > ?)
def found2 = projects.findAll{ 35 > it.id && it.id > 15 }
println found2.sql // => select * from project where ((? > id) and id > ?)

So we can remove this difference in treatment if we want.

Show
Paul King added a comment - OK, just confirmed my suspicions, there is a slight different treatment of left- and right-hand sided literals but it makes no practical difference. Here is an example: def found1 = projects.findAll{ it.id < 35 && it.id > 15 } println found1.sql // => select * from project where (id < ? and id > ?) def found2 = projects.findAll{ 35 > it.id && it.id > 15 } println found2.sql // => select * from project where ((? > id) and id > ?) So we can remove this difference in treatment if we want.
Hide
Permalink
Paul King added a comment -

Issue cloned and split out into "fix current error message/doco" and "do the enhancement".

Show
Paul King added a comment - Issue cloned and split out into "fix current error message/doco" and "do the enhancement".
Hide
Permalink
Paul King added a comment -

Currently, DataSet produces invalid Sql and silently continues on resulting in the user getting a strange error message at some point in the future. If we currently don't support particular expressions which cause such behaviour, we should produce a suitable error message at the point where the unsupported expression is discovered.

Show
Paul King added a comment - Currently, DataSet produces invalid Sql and silently continues on resulting in the user getting a strange error message at some point in the future. If we currently don't support particular expressions which cause such behaviour, we should produce a suitable error message at the point where the unsupported expression is discovered.
Hide
Permalink
Paul King added a comment -

Error message added and doco tweaked. Please continue discussions about the desired DataSet enhancement on GROOVY-5373. Thanks.

Show
Paul King added a comment - Error message added and doco tweaked. Please continue discussions about the desired DataSet enhancement on GROOVY-5373 . Thanks.
Hide
Permalink
Russel Winder added a comment -

The error message works for me.

Show
Russel Winder added a comment - The error message works for me.
Hide
Permalink
Russel Winder added a comment -

Seems to work as anticipated.

Show
Russel Winder added a comment - Seems to work as anticipated.

People

  • Assignee:
    Paul King
    Reporter:
    Russel Winder
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: