fix indexing bug in ValueFactory

[Minh-Ng]

fix ValueFactory.newMap(Entry<Value, Value>...pairs), which is currently iterating by 2 when it should be iterating through each passed in pair.

[xerial]

I believe the original code is already correct because [i * 2] index is already used. Could you add a test case so that we can confirm your fix is valid?

[Minh-Ng]

Test code on the project itself looks pretty light. In any case we can take a visual test case of
Map.Entry<? extends Value, ? extends Value> pairs = [Map.Entry<Value, Value> a, Map.Entry<Value, Value> b].

We want to process each pair, but the index you are referring to is filling the new array. So the for loop is actually trying to read non-existent indices of the pairs input array.

[Minh-Ng]

In other words try and replace the for loop condition with

int fillIndex = 0;
Value[] newArray = new Value[pairs.length * 2];
for (Map.Entry<? extends Value, ? extends Value> entry : pairs) {
     newArray[fillIndex] = entry.getKey();
     newArray[fillIndex + 1] = entry.getValue();
     fillIndex += 2;
}

In the original code, if you pairs is 2 Entry objects, the second pair will not get read because the second iteration will have condition (i = 2) < pairs.length, since pairs.length == 2, this statement is false and the second pair is not copied. As a result the items at kvs[2] and kvs[3] will be null.

[xerial]

ah. Ok. You are right. I was looking at the wrong code. We need to iterate the original input one by one.

If you can add a very simple test for this, it will be helpful.

[Minh-Ng]

Is it just me or there are no unit tests for src/test/java? I just came here from a nullpointerexception from an external library using messagepack core as a dependency. I'm not so familiar with this source.

[Minh-Ng]

Map.Entry<Value, Value> entry1 = new AbstractMap.SimpleImmutableEntry<>(ValueFactory.newInteger(10L), ValueFactory.newInteger(100L));
Map.Entry<Value, Value> entry2 = new AbstractMap.SimpleImmutableEntry<>(ValueFactory.newInteger(100L), ValueFactory.newInteger(10L));

MapValue newMap = ValueFactory.newMap(entry1, entry2);

String shouldNotThrowNPE = newMap.toString();
assertTrue(shouldNotThrowNPE != null && !"".equals(shouldNotThrowNPE));
assertEquals(2, newMap.asMapValue().entrySet().size())

[xerial]

Although some test is lacking, we should merge this fix. Thanks.