groovy
  1. groovy
  2. GROOVY-5371

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

          Activity

          Hide
          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
          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
          Russel Winder added a comment -

          So when is @Grapes needed?

          Show
          Russel Winder added a comment - So when is @Grapes needed?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Russel Winder added a comment -

          The error message works for me.

          Show
          Russel Winder added a comment - The error message works for me.
          Hide
          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
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: