Merge pull request #20650 from vespa-engine/deprecate-operation-attribute

Deprecate 'operation' attribute in user xml config

4 files changed, 45 insertions, 40 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/UserConfigBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/UserConfigBuilder.java
index e5cbb169b0c..2b8000f3e1a 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/UserConfigBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/UserConfigBuilder.java
@@ -43,7 +43,7 @@ public class UserConfigBuilder {
             logger.logApplicationPackage(Level.WARNING, "Unable to find config definition '" + key.asFileName() +
                                          "'. Please ensure that the name is spelled correctly, and that the def file is included in a bundle.");
         }
-        ConfigPayloadBuilder payloadBuilder = new DomConfigPayloadBuilder(def.orElse(null)).build(element);
+        ConfigPayloadBuilder payloadBuilder = new DomConfigPayloadBuilder(def.orElse(null), logger).build(element);
         ConfigPayloadBuilder old = builderMap.get(key);
         if (old != null) {
             logger.logApplicationPackage(Level.WARNING, "Multiple overrides for " + key + " found. Applying in the order they are discovered");
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilder.java
index 0c746ff4a75..2174616bacf 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilder.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.model.builder.xml.dom;
 
 import com.yahoo.collections.Tuple2;
 import com.yahoo.config.ConfigurationRuntimeException;
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.vespa.config.ConfigDefinition;
 import com.yahoo.vespa.config.ConfigDefinitionKey;
 import com.yahoo.vespa.config.ConfigPayloadBuilder;
@@ -13,6 +14,8 @@ import com.yahoo.vespa.config.util.ConfigUtils;
 import org.w3c.dom.Element;
 
 import java.util.List;
+import java.util.Optional;
+import java.util.logging.Level;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -31,9 +34,11 @@ public class DomConfigPayloadBuilder {
 
     /** The config definition, not null if not found */
     private final ConfigDefinition configDefinition;
+    private final Optional<DeployLogger> logger;
 
-    public DomConfigPayloadBuilder(ConfigDefinition configDefinition) {
+    public DomConfigPayloadBuilder(ConfigDefinition configDefinition, DeployLogger logger) {
         this.configDefinition = configDefinition;
+        this.logger = Optional.ofNullable(logger);
     }
 
     /**
@@ -218,6 +223,9 @@ public class DomConfigPayloadBuilder {
     }
 
     private void verifyLegalOperation(Element currElem) {
+        logger.ifPresent(log -> log.logApplicationPackage(
+                Level.WARNING, "The 'operation' attribute is deprecated for removal in Vespa 8. Use 'item' instead."));
+
         String operation = currElem.getAttribute("operation");
         if (! operation.equalsIgnoreCase("append"))
             throw new ConfigurationRuntimeException("The only supported array operation is 'append', got '"
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java
index 91482919e11..264d102161a 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/DomConfigPayloadBuilderTest.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.model.builder.xml.dom;
 
 import com.yahoo.config.ConfigurationRuntimeException;
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.codegen.DefParser;
 import com.yahoo.config.model.builder.xml.XmlHelper;
 import com.yahoo.slime.JsonFormat;
@@ -17,11 +18,11 @@ import org.w3c.dom.Element;
 import org.xml.sax.InputSource;
 
 import java.io.ByteArrayOutputStream;
-import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileReader;
 import java.io.Reader;
 import java.io.StringReader;
+import java.util.logging.Level;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
@@ -34,10 +35,13 @@ import static org.junit.Assert.fail;
  */
 public class DomConfigPayloadBuilderTest {
 
+    private static final DeployLogger logger = (level, message) ->  {
+        if (level.intValue() > Level.INFO.intValue()) System.err.println(message);
+    };
+
     @Test
     public void testFunctionTest_DefaultValues() throws FileNotFoundException {
         Element configRoot = getDocument(new FileReader("src/test/cfg/admin/userconfigs/functiontest-defaultvalues.xml"));
-        ConfigPayload config = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(configRoot));
         String expected = ""
                 + "{"
                 + "\"bool_val\":\"false\","
@@ -57,25 +61,14 @@ public class DomConfigPayloadBuilderTest {
                 + "\"enumarr\":[\"VALUES\"],"
                 + "\"myarray\":[{\"refval\":\":parent:\",\"fileVal\":\"command.com\",\"myStruct\":{\"a\":\"1\"},\"stringval\":[\"baah\",\"yikes\"],\"anotherarray\":[{\"foo\":\"7\"}]},{\"refval\":\":parent:\",\"fileVal\":\"display.sys\",\"myStruct\":{\"a\":\"-1\"},\"anotherarray\":[{\"foo\":\"1\"},{\"foo\":\"2\"}]}]"
                 + "}";
-        assertPayload(expected, config);
+        assertPayload(expected, configRoot);
     }
 
-    private void assertPayload(String expected, ConfigPayload payload) {
-        try {
-            ByteArrayOutputStream a = new ByteArrayOutputStream();
-            new JsonFormat(true).encode(a, payload.getSlime());
-            assertEquals(expected, a.toString());
-        } catch (Exception e) {
-            fail("Exception thrown when encoding slime: " + e.getMessage());
-        }
-
-    }
     // Multi line strings are not tested in 'DefaultValues', so here it is.
     @Test
     public void verifyThatWhitespaceIsPreservedForStrings() throws Exception {
         Element configRoot = getDocument(new FileReader("src/test/cfg/admin/userconfigs/whitespace-test.xml"));
-        ConfigPayload config = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(configRoot));
-        assertPayload("{\"stringVal\":\" This is a string\\n  that contains different kinds of whitespace \"}", config);
+        assertPayload("{\"stringVal\":\" This is a string\\n  that contains different kinds of whitespace \"}", configRoot);
     }
 
     @Test
@@ -87,8 +80,7 @@ public class DomConfigPayloadBuilderTest {
                 "    <item key=\"foo\">1337</item>" +
                 "  </intmap>" +
                 "</config>");
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(getDocument(xmlConfig)));
-        assertPayload("{\"intmap\":{\"bar\":\"1338\",\"foo\":\"1337\"}}", userConfig);
+        assertPayload("{\"intmap\":{\"bar\":\"1338\",\"foo\":\"1337\"}}", getDocument(xmlConfig));
     }
 
     @Test
@@ -104,8 +96,7 @@ public class DomConfigPayloadBuilderTest {
                 "    </item>" +
                 "  </innermap>" +
                 "</config>");
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(getDocument(xmlConfig)));
-        assertPayload("{\"innermap\":{\"bar\":{\"foo\":\"baz\"},\"foo\":{\"foo\":\"bar\"}}}", userConfig);
+        assertPayload("{\"innermap\":{\"bar\":{\"foo\":\"baz\"},\"foo\":{\"foo\":\"bar\"}}}", getDocument(xmlConfig));
     }
 
     @Test
@@ -127,10 +118,9 @@ public class DomConfigPayloadBuilderTest {
                 "    </item>" +
                 "  </nestedmap>" +
                 "</config>");
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(getDocument(xmlConfig)));
         assertPayload("{\"nestedmap\":{" +
                 "\"bar\":{\"inner\":{\"bar1\":\"30\",\"bar2\":\"40\"}}," +
-                "\"foo\":{\"inner\":{\"foo1\":\"10\",\"foo2\":\"20\"}}}}", userConfig);
+                "\"foo\":{\"inner\":{\"foo1\":\"10\",\"foo2\":\"20\"}}}}", getDocument(xmlConfig));
     }
 
     @Test
@@ -141,8 +131,7 @@ public class DomConfigPayloadBuilderTest {
                 "  <intarr operation=\"append\">1</intarr>" +
                 "  <intarr operation=\"append\">2</intarr>" +
                 "</config> ");
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(getDocument(xmlConfig)));
-        assertPayload("{\"intarr\":[\"1\",\"2\"]}", userConfig);
+        assertPayload("{\"intarr\":[\"1\",\"2\"]}", getDocument(xmlConfig));
     }
 
     @Test
@@ -151,8 +140,7 @@ public class DomConfigPayloadBuilderTest {
                 "<config name=\"test.function-test\">" +
                 "  <some-struct> <any-value>17</any-value> </some-struct>" +
                 "</config> ");
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(getDocument(xmlConfig)));
-        assertPayload("{\"someStruct\":{\"anyValue\":\"17\"}}", userConfig);
+        assertPayload("{\"someStruct\":{\"anyValue\":\"17\"}}", getDocument(xmlConfig));
     }
 
     // Verifies that an exception is thrown when the root element is not 'config'.
@@ -160,7 +148,7 @@ public class DomConfigPayloadBuilderTest {
     public void testFailWrongTagName() {
         Element configRoot = getDocument(new StringReader("<configs name=\"foo\"/>"));
         try {
-            new DomConfigPayloadBuilder(null).build(configRoot);
+            new DomConfigPayloadBuilder(null, logger).build(configRoot);
             fail("Expected exception for wrong tag name.");
         } catch (ConfigurationRuntimeException e) {
             assertEquals("The root element must be 'config', but was 'configs'.", e.getMessage());
@@ -172,7 +160,7 @@ public class DomConfigPayloadBuilderTest {
     public void testFailNoNameAttribute() {
         Element configRoot = getDocument(new StringReader("<config/>"));
         try {
-            new DomConfigPayloadBuilder(null).build(configRoot);
+            new DomConfigPayloadBuilder(null, logger).build(configRoot);
             fail("Expected exception for mismatch between def-name and xml name attribute.");
         } catch (ConfigurationRuntimeException e) {
             assertEquals("The 'config' element must have a 'name' attribute that matches the name of the config definition.", e.getMessage());
@@ -216,8 +204,7 @@ public class DomConfigPayloadBuilderTest {
                 "    </intarr>" +
                 "</config>");
 
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(configRoot));
-        assertPayload("{\"intarr\":[\"13\",\"10\",\"1337\"]}", userConfig);
+        assertPayload("{\"intarr\":[\"13\",\"10\",\"1337\"]}", configRoot);
     }
 
     @Test
@@ -231,9 +218,8 @@ public class DomConfigPayloadBuilderTest {
                         "    </lolarray>" +
                         "</config>");
 
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(configRoot));
         assertPayload("{\"lolarray\":[{\"foo\":\"hei\",\"bar\":\"hei2\"},{\"foo\":\"hoo\",\"bar\":\"hoo2\"},{\"foo\":\"happ\",\"bar\":\"happ2\"}]}",
-                      userConfig);
+                      configRoot);
     }
 
     @Test
@@ -247,8 +233,7 @@ public class DomConfigPayloadBuilderTest {
                 "    </lolarray>" +
                 "</config>");
 
-        ConfigPayload userConfig = ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(configRoot));
-        assertPayload("{\"lolarray\":[{\"fooarray\":[\"13\"]},{\"fooarray\":[\"10\"]},{\"fooarray\":[\"1337\"]}]}", userConfig);
+        assertPayload("{\"lolarray\":[{\"fooarray\":[\"13\"]},{\"fooarray\":[\"10\"]},{\"fooarray\":[\"1337\"]}]}", configRoot);
     }
 
     @Test(expected = ConfigurationRuntimeException.class)
@@ -257,7 +242,7 @@ public class DomConfigPayloadBuilderTest {
                 "<config name=\"test.arraytypes\" version=\"1\">" +
                 "    <item>13</item>" +
                 "</config>");
-        new DomConfigPayloadBuilder(null).build(configRoot);
+        new DomConfigPayloadBuilder(null, logger).build(configRoot);
     }
 
     @Test(expected=ConfigurationRuntimeException.class)
@@ -267,10 +252,22 @@ public class DomConfigPayloadBuilderTest {
                 "<longval>invalid</longval>" +
                 "</config>");
         DefParser defParser = new DefParser("simpletypes",
-                new FileReader(new File("src/test/resources/configdefinitions/test.simpletypes.def")));
+                new FileReader("src/test/resources/configdefinitions/test.simpletypes.def"));
         ConfigDefinition def = ConfigDefinitionBuilder.createConfigDefinition(defParser.getTree());
-        ConfigPayloadBuilder builder =  new DomConfigPayloadBuilder(def).build(configRoot);
-        //assertEquals(1, builder.warnings().size());
+        ConfigPayloadBuilder unused =  new DomConfigPayloadBuilder(def, logger).build(configRoot);
+    }
+
+
+    private void assertPayload(String expected, Element configRoot) {
+        ConfigPayload payload = ConfigPayload.fromBuilder(
+                new DomConfigPayloadBuilder(null, logger).build(configRoot));
+        try {
+            ByteArrayOutputStream a = new ByteArrayOutputStream();
+            new JsonFormat(true).encode(a, payload.getSlime());
+            assertEquals(expected, a.toString());
+        } catch (Exception e) {
+            fail("Exception thrown when encoding slime: " + e.getMessage());
+        }
     }
 
     private Element getDocument(Reader xmlReader) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java
index 3dd76c76cac..0e732984227 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java
@@ -129,7 +129,7 @@ public class RealDataScenarioTest {
     private static FlavorsConfig parseFlavors(Path path) {
         try {
             var element = XmlHelper.getDocumentBuilder().parse(path.toFile()).getDocumentElement();
-            return ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null).build(element)).toInstance(FlavorsConfig.class, "");
+            return ConfigPayload.fromBuilder(new DomConfigPayloadBuilder(null, null).build(element)).toInstance(FlavorsConfig.class, "");
         } catch (Exception e) {
             throw new RuntimeException(e);
         }