Merge pull request #7940 from vespa-engine/gjoranv/allow-adding-config-structs

Gjoranv/allow adding config structs

2 files changed, 165 insertions, 46 deletions

diff --git a/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadApplier.java b/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadApplier.java
index 3efa7fdd879..d406dfe2d77 100644
--- a/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadApplier.java
+++ b/config/src/main/java/com/yahoo/vespa/config/ConfigPayloadApplier.java
@@ -94,7 +94,9 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> {
             trace("top of stack=" + stack.peek().toString());
             String name = stack.peek().nameStack().peek();
             if (inspector.type().equals(Type.OBJECT)) {
-                stack.push(createBuilder(stack.peek(), name));
+                NamedBuilder builder = createBuilder(stack.peek(), name);
+                if (builder == null) return;  // Ignore non-existent struct array class
+                stack.push(builder);
             }
             handleValue(inspector);
             if (inspector.type().equals(Type.OBJECT)) {
@@ -143,7 +145,9 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> {
                     parentBuilder.nameStack().pop();
                     return;
                 } else {
-                    stack.push(createBuilder(parentBuilder, name));
+                    NamedBuilder builder = createBuilder(parentBuilder, name);
+                    if (builder == null) return;  // Ignore non-existent struct class
+                    stack.push(builder);
                 }
             } else if (inspector.type().equals(Type.ARRAY)) {
                 for (int i = 0; i < inspector.children(); i++) {
@@ -179,6 +183,8 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> {
 
     private void handleInnerMap(String name, Inspector inspector) {
         NamedBuilder builder = createBuilder(stack.peek(), stack.peek().peekName());
+        if (builder == null)
+            throw new RuntimeException("Missing map builder (this should never happen): " + stack.peek());
         setMapLeafValue(name, builder.builder());
         stack.push(builder);
         inspector.traverse(new ObjectTraverser() {
@@ -222,7 +228,8 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> {
 
     NamedBuilder createBuilder(NamedBuilder parentBuilder, String name) {
         Object builder = parentBuilder.builder();
-        Object newBuilder = getBuilderForStruct(findBuilderName(name), name, builder.getClass().getDeclaringClass());
+        Object newBuilder = getBuilderForStruct(name, builder.getClass().getDeclaringClass());
+        if (newBuilder == null) return null;
         trace("New builder for " + name + "=" + newBuilder);
         trace("Pushing builder for " + name + "=" + newBuilder + " onto stack");
         return new NamedBuilder((ConfigBuilder) newBuilder, name);
@@ -366,38 +373,51 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> {
     }
 
 
-    private String findBuilderName(String name) {
+    private String capitalize(String name) {
         StringBuilder sb = new StringBuilder();
         sb.append(name.substring(0, 1).toUpperCase()).append(name.substring(1));
         return sb.toString();
     }
 
-    private Constructor<?> lookupBuilderForStruct(String builderName, String name, Class<?> currentClass) {
+    private Constructor<?> lookupBuilderForStruct(String structName, String name, Class<?> currentClass) {
         final String currentClassName = currentClass.getName();
-        trace("builderName=" + builderName + ", name=" + name + ",current class=" + currentClassName);
-        Class<?> structClass = findClass(currentClass, currentClassName + "$" + builderName);
-        Class<?> structBuilderClass = findClass(structClass, currentClassName + "$" + builderName + "$Builder");
+        trace("structName=" + structName + ", name=" + name + ",current class=" + currentClassName);
+        Class<?> structClass = getInnerClass(currentClass, currentClassName + "$" + structName);
+        if (structClass == null) {
+            log.info("Could not find nested class '" + currentClassName + "$" + structName +
+                             "'. Ignoring it, assuming it's been added to a newer version of the config.");
+            return null;
+        }
+        return getStructBuilderConstructor(structClass, currentClassName, structName);
+    }
+
+    private Constructor<?> getStructBuilderConstructor(Class<?> structClass, String currentClassName, String builderName) {
+        String structBuilderName = currentClassName + "$" + builderName + "$Builder";
+        Class<?> structBuilderClass = getInnerClass(structClass, structBuilderName);
+        if (structBuilderClass == null)
+            throw new RuntimeException("Could not find builder class " + structBuilderName);
         try {
             return structBuilderClass.getDeclaredConstructor(new Class<?>[]{});
         } catch (NoSuchMethodException e) {
             throw new RuntimeException("Could not create class '" + "'" + structBuilderClass.getName() + "'");
         }
+
     }
 
     /**
-     * Finds a nested class or builder class with the given <code>name</code>name in <code>clazz</code>
+     * Finds a nested class with the given <code>name</code>name in <code>clazz</code>
      * @param clazz a Class
      * @param name a name
-     * @return class found, or throws an exception is no class is found
+     * @return class found, or null if no class is found
      */
-    private Class<?> findClass(Class<?> clazz, String name) {
+    private Class<?> getInnerClass(Class<?> clazz, String name) {
         for (Class<?> cls : clazz.getDeclaredClasses()) {
             if (cls.getName().equals(name)) {
                 trace("Found class " + cls.getName());
                 return cls;
             }
         }
-        throw new RuntimeException("could not find class representing '" + printCurrentConfigName() + "'");
+        return null;
     }
 
     private final Map<String, Constructor<?>> constructorCache = new HashMap<>();
@@ -405,11 +425,13 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> {
         return builderName + "." + name + "." + currentClass.getName();
     }
 
-    private Object getBuilderForStruct(String builderName, String name, Class<?> currentClass) {
-        String key = constructorCacheKey(builderName, name, currentClass);
+    private Object getBuilderForStruct(String name, Class<?> currentClass) {
+        String structName = capitalize(name);
+        String key = constructorCacheKey(structName, name, currentClass);
         Constructor<?> ctor = constructorCache.get(key);
         if (ctor == null) {
-            ctor = lookupBuilderForStruct(builderName, name, currentClass);
+            ctor = lookupBuilderForStruct(structName, name, currentClass);
+            if (ctor == null) return null;
             constructorCache.put(key, ctor);
         }
         Object builder;
@@ -421,22 +443,6 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> {
         return builder;
     }
 
-    private String printCurrentConfigName() {
-        StringBuilder sb = new StringBuilder();
-        ArrayList<String> stackElements = new ArrayList<>();
-        Stack<String> nameStack = stack.peek().nameStack();
-        while (!nameStack.empty()) {
-            stackElements.add(nameStack.pop());
-        }
-        Collections.reverse(stackElements);
-        for (String s : stackElements) {
-            sb.append(s);
-            sb.append(".");
-        }
-        sb.deleteCharAt(sb.length() - 1); // remove last .
-        return sb.toString();
-    }
-
     private void debug(String message) {
         if (log.isLoggable(LogLevel.DEBUG)) {
             log.log(LogLevel.DEBUG, message);
diff --git a/config/src/test/java/com/yahoo/vespa/config/ConfigPayloadTest.java b/config/src/test/java/com/yahoo/vespa/config/ConfigPayloadTest.java
index a21a08f88d6..63a55d20edf 100644
--- a/config/src/test/java/com/yahoo/vespa/config/ConfigPayloadTest.java
+++ b/config/src/test/java/com/yahoo/vespa/config/ConfigPayloadTest.java
@@ -1,9 +1,14 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config;
 
-import com.yahoo.foo.*;
 import com.yahoo.config.codegen.DefParser;
 import com.yahoo.config.codegen.InnerCNode;
+import com.yahoo.foo.AppConfig;
+import com.yahoo.foo.ArraytypesConfig;
+import com.yahoo.foo.IntConfig;
+import com.yahoo.foo.MaptypesConfig;
+import com.yahoo.foo.SimpletypesConfig;
+import com.yahoo.foo.StructtypesConfig;
 import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Slime;
 import com.yahoo.text.StringUtilities;
@@ -13,7 +18,11 @@ import java.io.IOException;
 import java.io.StringReader;
 
 import static org.hamcrest.CoreMatchers.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;
+import static org.junit.Assert.assertTrue;
 
 /**
  * @author Ulf Lilleengen                                               3
@@ -253,14 +262,16 @@ public class ConfigPayloadTest {
 
     @Test
     public void test_simple_struct() throws Exception {
-        StructtypesConfig config = createStructtypesConfigSimple("foobar", "MALE", new String[] { "foo@bar", "bar@foo" });
+        Slime slime = new Slime();
+        addStructFields(slime.setObject().setObject("simple"), "foobar", "MALE", new String[] { "foo@bar", "bar@foo" });
+        StructtypesConfig config = new ConfigPayload(slime).toInstance(StructtypesConfig.class, "");
+
         assertThat(config.simple().name(), is("foobar"));
         assertThat(config.simple().gender(), is(StructtypesConfig.Simple.Gender.Enum.MALE));
         assertThat(config.simple().emails(0), is("foo@bar"));
         assertThat(config.simple().emails(1), is("bar@foo"));
     }
 
-
     @Test
     public void test_simple_struct_arrays() throws Exception {
         StructtypesConfig config = createStructtypesConfigArray(new String[] { "foo", "bar" },
@@ -325,13 +336,121 @@ public class ConfigPayloadTest {
     }
 
     @Test
-    public void test_function_test() {
-        // TODO: Test function test config as a complete config example
+    public void test_simple_map() {
+        Slime slime = new Slime();
+        Cursor map = slime.setObject().setObject("stringmap");
+        map.setString("key","val");
+
+        MaptypesConfig config = new ConfigPayload(slime).toInstance(MaptypesConfig.class, "");
+        assertThat(config.stringmap("key"), is("val"));
+    }
+
+    @Test
+    public void test_map_of_struct() {
+        Slime slime = new Slime();
+        Cursor map = slime.setObject().setObject("innermap");
+        map.setObject("one").setLong("foo", 1);
+        map.setObject("two").setLong("foo", 2);
+
+        MaptypesConfig config = new ConfigPayload(slime).toInstance(MaptypesConfig.class, "");
+        assertThat(config.innermap("one").foo(), is(1));
+        assertThat(config.innermap("two").foo(), is(2));
+    }
+
+    @Test
+    public void test_map_of_map() {
+        Slime slime = new Slime();
+        Cursor map = slime.setObject().setObject("nestedmap").setObject("my-nested").setObject("inner");
+        map.setLong("one", 1);
+        map.setLong("two", 2);
+
+        MaptypesConfig config = new ConfigPayload(slime).toInstance(MaptypesConfig.class, "");
+        assertThat(config.nestedmap("my-nested").inner("one"), is(1));
+        assertThat(config.nestedmap("my-nested").inner("two"), is(2));
+    }
+
+    /* Non existent fields of all types must be ignored to allow adding new fields to a config definition
+     * without breaking hosted Vespa applications that have config overrides for the given config class.
+     * The generated payload for the user override will contain the new field (empty as the user doesn't
+     * override it), because it exists in the latest config def version. The config class version follows
+     * the applications's Vespa version, which may be older and doesn't have the new struct. Hence, we just
+     * ignore unknown fields in the payload.
+     */
+
+    @Test
+    public void non_existent_leaf_in_payload_is_ignored() {
+        SimpletypesConfig config = createSimpletypesConfig("non_existent", "");
+        assertNotNull(config);
+    }
+
+    @Test
+    public void non_existent_struct_in_payload_is_ignored() {
+        Slime slime = new Slime();
+        addStructFields(slime.setObject().setObject("non_existent"), "", "", null);
+        StructtypesConfig config = new ConfigPayload(slime).toInstance(StructtypesConfig.class, "");
+        assertNotNull(config);
+    }
+
+    @Test
+    public void non_existent_struct_in_struct_in_payload_is_ignored() {
+        Slime slime = new Slime();
+        addStructFields(slime.setObject().setObject("nested").setObject("non_existent_inner"), "", "", null);
+        StructtypesConfig config = new ConfigPayload(slime).toInstance(StructtypesConfig.class, "");
+        assertNotNull(config);
+    }
+
+    @Test
+    public void non_existent_array_of_struct_in_payload_is_ignored() {
+        Slime slime = new Slime();
+        Cursor array = slime.setObject().setArray("non_existent_arr");
+        array.addObject().setString("name", "val");
+        StructtypesConfig config = new ConfigPayload(slime).toInstance(StructtypesConfig.class, "");
+        assertNotNull(config);
     }
 
     @Test
-    public void test_set_nonexistent_field() throws Exception {
-        createSimpletypesConfig("doesnotexist", "blabla");
+    public void non_existent_struct_in_array_of_struct_in_payload_is_ignored() {
+        Slime slime = new Slime();
+        Cursor nestedArrEntry = slime.setObject().setArray("nestedarr").addObject();
+        addStructFields(nestedArrEntry.setObject("inner"), "existing", "MALE", null);
+        addStructFields(nestedArrEntry.setObject("non_existent"), "non-existent", "MALE", null);
+
+        StructtypesConfig config =  new ConfigPayload(slime).toInstance(StructtypesConfig.class, "");
+        assertThat(config.nestedarr(0).inner().name(), is("existing"));
+    }
+
+    @Test
+    public void non_existent_simple_map_in_payload_is_ignored() {
+        Slime slime = new Slime();
+        Cursor map = slime.setObject().setObject("non_existent_map");
+        map.setString("key","val");
+        MaptypesConfig config = new ConfigPayload(slime).toInstance(MaptypesConfig.class, "");
+        assertNotNull(config);
+    }
+
+    @Test
+    public void non_existent_map_of_struct_in_payload_is_ignored() {
+        Slime slime = new Slime();
+        Cursor map = slime.setObject().setObject("non_existent_inner_map");
+        map.setObject("one").setLong("foo", 1);
+        MaptypesConfig config = new ConfigPayload(slime).toInstance(MaptypesConfig.class, "");
+        assertNotNull(config);
+    }
+
+    @Test
+    public void non_existent_map_in_map_in_payload_is_ignored() {
+        Slime slime = new Slime();
+        Cursor map = slime.setObject().setObject("nestedmap").setObject("my-nested");
+        map.setObject("inner").setLong("one", 1);
+        map.setObject("non_existent").setLong("non-existent", 0);
+
+        MaptypesConfig config = new ConfigPayload(slime).toInstance(MaptypesConfig.class, "");
+        assertThat(config.nestedmap("my-nested").inner("one"), is(1));
+    }
+
+    @Test
+    public void test_function_test() {
+        // TODO: Test function test config as a complete config example
     }
 
     @Test
@@ -362,7 +481,7 @@ public class ConfigPayloadTest {
         assertThat(payload.toString(true), is("{\"boolval\":false,\"doubleval\":0.0,\"enumval\":\"VAL1\",\"intval\":0,\"longval\":0,\"stringval\":\"s\",\"newfield\":\"3\"}"));
     }
 
-    /**
+    /*
      * TODO: Test invalid slime trees?
      * TODO: Test sending in wrong class
      */
@@ -412,12 +531,6 @@ public class ConfigPayloadTest {
         }
     }
 
-    private StructtypesConfig createStructtypesConfigSimple(String name, String gender, String [] emails) {
-        Slime slime = new Slime();
-        addStructFields(slime.setObject().setObject("simple"), name, gender, emails);
-        return new ConfigPayload(slime).toInstance(StructtypesConfig.class, "");
-    }
-
     private StructtypesConfig createStructtypesConfigArray(String[] names, String[] genders) {
         Slime slime = new Slime();
         Cursor array = slime.setObject().setArray("simplearr");