[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 ---