Paul King created GROOVY-12461:
----------------------------------
Summary: @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
Reporter: Paul King
Assignee: Paul King
h2. Problem
When a class or script uses {{@Grab}} together with
{{@GrabConfig(systemProperties = ...)}}, {{GrabAnnotationTransformation}} 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 6.0.0-beta-2:
||Setup||Grab runs?||{{probe.flag}}||
|core jar only ({{java -cp groovy-6.0.0-beta-2.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
(v8.20.10#820010)
