[
https://issues.apache.org/jira/browse/GROOVY-12461?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131061#comment-18131061
]
ASF GitHub Bot commented on GROOVY-12461:
-----------------------------------------
Copilot commented on code in PR #2995:
URL: https://github.com/apache/groovy/pull/2995#discussion_r4362256802
##########
src/main/java/groovy/grape/GrabAnnotationTransformation.java:
##########
@@ -486,8 +486,10 @@ private void callGrabAsStaticInitIfNeeded(final ClassNode
classNode, final Class
for (Map.Entry<String, String> e : systemProperties.entrySet()) {
block.addStatement(stmt(callX(SYSTEM_CLASSNODE, "setProperty",
args(constX(e.getKey()), constX(e.getValue())))));
}
- StaticMethodCallExpression enabled = callX(SYSTEM_CLASSNODE,
"getProperty", args(constX("groovy.grape.enable"), constX("true")));
- grabInitializers.add(ifS(eqX(enabled, constX("true")), block));
+ // only set the properties when the grab they support will
actually run
+ Expression grabAvailable = andX(callX(grapeClassNode,
"getEnableGrapes"),
+ notNullX(callX(grapeClassNode,
"getInstance")));
+ grabInitializers.add(ifS(grabAvailable, block));
Review Comment:
The generated call to `getEnableGrapes` assumes that method exists on
`Grape`. If `enableGrapes` is a boolean property following standard JavaBeans
conventions, the getter may be `isEnableGrapes()` instead, which would cause a
runtime `MissingMethodException` in compiled scripts. Prefer generating a
property access for `enableGrapes` (so Groovy resolves `is*` vs `get*`
correctly), or update the call to match the actual `Grape` API method name.
##########
src/test/groovy/bugs/Groovy12461.groovy:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package bugs
+
+import groovy.grape.Grape
+import groovy.grape.GrapeEngine
+import org.junit.jupiter.api.AfterEach
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
+
+/**
+ * The system properties of {@code @GrabConfig} are only set when the grab
runs.
+ */
+final class Groovy12461 {
+
+ private static final String PROBE = 'groovy.test.grabconfig.probe'
+
+ private static final String SCRIPT = """
+ @GrabConfig(systemProperties = '$PROBE=set-by-grabconfig')
+ @Grab('org.example:not-fetched:1.0')
+ import java.util.List
+ System.getProperty('$PROBE')
+ """
+
+ private GrapeEngine savedInstance
+ private boolean savedEnableGrapes
+ private String savedImpl
+
+ @BeforeEach
+ void setUp() {
+ savedInstance = Grape.instance
+ savedEnableGrapes = Grape.enableGrapes
+ savedImpl = System.getProperty('groovy.grape.impl')
+ System.clearProperty(PROBE)
+ }
+
+ @AfterEach
+ void tearDown() {
+ Grape.instance = savedInstance
+ Grape.enableGrapes = savedEnableGrapes
+ if (savedImpl == null) System.clearProperty('groovy.grape.impl')
+ else System.setProperty('groovy.grape.impl', savedImpl)
+ System.clearProperty(PROBE)
+ }
+
+ @Test
+ void testPropertiesSetWhenGrabRuns() {
+ Grape.instance = new NoOpGrapeEngine()
+ assert new GroovyShell().evaluate(SCRIPT) == 'set-by-grabconfig'
+ }
+
+ @Test
+ void testPropertiesNotSetWhenGrapesDisabled() {
+ Grape.instance = new NoOpGrapeEngine()
+ Grape.enableGrapes = false
+ assert new GroovyShell().evaluate(SCRIPT) == null
+ }
+
+ @Test
+ void testPropertiesNotSetWhenNoEngineAvailable() {
+ Grape.instance = null
+ System.setProperty('groovy.grape.impl',
'org.example.NoSuchGrapeEngine')
+ assert new GroovyShell().evaluate(SCRIPT) == null
+ }
+
+ private static class NoOpGrapeEngine implements GrapeEngine {
+ Object grab(String endorsedModule) { null }
+ Object grab(Map args) { null }
+ Object grab(Map args, Map... dependencies) { null }
+ Map<String, Map<String, List<String>>> enumerateGrapes() { [:] }
+ URI[] resolve(Map args, Map... dependencies) { new URI[0] }
+ URI[] resolve(Map args, List depsInfo, Map... dependencies) { new
URI[0] }
+ Map[] listDependencies(ClassLoader classLoader) { new Map[0] }
Review Comment:
Consider adding `@Override` annotations to these `GrapeEngine` method
implementations. It’s a low-cost way to ensure the test double stays in sync
with the interface (e.g., if method signatures change) and helps the compiler
catch accidental mismatches.
##########
src/test/groovy/bugs/Groovy12461.groovy:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package bugs
+
+import groovy.grape.Grape
+import groovy.grape.GrapeEngine
+import org.junit.jupiter.api.AfterEach
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
+
+/**
+ * The system properties of {@code @GrabConfig} are only set when the grab
runs.
+ */
+final class Groovy12461 {
+
+ private static final String PROBE = 'groovy.test.grabconfig.probe'
+
+ private static final String SCRIPT = """
+ @GrabConfig(systemProperties = '$PROBE=set-by-grabconfig')
+ @Grab('org.example:not-fetched:1.0')
+ import java.util.List
+ System.getProperty('$PROBE')
+ """
+
+ private GrapeEngine savedInstance
+ private boolean savedEnableGrapes
+ private String savedImpl
+
+ @BeforeEach
+ void setUp() {
+ savedInstance = Grape.instance
+ savedEnableGrapes = Grape.enableGrapes
+ savedImpl = System.getProperty('groovy.grape.impl')
+ System.clearProperty(PROBE)
+ }
+
+ @AfterEach
+ void tearDown() {
+ Grape.instance = savedInstance
+ Grape.enableGrapes = savedEnableGrapes
+ if (savedImpl == null) System.clearProperty('groovy.grape.impl')
+ else System.setProperty('groovy.grape.impl', savedImpl)
+ System.clearProperty(PROBE)
+ }
Review Comment:
These tests mutate global JVM state (`Grape.instance`, `Grape.enableGrapes`,
and system properties). If the build enables JUnit 5 parallel execution, this
can become flaky due to cross-test interference. Consider adding JUnit 5
resource locks (e.g., locking `Resources.SYSTEM_PROPERTIES` and a custom lock
for `groovy.grape.Grape`) or otherwise marking the class to run in the same
thread to ensure isolation.
> @GrabConfig(systemProperties = ...) sets JVM system properties at runtime
> even when Grape is disabled or unavailable
> --------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12461
> URL: https://issues.apache.org/jira/browse/GROOVY-12461
> Project: Groovy
> Issue Type: Improvement
> Affects Versions: 6.0.0
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Problem
> When a class or script uses {{@Grab}} together with
> {{@GrabConfig(systemProperties = ...)}}, {{GrabAnnotationTransformation}}
> (line 490 in 6.0.0) adds a static initializer that does two things in order:
> # sets each listed system property, guarded by
> {{System.getProperty("groovy.grape.enable", "true") == "true"}};
> # calls {{Grape.grab(...)}}.
> The {{@GrabConfig}} javadoc says the properties "must be set before invoking
> the grab", for example SSL or proxy settings. Their only purpose is to
> support the grab. But the guard looks at the system property alone. It
> ignores the other two ways Grape can be off:
> * *No engine on the classpath.* In Groovy 6, Grape engines live in
> {{groovy-grape-ivy}} and {{groovy-grape-maven}}. With neither present,
> {{Grape.getInstance()}} returns null, {{Grape.grab}} does nothing, and the
> log says "Grapes disabled". The properties are still set.
> * *{{Grape.setEnableGrapes(false)}}.* {{Grape.grab}} checks the static
> {{enableGrapes}} flag and does nothing. The generated guard never looks at
> that flag, so the properties are still set.
> In both cases the class changes process-wide state, possibly trust-store or
> proxy settings, for a grab that never happens. Hosts that turn Grape off,
> such as embedders, tooling and notebook kernels, reasonably expect
> {{@GrabConfig}} to be inert too.
> A smaller inconsistency: {{Grape}} reads the property with
> {{Boolean.parseBoolean}}, which ignores case, while the generated guard
> compares it to the exact string "true". So {{-Dgroovy.grape.enable=TRUE}}
> turns grabbing on but skips the properties.
> h2. Reproducer
> {code:groovy}
> // G2.groovy
> @GrabConfig(systemProperties = 'probe.flag=set-by-grabconfig')
> @Grab('org.apache.commons:commons-lang3:3.14.0')
> import java.util.List
> println "flag=" + System.getProperty('probe.flag')
> {code}
> Results against Groovy 6.0.0 (JDK 17):
> ||Setup||Grab runs?||{{probe.flag}}||
> |core jar only ({{java -cp groovy-6.0.0.jar groovy.ui.GroovyMain
> G2.groovy}})|no ("Grapes disabled")|*set-by-grabconfig*|
> |engines present, {{Grape.enableGrapes = false}} before evaluating with
> {{GroovyShell}}|no|*set-by-grabconfig*|
> |core jar only, {{-Dgroovy.grape.enable=false}}|no|null|
> |engines present, {{GrabAnnotationTransformation}} disabled via
> {{CompilerConfiguration.disabledGlobalASTTransformations}}|no|null|
> |engines present, defaults|yes|set-by-grabconfig (expected)|
> When {{@Grab}} is on a class rather than a script, the properties are set
> when that class initializes, not when the script starts.
> h2. Proposed fix
> Set the properties only when a grab will actually run. The simplest form is
> to add a small public helper to {{Grape}}, for example
> {{Grape.isGrabAvailable()}}, returning {{enableGrapes && getInstance() !=
> null}}. The generated guard then calls the helper instead of reading the
> system property. This also removes the case mismatch, because the helper uses
> the same parsed flag as {{Grape.grab}}.
> An alternative is to pass the properties to {{Grape.grab}} in its arguments
> map, which the compile-time path already does but no engine reads. The facade
> would then apply them just before delegating to the engine. That changes
> compile-time behaviour, because the properties would also be set in the
> compiler's JVM. That should be a separate decision, so the minimal fix above
> is preferred for this ticket.
> h2. Compatibility
> Behaviour changes only when Grape is disabled or no engine is available. That
> is exactly when the properties had no effect on any grab. Code that relied on
> {{@GrabConfig}} as a general way to set system properties, without a working
> grab, would need to call {{System.setProperty}} itself. That seems unlikely,
> and it could be noted in the release notes.
> h2. Tests
> * With no engine on the test classpath, a class using
> {{@GrabConfig(systemProperties)}} leaves the property unset. Run it in a
> forked JVM, or in {{groovy-grape-test}} with a classloader that hides the
> engines.
> * After {{Grape.setEnableGrapes(false)}}, the same class leaves the property
> unset. Restore the flag afterwards.
> * With an engine present and grapes enabled, behaviour is unchanged.
> h2. Context
> Found while drafting GEP-32 (restricted compiler profile). The profile turns
> off the Grab transform for each compilation, which avoids this. Hosts that
> rely on keeping the engines off the classpath, or on
> {{Grape.setEnableGrapes(false)}}, do not get that protection.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
