[GitHub] storm pull request #2744: [STORM-3132] Avoid NPE in the Values Constructor
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2744#discussion_r225509252
--- Diff: storm-client/test/jvm/org/apache/storm/tuple/ValuesTest.java ---
@@ -0,0 +1,49 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.storm.tuple;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ValuesTest {
+
+@Test
+public void testNoArgsToValues() {
+Values vals = new Values();
+Assert.assertTrue("Failed to add null to Values", vals.size() ==
0);
+}
+
+@Test
+public void testNullArgsToValues() {
+Values vals = new Values(null);
+Assert.assertTrue("Failed to add null to Values", vals.size() ==
1);
--- End diff --
@HeartSaVioR, Make changes to unit tests to explicitly check elements in
values.
---
[GitHub] storm pull request #2744: [STORM-3132] Avoid NPE in the Values Constructor
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2744#discussion_r225509106
--- Diff: storm-client/src/jvm/org/apache/storm/tuple/Values.java ---
@@ -23,9 +23,13 @@ public Values() {
}
public Values(Object... vals) {
-super(vals.length);
-for (Object o : vals) {
-add(o);
+super(vals != null ? vals.length : 0);
--- End diff --
Making change
---
[GitHub] storm pull request #2744: [STORM-3132] Avoid NPE in the Values Constructor
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2744#discussion_r225330762
--- Diff: storm-client/src/jvm/org/apache/storm/tuple/Values.java ---
@@ -23,9 +23,13 @@ public Values() {
}
public Values(Object... vals) {
-super(vals.length);
-for (Object o : vals) {
-add(o);
+super(vals != null ? vals.length : 0);
--- End diff --
I think this can be `super(vals != null ? vals.length : 1);` since `if
(vals==null)` we are going to add `null` to it. The length will be 1.
---
[GitHub] storm pull request #2744: [STORM-3132] Avoid NPE in the Values Constructor
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2744#discussion_r202568702
--- Diff: storm-client/test/jvm/org/apache/storm/tuple/ValuesTest.java ---
@@ -0,0 +1,49 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.storm.tuple;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ValuesTest {
+
+@Test
+public void testNoArgsToValues() {
+Values vals = new Values();
+Assert.assertTrue("Failed to add null to Values", vals.size() ==
0);
+}
+
+@Test
+public void testNullArgsToValues() {
+Values vals = new Values(null);
+Assert.assertTrue("Failed to add null to Values", vals.size() ==
1);
--- End diff --
I'm sorry to comment this too late, but let's make sure checking elements
in values. Same applies to other tests.
---
[GitHub] storm pull request #2744: [STORM-3132] Avoid NPE in the Values Constructor
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2744#discussion_r201218268
--- Diff: storm-client/src/jvm/org/apache/storm/tuple/Values.java ---
@@ -23,9 +23,13 @@ public Values() {
}
public Values(Object... vals) {
-super(vals.length);
-for (Object o : vals) {
-add(o);
+super(vals != null ? vals.length : 0);
+if (vals != null && vals.length != 0 ) {
--- End diff --
`vals.length != 0` should be removed in this condition, cause condition for
else statement would be `vals == null || vals.length == 0`, which
`Values(List())` goes into else statement and comes out of `Values(null)`
instead of no element.
---
[GitHub] storm pull request #2744: [STORM-3132] Avoid NPE in the Values Constructor
GitHub user kishorvpatil opened a pull request: https://github.com/apache/storm/pull/2744 [STORM-3132] Avoid NPE in the Values Constructor `Values` construction could end up throwing NPE. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kishorvpatil/incubator-storm storm3132 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2744.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2744 commit e6f95fb92597216aae50a884d62d0ead6a16bbcd Author: Kishor Patil Date: 2018-06-29T05:48:43Z Avoid NPE in the Values Constructor ---
