Fix behavior when merging object,primitive,object.

The AbstractConfigObject.merge() method was wrong; it merged in the
wrong order, which produced incorrect results. But there was a test
verifying that the results were wrong! So it's OK right?
Anyway, fix the test, fix the code, be sure it's crystal clear in spec.
This commit is contained in:
Havoc Pennington 2011-11-16 22:07:14 -05:00
parent e1329e54bf
commit 9dc6f77078
3 changed files with 68 additions and 9 deletions

View File

@ -150,7 +150,12 @@ To merge objects:
these same rules.
Object merge can be prevented by setting the key to another value
first.
first. This is because merging is always done two values at a
time; if you set a key to an object, a non-object, then an object,
first the non-object falls back to the object (non-object always
wins), and then the object falls back to the non-object (no
merging, object is the new value). So the two objects never see
each other.
These two are equivalent:
@ -757,6 +762,48 @@ might wish to support both the SI power of ten units and the IEC
power of two units. But until an implementation needs that, no
such thing is documented here.)
### Config object merging and file merging
It may be useful to offer a method to merge two objects. If such a
method is provided, it should work as if the two objects were
duplicate values for the same key in the same file. (See the
section earlier on duplicate key handling.)
As with duplicate keys, an intermediate non-object value "hides"
earlier object values. So say you merge three objects in this
order:
- `{ a : { x : 1 } }` (first priority)
- `{ a : 42 }` (fallback)
- `{ a : { y : 2 } }` (another fallback)
The result would be `{ a : { x : 1 } }`. The two objects are not
merged because they are not "adjacent"; the merging is done in
pairs, and when `42` is paired with `{ y : 2 }`, `42` simply wins
and loses all information about what it overrode.
But if you re-ordered like this:
- `{ a : { x : 1 } }` (first priority)
- `{ a : { y : 2 } }` (fallback)
- `{ a : 42 }` (another fallback)
Now the result would be `{ a : { x : 1, y : 2 } }` because the two
objects are adjacent.
This rule for merging objects loaded from different files is
_exactly_ the same behavior as for merging duplicate fields in the
same file. All merging works the same way.
Needless to say, normally it's well-defined whether a config
setting is supposed to be a number or an object. This kind of
weird pathology where the two are mixed should not be happening.
The one place where it matters, though, is that it allows you to
"clear" an object and start over by setting it to null and then
setting it back to a new object. So this behavior gives people a
way to get rid of default fallback values they don't want.
### Java properties mapping
It may be useful to merge Java properties data with data loaded

View File

@ -7,6 +7,7 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
@ -273,9 +274,15 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
} else if (stack.size() == 1) {
return stack.get(0);
} else {
AbstractConfigObject merged = stack.get(0);
for (int i = 1; i < stack.size(); ++i) {
merged = merged.withFallback(stack.get(i));
// to be consistent with the semantics of duplicate keys
// in the same file, we have to go backward like this.
// importantly, a primitive value always permanently
// hides a previous object value.
ListIterator<AbstractConfigObject> i = stack.listIterator(stack
.size());
AbstractConfigObject merged = i.previous();
while (i.hasPrevious()) {
merged = i.previous().withFallback(merged);
}
return merged;
}

View File

@ -188,9 +188,9 @@ class ConfigTest extends TestUtils {
@Test
def mergeObjectThenPrimitiveThenObject() {
// the semantic here is that the primitive gets ignored, because
// it can't be merged with the object. But potentially it should
// throw an exception even, or warn.
// the semantic here is that the primitive blocks the
// object that occurs at lower priority. This is consistent
// with duplicate keys in the same file.
val obj1 = parseObject("""{ "a" : { "b" : 42 } }""")
val obj2 = parseObject("""{ "a" : 2 }""")
val obj3 = parseObject("""{ "a" : { "b" : 43, "c" : 44 } }""")
@ -198,8 +198,13 @@ class ConfigTest extends TestUtils {
val merged = merge(obj1, obj2, obj3)
assertEquals(42, merged.getInt("a.b"))
assertEquals(1, merged.size)
assertEquals(44, merged.getInt("a.c"))
assertEquals(2, merged.getObject("a").size())
assertEquals(1, merged.getObject("a").size())
val merged2 = merge(obj3, obj2, obj1)
assertEquals(43, merged2.getInt("a.b"))
assertEquals(44, merged2.getInt("a.c"))
assertEquals(1, merged2.size)
assertEquals(2, merged2.getObject("a").size())
}
@Test