[
https://issues.apache.org/jira/browse/GROOVY-5373?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18072826#comment-18072826
]
ASF GitHub Bot commented on GROOVY-5373:
----------------------------------------
Copilot commented on code in PR #2459:
URL: https://github.com/apache/groovy/pull/2459#discussion_r3067872697
##########
subprojects/groovy-sql/src/test/groovy/groovy/sql/PersonTest.groovy:
##########
@@ -78,13 +78,11 @@ order by firstName DESC, age'''
assertSql(complexBlogs, expectedSql, expectedParams)
}
- // GROOVY-5371 can be removed once GROOVY-5375 is completed and this is
supported
- void testNonLiteralExpressionsCurrentlyNotSupported() {
+ // GROOVY-5373: variables from enclosing scope are now supported
+ void testVariablesFromEnclosingScope() {
def cutoff = 10
- def message = shouldFail {
- people.findAll { it.size < cutoff && it.lastName == "Bloggs"
}.rows()
- }
- assert message.contains("DataSet currently doesn't support arbitrary
variables, only literals")
+ def blogs = people.findAll { it.size < cutoff && it.lastName ==
"Bloggs" }
+ assertSql(blogs, "select * from person where ((size < ?) and lastName
= ?)", [cutoff, 'Bloggs'])
}
Review Comment:
This new test validates basic captured-variable support, but it doesn’t
exercise the Reference-unwrapping branch in SqlWhereVisitor (used when the
captured variable is reassigned/shared). Consider extending this test (or
adding another) where the captured variable is reassigned after the DataSet is
created but before sql/parameters are accessed, to ensure the parameter value
is unwrapped correctly and remains stable.
```suggestion
void testVariablesFromEnclosingScopeAfterReassignment() {
def cutoff = 10
def blogs = people.findAll { it.size < cutoff && it.lastName ==
"Bloggs" }
cutoff = 20
assertSql(blogs, "select * from person where ((size < ?) and
lastName = ?)", [20, 'Bloggs'])
}
```
##########
subprojects/groovy-sql/src/main/java/groovy/sql/SqlWhereVisitor.java:
##########
@@ -81,9 +83,32 @@ public void visitPropertyExpression(PropertyExpression
expression) {
buffer.append(expression.getPropertyAsString());
}
+ public void setClosure(Closure<?> closure) {
+ this.closure = closure;
+ }
+
@Override
public void visitVariableExpression(VariableExpression expression) {
- throw new GroovyRuntimeException("DataSet currently doesn't support
arbitrary variables, only literals: found attempted reference to variable '" +
expression.getName() + "'");
+ // Try to resolve captured variables from the closure's context
+ if (closure != null) {
+ String name = expression.getName();
+ try {
+ java.lang.reflect.Field field =
closure.getClass().getDeclaredField(name);
+ field.setAccessible(true);
+ Object value = field.get(closure);
+ // Groovy wraps shared (mutable) variables in a Reference
+ if (value instanceof groovy.lang.Reference) {
+ value = ((groovy.lang.Reference<?>) value).get();
+ }
+ getParameters().add(value);
+ buffer.append("?");
+ return;
+ } catch (NoSuchFieldException | IllegalAccessException ignored) {
Review Comment:
visitVariableExpression() uses Field#setAccessible(true) but only catches
NoSuchFieldException/IllegalAccessException. On Java 9+ this can also throw
InaccessibleObjectException/SecurityException (e.g., when running with strong
encapsulation), which would leak out unexpectedly. Consider using
trySetAccessible() (or catching those runtime exceptions) and including the
reflection failure as a cause/suppressed exception in the
GroovyRuntimeException for easier debugging.
##########
subprojects/groovy-sql/src/main/java/groovy/sql/DataSet.java:
##########
@@ -414,6 +414,7 @@ public List<Object> getParameters() {
protected SqlWhereVisitor getSqlWhereVisitor() {
if (visitor == null) {
visitor = new SqlWhereVisitor();
+ visitor.setClosure(where);
visit(where, visitor);
}
Review Comment:
DataSet’s class-level Javadoc still states that where/sort expressions can
only reference POGO fields or literals. With the new support for captured
variables, that documentation is now partially outdated and may mislead users.
Please update the Javadoc to describe the current supported expression set
(literals + captured variables) and any remaining limitations.
> Sql DataSet fails to work with non-literals in queries (enhancement required)
> -----------------------------------------------------------------------------
>
> Key: GROOVY-5373
> URL: https://issues.apache.org/jira/browse/GROOVY-5373
> Project: Groovy
> Issue Type: New Feature
> Components: SQL processing
> Affects Versions: 2.0-beta-3, 2.1.4, 2.2.0-beta-1
> Reporter: Dr. Russel Winder
> Assignee: Paul King
> Priority: Major
>
> 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.
> {code}
> @Grab('org.xerial:sqlite-jdbc:3.7.2')
> @GrabConfig(systemClassLoader=true)
> import groovy.sql.DataSet
> import groovy.sql.Sql
> final words = []
> Sql.withInstance('jdbc:sqlite:database.db', 'org.sqlite.JDBC') {database ->
> final wordsTable = new DataSet(database, 'words')
> (0 ..< 4).each {i ->
> // This doesn't work as SqlWhereVisitor doesn't resolve variables it only
> looks for
> // literals. cf. http://jira.codehaus.org/browse/GROOVY-5373
> final query = {item -> item.id == i}
> query.delegate = {i: i}
> query.resolveStrategy = Closure.DELEGATE_FIRST
> words << wordsTable.findAll(query).firstRow().word
> }
> }
> println words.join('')
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
