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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
