FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix indexing bug in ValueFactory by Minh-Ng · Pull Request #525 · msgpack/msgpack-java · GitHub

fix indexing bug in ValueFactory - #525

Merged
xerial merged 1 commit into
msgpack:developfrom
Minh-Ng:patch-1
Aug 31, 2020
Merged

fix indexing bug in ValueFactory#525
xerial merged 1 commit into
msgpack:developfrom
Minh-Ng:patch-1

Conversation

Minh-Ng commented Jun 23, 2020

Copy link
Copy Markdown
Contributor

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

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

xerial commented Jun 23, 2020

Copy link
Copy Markdown
Member

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 commented Jun 23, 2020

Copy link
Copy Markdown
Contributor Author

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 commented Jun 23, 2020
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Jun 23, 2020

Copy link
Copy Markdown
Member

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 commented Jun 23, 2020

Copy link
Copy Markdown
Contributor Author

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 commented Jun 23, 2020
edited
Loading

Copy link
Copy Markdown
Contributor Author
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 self-assigned this Jun 23, 2020
xerial merged commit 0348a49 into msgpack:develop Aug 31, 2020

xerial commented Aug 31, 2020

Copy link
Copy Markdown
Member

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

Minh-Ng deleted the patch-1 branch September 18, 2020 07:25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL