Handle case when groovy configuration is removed.

apache · Feb 11, 2025 · 5f19543 · 5f19543
1 parent c72e166
commit 5f19543
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 13 deletions.
diff --git a/...cal/src/main/java/org/apache/pinot/segment/local/function/GroovyConfigChangeListener.java b/...cal/src/main/java/org/apache/pinot/segment/local/function/GroovyConfigChangeListener.java
@@ -35,7 +35,10 @@ public void onChange(Set<String> changedConfigs, Map<String, String> clusterConf
       try {
         String configJson = clusterConfigs.get(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG);
         LOGGER.info("Updating Groovy Static Analyzer configuration with latest config: {}", configJson);
-        GroovyStaticAnalyzerConfig config = GroovyStaticAnalyzerConfig.fromJson(configJson);
+        GroovyStaticAnalyzerConfig config = null;
+        if (configJson != null) {
+          config = GroovyStaticAnalyzerConfig.fromJson(configJson);
+        }
         GroovyFunctionEvaluator.setConfig(config);
       } catch (Exception e) {
         LOGGER.error("Failed to update Groovy Static Analyzer configuration", e);

diff --git a/...-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java b/...-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java
@@ -213,7 +213,8 @@ public static void setMetrics(AbstractMetrics metrics, AbstractMetrics.Meter sec
     }
   }
 
-  public static void configureGroovySecurity(String groovyASTConfig) throws Exception {
+  public static void configureGroovySecurity(String groovyASTConfig)
+      throws Exception {
     try {
       if (groovyASTConfig != null) {
         setConfig(GroovyStaticAnalyzerConfig.fromJson(groovyASTConfig));
@@ -227,24 +228,14 @@ public static void configureGroovySecurity(String groovyASTConfig) throws Except
     }
   }
 
-
   /**
    * Initialize or update the configuration for the Groovy Static Analyzer.
    * @param config GroovyStaticAnalyzerConfig instance to be used for static syntax analysis.
    */
   public static void setConfig(GroovyStaticAnalyzerConfig config)
       throws JsonProcessingException {
     synchronized (GroovyFunctionEvaluator.class) {
-      if (_config == null) {
-        LOGGER.info("Initializing Groovy Static Analyzer: {}", config.toJson());
-        _config = config;
-      } else {
-        // It's fine to update the configuration because the static variable stores the address to a configuration
-        // object and whenever a GroovyFunctionEvaluator is created it makes a local copy of that address by reading
-        // the static variable atomically.  So, if the static config variable changes it will have no effect on
-        // any currently running evaluators.
-        _config = config;
-      }
+      _config = config;
     }
   }
 }
diff --git a/...src/test/java/org/apache/pinot/segment/local/function/GroovyConfigChangeListenerTest.java b/...src/test/java/org/apache/pinot/segment/local/function/GroovyConfigChangeListenerTest.java
@@ -19,6 +19,7 @@
 package org.apache.pinot.segment.local.function;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -65,6 +66,18 @@ public void testGroovyConfigIsUpdated() {
     assertEquals(GroovyFunctionEvaluator.getConfig().getAllowedReceivers(), List.of("java.lang.String"));
   }
 
+  @Test
+  public void testGroovyConfigIsDeleted() {
+    Set<String> changedConfigs = Set.of(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG);
+    Map<String, String> clusterConfigs = new HashMap<>();
+    clusterConfigs.put(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG, null);
+    assertEquals(GroovyFunctionEvaluator.getConfig().getAllowedReceivers(), List.of("java.lang.Math"));
+
+    GroovyConfigChangeListener groovyConfigChangeListener = new GroovyConfigChangeListener();
+    groovyConfigChangeListener.onChange(changedConfigs, clusterConfigs);
+    assertNull(GroovyFunctionEvaluator.getConfig());
+  }
+
   @Test
   public void testGroovyConfigIsUnchanged() {
     Set<String> changedConfigs = Set.of("random.test.key");