more fully test origin serialization and clean up code a bit

Mostly removing impossible codepaths.

config/src/main/java/com/typesafe/config/impl/SerializedConfigValue.java

@@ -63,7 +63,6 @@ class SerializedConfigValue extends AbstractConfigValue implements Externalizabl
         ORIGIN_TYPE,
         ORIGIN_URL,
         ORIGIN_COMMENTS,
-        ORIGIN_NULL_DESCRIPTION,
         ORIGIN_NULL_URL,
         ORIGIN_NULL_COMMENTS;
 
@@ -188,7 +187,6 @@ class SerializedConfigValue extends AbstractConfigValue implements Externalizabl
                 out.writeUTF(s);
             }
             break;
-        case ORIGIN_NULL_DESCRIPTION: // FALL THRU
         case ORIGIN_NULL_URL: // FALL THRU
         case ORIGIN_NULL_COMMENTS:
             // nothing to write out besides code and length
@@ -248,7 +246,6 @@ class SerializedConfigValue extends AbstractConfigValue implements Externalizabl
                 }
                 v = list;
                 break;
-            case ORIGIN_NULL_DESCRIPTION: // FALL THRU
             case ORIGIN_NULL_URL: // FALL THRU
             case ORIGIN_NULL_COMMENTS:
                 // nothing to read besides code and length

config/src/main/java/com/typesafe/config/impl/SimpleConfigOrigin.java

@@ -32,6 +32,8 @@ final class SimpleConfigOrigin implements ConfigOrigin {
 
     protected SimpleConfigOrigin(String description, int lineNumber, int endLineNumber,
             OriginType originType, String urlOrNull, List<String> commentsOrNull) {
+        if (description == null)
+            throw new ConfigException.BugOrBroken("description may not be null");
         this.description = description;
         this.lineNumber = lineNumber;
         this.endLineNumber = endLineNumber;
@@ -354,7 +356,6 @@ final class SimpleConfigOrigin implements ConfigOrigin {
     Map<SerializedField, Object> toFields() {
         Map<SerializedField, Object> m = new EnumMap<SerializedField, Object>(SerializedField.class);
 
-        if (description != null)
         m.put(SerializedField.ORIGIN_DESCRIPTION, description);
 
         if (lineNumber >= 0)
@@ -399,8 +400,8 @@ final class SimpleConfigOrigin implements ConfigOrigin {
                 // if field has been removed, we have to add a deletion entry
                 switch (f) {
                 case ORIGIN_DESCRIPTION:
-                    m.put(SerializedField.ORIGIN_NULL_DESCRIPTION, "");
+                    throw new ConfigException.BugOrBroken("origin missing description field? "
-                    break;
+                            + child);
                 case ORIGIN_LINE_NUMBER:
                     m.put(SerializedField.ORIGIN_LINE_NUMBER, -1);
                     break;
@@ -415,11 +416,11 @@ final class SimpleConfigOrigin implements ConfigOrigin {
                 case ORIGIN_COMMENTS:
                     m.put(SerializedField.ORIGIN_NULL_COMMENTS, "");
                     break;
-                case ORIGIN_NULL_DESCRIPTION: // FALL THRU
                 case ORIGIN_NULL_URL: // FALL THRU
                 case ORIGIN_NULL_COMMENTS:
-                    // inherit the deletion, nothing to do
+                    throw new ConfigException.BugOrBroken(
-                    break;
+                            "computing delta, base object should not contain " + f
+                            + " " + base);
                 case END_MARKER:
                 case ROOT_VALUE:
                 case ROOT_WAS_CONFIG:
@@ -428,6 +429,8 @@ final class SimpleConfigOrigin implements ConfigOrigin {
                 case VALUE_ORIGIN:
                     throw new ConfigException.BugOrBroken("should not appear here: " + f);
                 }
+            } else {
+                // field is in base and child, but differs, so leave it
             }
         }
 
@@ -459,35 +462,33 @@ final class SimpleConfigOrigin implements ConfigOrigin {
             if (delta.containsKey(f)) {
                 // delta overrides when keys are in both
                 // "m" should already contain the right thing
-            } else if (!delta.containsKey(f)) {
+            } else {
                 // base has the key and delta does not.
                 // we inherit from base unless a "NULL" key blocks.
                 switch (f) {
                 case ORIGIN_DESCRIPTION:
-                    // add to assembled unless delta nulls
-                    if (!delta.containsKey(SerializedField.ORIGIN_NULL_DESCRIPTION))
                     m.put(f, base.get(f));
                     break;
                 case ORIGIN_URL:
-                    if (!delta.containsKey(SerializedField.ORIGIN_NULL_URL))
+                    if (delta.containsKey(SerializedField.ORIGIN_NULL_URL)) {
+                        m.remove(SerializedField.ORIGIN_NULL_URL);
+                    } else {
                         m.put(f, base.get(f));
+                    }
                     break;
                 case ORIGIN_COMMENTS:
-                    if (!delta.containsKey(SerializedField.ORIGIN_NULL_COMMENTS))
+                    if (delta.containsKey(SerializedField.ORIGIN_NULL_COMMENTS)) {
-                        m.put(f, base.get(f));
+                        m.remove(SerializedField.ORIGIN_NULL_COMMENTS);
-                    break;
+                    } else {
-                case ORIGIN_NULL_DESCRIPTION:
-                    if (!delta.containsKey(SerializedField.ORIGIN_DESCRIPTION))
-                        m.put(f, base.get(f));
-                    break;
-                case ORIGIN_NULL_URL:
-                    if (!delta.containsKey(SerializedField.ORIGIN_URL))
-                        m.put(f, base.get(f));
-                    break;
-                case ORIGIN_NULL_COMMENTS:
-                    if (!delta.containsKey(SerializedField.ORIGIN_COMMENTS))
                         m.put(f, base.get(f));
+                    }
                     break;
+                case ORIGIN_NULL_URL: // FALL THRU
+                case ORIGIN_NULL_COMMENTS: // FALL THRU
+                    // base objects shouldn't contain these, should just
+                    // lack the field. these are only in deltas.
+                    throw new ConfigException.BugOrBroken(
+                            "applying fields, base object should not contain " + f + " " + base);
                 case ORIGIN_END_LINE_NUMBER: // FALL THRU
                 case ORIGIN_LINE_NUMBER: // FALL THRU
                 case ORIGIN_TYPE:

config/src/test/scala/com/typesafe/config/impl/ConfigValueTest.scala

@@ -750,4 +750,54 @@ class ConfigValueTest extends TestUtils {
         assertEquals(ResolveStatus.UNRESOLVED, obj.withoutKey("a").resolveStatus())
         assertEquals(ResolveStatus.RESOLVED, obj.withoutKey("a").withoutKey("b").resolveStatus())
     }
+
+    @Test
+    def configOriginsInSerialization() {
+        import scala.collection.JavaConverters._
+        val bases = Seq(
+            SimpleConfigOrigin.newSimple("foo"),
+            SimpleConfigOrigin.newFile("/tmp/blahblah"),
+            SimpleConfigOrigin.newURL(new URL("http://example.com")),
+            SimpleConfigOrigin.newResource("myresource"))
+        val combos = bases.flatMap({
+            base =>
+                Seq(
+                    (base, base.setComments(Seq("this is a comment", "another one").asJava)),
+                    (base, base.setComments(null)),
+                    (base, base.setLineNumber(41)),
+                    (base, SimpleConfigOrigin.mergeOrigins(base.setLineNumber(10), base.setLineNumber(20))))
+        }) ++
+            bases.sliding(2).map({ seq => (seq.head, seq.tail.head) }) ++
+            bases.sliding(3).map({ seq => (seq.head, seq.tail.tail.head) }) ++
+            bases.sliding(4).map({ seq => (seq.head, seq.tail.tail.tail.head) })
+        val withFlipped = combos ++ combos.map(_.swap)
+        val withDuplicate = withFlipped ++ withFlipped.map(p => (p._1, p._1))
+        val values = withDuplicate.flatMap({
+            combo =>
+                Seq(
+                    // second inside first
+                    new SimpleConfigList(combo._1,
+                        Seq[AbstractConfigValue](new ConfigInt(combo._2, 42, "42")).asJava),
+                    // triple-nested means we have to null then un-null then null, which is a tricky case
+                    // in the origin-serialization code.
+                    new SimpleConfigList(combo._1,
+                        Seq[AbstractConfigValue](new SimpleConfigList(combo._2,
+                            Seq[AbstractConfigValue](new ConfigInt(combo._1, 42, "42")).asJava)).asJava))
+        })
+        def top(v: SimpleConfigList) = v.origin
+        def middle(v: SimpleConfigList) = v.get(0).origin
+        def bottom(v: SimpleConfigList) = if (v.get(0).isInstanceOf[ConfigList])
+            Some(v.get(0).asInstanceOf[ConfigList].get(0).origin)
+        else
+            None
+
+        //System.err.println("values=\n  " + values.map(v => top(v).description + ", " + middle(v).description + ", " + bottom(v).map(_.description)).mkString("\n  "))
+        for (v <- values) {
+            val deserialized = checkSerializable(v)
+            // double-check that checkSerializable verified the origins
+            assertEquals(top(v), top(deserialized))
+            assertEquals(middle(v), middle(deserialized))
+            assertEquals(bottom(v), bottom(deserialized))
+        }
+    }
 }