Merge pull request #3655 from vespa-engine/lesters/log-to-deploylogger

Lesters/log to deploylogger

4 files changed, 125 insertions, 5 deletions

diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/MacroShadower.java b/config-model/src/main/java/com/yahoo/searchdefinition/MacroShadower.java
index 9e847492a81..edf0ce69819 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/MacroShadower.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/MacroShadower.java
@@ -6,7 +6,6 @@ import com.yahoo.searchlib.rankingexpression.rule.*;
 import com.yahoo.searchlib.rankingexpression.transform.ExpressionTransformer;
 
 import java.util.Map;
-import java.util.logging.Logger;
 
 /**
  * Transforms function nodes to reference nodes if a macro shadows a built-in function.
@@ -23,8 +22,6 @@ import java.util.logging.Logger;
  */
 class MacroShadower extends ExpressionTransformer {
 
-    private static final Logger log = Logger.getLogger(MacroShadower.class.getName());
-
     private final Map<String, RankProfile.Macro> macros;
 
     public MacroShadower(Map<String, RankProfile.Macro> macros) {
@@ -58,11 +55,9 @@ class MacroShadower extends ExpressionTransformer {
         int functionArity = function.getFunction().arity();
         int macroArity = macro.getFormalParams() != null ? macro.getFormalParams().size() : 0;
         if (functionArity != macroArity) {
-            log.warning("Macro \"" + name + "\" has the same name as a built-in function. Due to different number of arguments, the built-in function will be used.");
             return transformChildren(function);
         }
 
-        log.warning("Macro \"" + name + "\" shadows the built-in function with the same name.");
         ReferenceNode node = new ReferenceNode(name, function.children(), null);
         return transformChildren(node);
     }
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java
index 536d4b4d67c..26f98026d4f 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java
@@ -74,6 +74,7 @@ public class Processing {
                 RankProfileTypeSettingsProcessor::new,
                 ReferenceFieldsProcessor::new,
                 FastAccessValidator::new,
+                ReservedMacroNames::new,
 
                 // These two should be last.
                 IndexingValidation::new,
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/ReservedMacroNames.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/ReservedMacroNames.java
new file mode 100644
index 00000000000..19063b8e7f9
--- /dev/null
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/ReservedMacroNames.java
@@ -0,0 +1,50 @@
+package com.yahoo.searchdefinition.processing;
+
+import com.yahoo.config.application.api.DeployLogger;
+import com.yahoo.searchdefinition.RankProfile;
+import com.yahoo.searchdefinition.RankProfileRegistry;
+import com.yahoo.searchdefinition.Search;
+import com.yahoo.searchlib.rankingexpression.parser.RankingExpressionParserConstants;
+import com.yahoo.vespa.model.container.search.QueryProfiles;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.logging.Level;
+
+/**
+ * Issues a warning if some macro has a reserved name. This is not necessarily
+ * an error, as a macro can shadow a built-in function.
+ *
+ * @author lesters
+ */
+public class ReservedMacroNames extends Processor {
+
+    public ReservedMacroNames(Search search, DeployLogger deployLogger, RankProfileRegistry rankProfileRegistry, QueryProfiles queryProfiles) {
+        super(search, deployLogger, rankProfileRegistry, queryProfiles);
+    }
+
+    @Override
+    public void process() {
+        Set<String> reservedNames = getReservedNames();
+        for (RankProfile rp : rankProfileRegistry.allRankProfiles()) {
+            for (String macroName : rp.getMacros().keySet()) {
+                if (reservedNames.contains(macroName)) {
+                    deployLogger.log(Level.WARNING, "Macro \"" + macroName + "\" " +
+                            "in rank profile \"" + rp.getName() + "\" " +
+                            "has a reserved name. This might mean that the macro shadows " +
+                            "the built-in function with the same name."
+                    );
+                }
+            }
+        }
+    }
+
+    private Set<String> getReservedNames() {
+        Set<String> names = new HashSet<>();
+        for (String token : RankingExpressionParserConstants.tokenImage) {
+            String tokenWithoutQuotes = token.substring(1, token.length()-1);
+            names.add(tokenWithoutQuotes);
+        }
+        return names;
+    }
+}
diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/processing/ReservedMacroNamesTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/processing/ReservedMacroNamesTestCase.java
new file mode 100644
index 00000000000..ba641dd4a3e
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/searchdefinition/processing/ReservedMacroNamesTestCase.java
@@ -0,0 +1,74 @@
+package com.yahoo.searchdefinition.processing;
+
+import com.yahoo.config.application.api.DeployLogger;
+import com.yahoo.searchdefinition.RankProfileRegistry;
+import com.yahoo.searchdefinition.Search;
+import com.yahoo.searchdefinition.SearchBuilder;
+import com.yahoo.searchdefinition.parser.ParseException;
+import org.junit.Test;
+
+import java.util.logging.Level;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * @author lesters
+ */
+public class ReservedMacroNamesTestCase {
+
+    @Test
+    public void requireThatMacrosWithReservedNamesIssueAWarning() throws ParseException {
+        RankProfileRegistry rankProfileRegistry = new RankProfileRegistry();
+        SearchBuilder builder = new SearchBuilder(rankProfileRegistry);
+        builder.importString(
+                "search test {\n" +
+                        "    document test { \n" +
+                        "        field a type string { \n" +
+                        "            indexing: index \n" +
+                        "        }\n" +
+                        "    }\n" +
+                        "    \n" +
+                        "    rank-profile test_rank_profile {\n" +
+                        "        macro not_a_reserved_name(x) {\n" +
+                        "            expression: x + x\n" +
+                        "        }\n" +
+                        "        macro sigmoid(x) {\n" +
+                        "            expression: x * x\n" +
+                        "        }\n" +
+                        "        first-phase {\n" +
+                        "            expression: sigmoid(2) + not_a_reserved_name(1)\n" +
+                        "        }\n" +
+                        "    }\n" +
+                        "    rank-profile test_rank_profile_2 inherits test_rank_profile {\n" +
+                        "        macro sin(x) {\n" +
+                        "            expression: x * x\n" +
+                        "        }\n" +
+                        "        first-phase {\n" +
+                        "            expression: sigmoid(2) + sin(1)\n" +
+                        "        }\n" +
+                        "    }\n" +
+                        "}\n");
+        builder.build();
+        Search search = builder.getSearch();
+        TestDeployLogger deployLogger = new TestDeployLogger();
+        ReservedMacroNames processor = new ReservedMacroNames(search, deployLogger, rankProfileRegistry, null);
+        processor.process();
+
+        assertTrue(deployLogger.log.contains("sigmoid") && deployLogger.log.contains("test_rank_profile"));
+        assertTrue(deployLogger.log.contains("sigmoid") && deployLogger.log.contains("test_rank_profile_2"));
+        assertTrue(deployLogger.log.contains("sin") && deployLogger.log.contains("test_rank_profile_2"));
+        assertFalse(deployLogger.log.contains("not_a_reserved_name") && deployLogger.log.contains("test_rank_profile"));
+        assertFalse(deployLogger.log.contains("not_a_reserved_name") && deployLogger.log.contains("test_rank_profile_2"));
+
+    }
+
+    public static class TestDeployLogger implements DeployLogger {
+        public String log = "";
+        @Override
+        public void log(Level level, String message) {
+            log += message;
+        }
+    }
+    
+}