diff options
author | gjoranv <gv@oath.com> | 2018-12-14 16:04:41 +0100 |
---|---|---|
committer | gjoranv <gv@oath.com> | 2018-12-14 16:22:04 +0100 |
commit | 264d41c835c1e3f9cd64ff07d0e0d44caa17c28c (patch) | |
tree | 6fb2fe0c75b6b590f266bfda73e3c6c4762c6a9b /config | |
parent | 6e61798cac444b0dcebce79bce4f0284f446b2e7 (diff) |
Allow non-existent structs, arrays and maps in config payloads.
- User config is already validated against the def schema during
deploy, so we can assume that non-existent fields have been
added to a later version of the config class than the
application is using.
+ Add tests for complex data types in config payload.
+ Rename some methods and parameters for clarity.
Diffstat (limited to 'config')
-rw-r--r-- | config/src/main/java/com/yahoo/vespa/config/ConfigPayloadApplier.java | 64 | ||||
-rw-r--r-- | config/src/test/java/com/yahoo/vespa/config/ConfigPayloadTest.java | 132 |
2 files changed, 159 insertions, 37 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..271ce621d59 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,22 +373,35 @@ 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() + "'"); } + } /** @@ -390,14 +410,14 @@ public class ConfigPayloadApplier<T extends ConfigInstance.Builder> { * @param name a name * @return class found, or throws an exception is 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 028191cf844..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 @@ -263,7 +272,6 @@ public class ConfigPayloadTest { assertThat(config.simple().emails(1), is("bar@foo")); } - @Test public void test_simple_struct_arrays() throws Exception { StructtypesConfig config = createStructtypesConfigArray(new String[] { "foo", "bar" }, @@ -328,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 test_set_nonexistent_field() throws Exception { - createSimpletypesConfig("doesnotexist", "blabla"); + 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 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 @@ -365,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 */ |