From 6ada0742477c575cc6941704500c0ee8c9ac5596 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Thu, 1 Nov 2018 14:44:09 +0100 Subject: Supporting clearing config array when ovverriding config --- .../builder/xml/dom/DomConfigPayloadBuilder.java | 27 ++++-- .../vespa/model/builder/UserConfigBuilderTest.java | 100 +++++++++++++++------ .../yahoo/vespa/config/ConfigPayloadBuilder.java | 25 ++++++ 3 files changed, 118 insertions(+), 34 deletions(-) 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 e9abcfd87a9..d0abffcbac4 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 @@ -169,9 +169,23 @@ public class DomConfigPayloadBuilder { // Check for legacy (pre Vespa 6) usage throw new IllegalArgumentException("The 'index' attribute on config elements is not supported - use "); } else if (element.hasAttribute("operation")) { - // inner array, currently the only supported operation is 'append' - verifyLegalOperation(element); - ConfigPayloadBuilder childPayloadBuilder = payloadBuilder.getArray(name).append(); + // inner array, the supported operations are 'append' and 'clear' + String operation = verifyLegalOperation(element); + ConfigPayloadBuilder childPayloadBuilder; + switch (operation) { + case "append": + childPayloadBuilder = payloadBuilder.getArray(name).append(); + break; + case "clear": + // Clear array if it exists, use the existing builder + // Creating the array happens when handling the children ('item's) + if (payloadBuilder.arrayExists(name)) + payloadBuilder.clearArray(name); + childPayloadBuilder = payloadBuilder; + break; + default: + throw new RuntimeException("Unknown operation '" + operation + "'"); + } //Cursor array = node.setArray(name); for (Element child : children) { //Cursor struct = array.addObject(); @@ -233,11 +247,12 @@ public class DomConfigPayloadBuilder { } } - private void verifyLegalOperation(Element currElem) { + private String verifyLegalOperation(Element currElem) { String operation = currElem.getAttribute("operation"); - if (! operation.equalsIgnoreCase("append")) - throw new ConfigurationRuntimeException("The only supported array operation is 'append', got '" + if (! Arrays.asList("append", "clear").contains(operation)) + throw new ConfigurationRuntimeException("The supported array operations are 'append' and 'clear', got '" + operation + "' at XML node '" + XML.getNodePath(currElem, " > ") + "'."); + return operation; } } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java index d1ef1010b73..6414f12c2fb 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java @@ -8,7 +8,6 @@ import com.yahoo.config.model.deploy.ConfigDefinitionStore; import com.yahoo.test.SimpletypesConfig; import com.yahoo.config.model.producer.UserConfigRepo; import com.yahoo.config.model.builder.xml.XmlHelper; -import com.yahoo.vespa.config.ConfigDefinition; import com.yahoo.vespa.config.ConfigDefinitionKey; import com.yahoo.vespa.config.ConfigPayload; import com.yahoo.vespa.config.ConfigPayloadBuilder; @@ -17,30 +16,28 @@ import org.junit.Test; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.xml.sax.InputSource; -import org.xml.sax.SAXException; -import javax.xml.parsers.ParserConfigurationException; -import java.io.IOException; import java.io.Reader; import java.io.StringReader; +import java.util.Arrays; +import java.util.Collections; import java.util.Optional; import static org.hamcrest.core.Is.is; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; /** * @author Ulf Lilleengen - * @since 5.1 */ public class UserConfigBuilderTest { - private final ConfigDefinitionStore configDefinitionStore = new ConfigDefinitionStore() { - @Override - public Optional getConfigDefinition(ConfigDefinitionKey defKey) { return Optional.empty(); } - }; + private final ConfigDefinitionStore configDefinitionStore = defKey -> Optional.empty(); @Test - public void require_that_simple_config_is_resolved() throws ParserConfigurationException, IOException, SAXException { + public void require_that_simple_config_is_resolved() { Element configRoot = getDocument("" + " 13" + "" + @@ -56,31 +53,67 @@ public class UserConfigBuilderTest { assertThat(config.stringval(), is("foolio")); } - public static ConfigType createConfig(Class clazz, ConfigPayloadBuilder builder) { - return ConfigPayload.fromBuilder(builder).toInstance(clazz, ""); - } - - @Test - public void require_that_arrays_config_is_resolved() throws ParserConfigurationException, IOException, SAXException { + public void require_that_arrays_config_is_resolved() { Element configRoot = getDocument("" + - " 13" + - " 10" + - " 1337" + - ""); + " 13" + + " 10" + + " 1337" + + ""); UserConfigRepo map = UserConfigBuilder.build(configRoot, configDefinitionStore, new BaseDeployLogger()); assertFalse(map.isEmpty()); ConfigDefinitionKey key = new ConfigDefinitionKey("arraytypes", "config"); assertNotNull(map.get(key)); ArraytypesConfig config = createConfig(ArraytypesConfig.class, map.get(key)); - assertThat(config.intarr().size(), is(3)); - assertThat(config.intarr(0), is(13)); - assertThat(config.intarr(1), is(10)); - assertThat(config.intarr(2), is(1337)); + assertEquals(Arrays.asList(13,10,1337), config.intarr()); + + Element configRoot2 = getDocument("" + + " 456" + + ""); + UserConfigRepo map2 = UserConfigBuilder.build(configRoot2, configDefinitionStore, new BaseDeployLogger()); + map.merge(map2); + ArraytypesConfig config2 = createConfig(ArraytypesConfig.class, map2.get(key)); + assertEquals(Collections.singletonList(456), config2.intarr()); + } + + @Test + public void require_that_array_with_items_is_resolved() { + ConfigDefinitionKey key = new ConfigDefinitionKey("arraytypes", "config"); + + Element configRoot = getDocument("" + + " " + + " 0" + + " " + + ""); + UserConfigRepo map = UserConfigBuilder.build(configRoot, configDefinitionStore, new BaseDeployLogger()); + assertFalse(map.isEmpty()); + assertNotNull(map.get(key)); + ArraytypesConfig config = createConfig(ArraytypesConfig.class, map.get(key)); + assertEquals(Collections.singletonList(0), config.intarr()); + + Element configRoot2 = getDocument("" + + " " + + " 1" + + " 2" + + " " + + ""); + UserConfigRepo map2 = mergeAndCreateConfig(map, configRoot2); + ArraytypesConfig config2 = createConfig(ArraytypesConfig.class, map2.get(key)); + assertEquals(Arrays.asList(1, 2), config2.intarr()); + + + Element configRoot3 = getDocument("" + + " " + + " 3" + + " " + + ""); + UserConfigRepo map3 = mergeAndCreateConfig(map2, configRoot3); + ArraytypesConfig config3 = createConfig(ArraytypesConfig.class, map3.get(key)); + assertEquals(Collections.singletonList(3), config3.intarr()); } @Test - public void require_that_arrays_of_structs_are_resolved() throws ParserConfigurationException, IOException, SAXException { + public void require_that_arrays_of_structs_are_resolved() { Element configRoot = getDocument( " " + " " + @@ -107,7 +140,7 @@ public class UserConfigBuilderTest { } @Test - public void no_exception_when_config_class_does_not_exist() throws ParserConfigurationException, IOException, SAXException { + public void no_exception_when_config_class_does_not_exist() { Element configRoot = getDocument("" + " 1" + ""); @@ -116,7 +149,7 @@ public class UserConfigBuilderTest { assertNotNull(builder); } - private Element getDocument(String xml) throws ParserConfigurationException { + private Element getDocument(String xml) { Reader xmlReader = new StringReader("" + xml + ""); Document doc; try { @@ -126,4 +159,15 @@ public class UserConfigBuilderTest { } return doc.getDocumentElement(); } + + private static ConfigType createConfig(Class clazz, ConfigPayloadBuilder builder) { + return ConfigPayload.fromBuilder(builder).toInstance(clazz, ""); + } + + private UserConfigRepo mergeAndCreateConfig(UserConfigRepo original, Element newElement) { + UserConfigRepo map = UserConfigBuilder.build(newElement, configDefinitionStore, new BaseDeployLogger()); + original.merge(map); + return map; + } + } diff --git a/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java b/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java index bb974ddae42..88eed8cbf36 100644 --- a/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java +++ b/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java @@ -109,6 +109,27 @@ public class ConfigPayloadBuilder { return a; } + /** + * Check if array with this name exists. + * + * @param name Name of array. + * @return true if array exists, false otherwise + */ + public boolean arrayExists(String name) { + return arrayMap.containsKey(name); + } + + /** + * Clears contents of an array + * + * @param name Name of array. + */ + public void clearArray(String name) { + Array a = arrayMap.get(name); + if (a != null) + a.clear(); + } + private void validateArray(String name) { if (configDefinition != null) { configDefinition.verify(name); @@ -417,6 +438,10 @@ public class ConfigPayloadBuilder { } return this; } + + public void clear() { + elements.clear(); + } } private ConfigPayloadBuilder(ConfigPayloadBuilder other) { -- cgit v1.2.3 From 6f10e3c51701f97224b48a7285971e3febeec3e0 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Mon, 5 Nov 2018 08:16:49 +0100 Subject: Simplify by using a removeArray method, make operation case insensitive --- .../model/builder/xml/dom/DomConfigPayloadBuilder.java | 7 +++---- .../com/yahoo/vespa/config/ConfigPayloadBuilder.java | 18 +++--------------- 2 files changed, 6 insertions(+), 19 deletions(-) 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 d0abffcbac4..568a1229a36 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 @@ -170,7 +170,7 @@ public class DomConfigPayloadBuilder { throw new IllegalArgumentException("The 'index' attribute on config elements is not supported - use "); } else if (element.hasAttribute("operation")) { // inner array, the supported operations are 'append' and 'clear' - String operation = verifyLegalOperation(element); + String operation = element.getAttribute("operation"); ConfigPayloadBuilder childPayloadBuilder; switch (operation) { case "append": @@ -179,8 +179,7 @@ public class DomConfigPayloadBuilder { case "clear": // Clear array if it exists, use the existing builder // Creating the array happens when handling the children ('item's) - if (payloadBuilder.arrayExists(name)) - payloadBuilder.clearArray(name); + payloadBuilder.removeArray(name); childPayloadBuilder = payloadBuilder; break; default: @@ -249,7 +248,7 @@ public class DomConfigPayloadBuilder { private String verifyLegalOperation(Element currElem) { String operation = currElem.getAttribute("operation"); - if (! Arrays.asList("append", "clear").contains(operation)) + if (! Arrays.asList("append", "clear").contains(operation.toLowerCase())) throw new ConfigurationRuntimeException("The supported array operations are 'append' and 'clear', got '" + operation + "' at XML node '" + XML.getNodePath(currElem, " > ") + "'."); return operation; diff --git a/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java b/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java index 88eed8cbf36..863505af5b0 100644 --- a/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java +++ b/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadBuilder.java @@ -110,24 +110,12 @@ public class ConfigPayloadBuilder { } /** - * Check if array with this name exists. + * Removes an array * * @param name Name of array. - * @return true if array exists, false otherwise */ - public boolean arrayExists(String name) { - return arrayMap.containsKey(name); - } - - /** - * Clears contents of an array - * - * @param name Name of array. - */ - public void clearArray(String name) { - Array a = arrayMap.get(name); - if (a != null) - a.clear(); + public void removeArray(String name) { + arrayMap.remove(name); } private void validateArray(String name) { -- cgit v1.2.3 From 0bd5c6488b8d1b5792ec7ecee8c9dc3ef77d207c Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Mon, 5 Nov 2018 09:33:30 +0100 Subject: Support 'clear' operation when using 'item' syntax only Make sure to lowercase operation attribute --- .../builder/xml/dom/DomConfigPayloadBuilder.java | 44 ++++++++++++---------- .../vespa/model/builder/UserConfigBuilderTest.java | 8 ---- 2 files changed, 24 insertions(+), 28 deletions(-) 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 568a1229a36..20415343755 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 @@ -169,22 +169,7 @@ public class DomConfigPayloadBuilder { // Check for legacy (pre Vespa 6) usage throw new IllegalArgumentException("The 'index' attribute on config elements is not supported - use "); } else if (element.hasAttribute("operation")) { - // inner array, the supported operations are 'append' and 'clear' - String operation = element.getAttribute("operation"); - ConfigPayloadBuilder childPayloadBuilder; - switch (operation) { - case "append": - childPayloadBuilder = payloadBuilder.getArray(name).append(); - break; - case "clear": - // Clear array if it exists, use the existing builder - // Creating the array happens when handling the children ('item's) - payloadBuilder.removeArray(name); - childPayloadBuilder = payloadBuilder; - break; - default: - throw new RuntimeException("Unknown operation '" + operation + "'"); - } + ConfigPayloadBuilder childPayloadBuilder = getBuilderForInnerArray(element, payloadBuilder, name); //Cursor array = node.setArray(name); for (Element child : children) { //Cursor struct = array.addObject(); @@ -246,12 +231,31 @@ public class DomConfigPayloadBuilder { } } - private String verifyLegalOperation(Element currElem) { + private void verifyLegalOperation(Element currElem) { String operation = currElem.getAttribute("operation"); - if (! Arrays.asList("append", "clear").contains(operation.toLowerCase())) - throw new ConfigurationRuntimeException("The supported array operations are 'append' and 'clear', got '" + if (! operation.equalsIgnoreCase("append")) + throw new ConfigurationRuntimeException("The only supported array operation is 'append', got '" + operation + "' at XML node '" + XML.getNodePath(currElem, " > ") + "'."); - return operation; + } + + private ConfigPayloadBuilder getBuilderForInnerArray(Element element, ConfigPayloadBuilder payloadBuilder, String arrayName) { + // inner array, the supported operations are 'append' and 'clear' + String operation = element.getAttribute("operation").toLowerCase(); + ConfigPayloadBuilder arrayPayloadBuilder; + switch (operation) { + case "append": + arrayPayloadBuilder = payloadBuilder.getArray(arrayName).append(); + break; + case "clear": + // Clear array if it exists, use the existing builder + // Creating the array happens when handling the children ('item's) + payloadBuilder.removeArray(arrayName); + arrayPayloadBuilder = payloadBuilder; + break; + default: + throw new RuntimeException("Unknown operation '" + operation + "' at XML node '" + XML.getNodePath(element, " > ") + "'."); + } + return arrayPayloadBuilder; } } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java index 6414f12c2fb..747ce93d86b 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/builder/UserConfigBuilderTest.java @@ -66,14 +66,6 @@ public class UserConfigBuilderTest { assertNotNull(map.get(key)); ArraytypesConfig config = createConfig(ArraytypesConfig.class, map.get(key)); assertEquals(Arrays.asList(13,10,1337), config.intarr()); - - Element configRoot2 = getDocument("" + - " 456" + - ""); - UserConfigRepo map2 = UserConfigBuilder.build(configRoot2, configDefinitionStore, new BaseDeployLogger()); - map.merge(map2); - ArraytypesConfig config2 = createConfig(ArraytypesConfig.class, map2.get(key)); - assertEquals(Collections.singletonList(456), config2.intarr()); } @Test -- cgit v1.2.3