From 33ca6c1c010ed29dd7f1df17cd81023fba438e86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stig=20Rohde=20D=C3=B8ssing?= Date: Mon, 28 May 2018 22:47:15 +0200 Subject: [PATCH] STORM-3087: Make FluxBuilder.canInvokeWithArgs check whether the actual argument type is assignable to Number before deciding that a method with a primitive parameter can be invoked --- .../org/apache/storm/flux/FluxBuilder.java | 6 +- .../java/org/apache/storm/flux/TCKTest.java | 39 ++++++++++--- .../org/apache/storm/flux/test/TestBolt.java | 4 ++ .../configs/bad_static_factory_test.yaml | 56 +++++++++++++++++++ 4 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 external/flux/flux-core/src/test/resources/configs/bad_static_factory_test.yaml diff --git a/external/flux/flux-core/src/main/java/org/apache/storm/flux/FluxBuilder.java b/external/flux/flux-core/src/main/java/org/apache/storm/flux/FluxBuilder.java index 20bde18390c..0047fbade57 100644 --- a/external/flux/flux-core/src/main/java/org/apache/storm/flux/FluxBuilder.java +++ b/external/flux/flux-core/src/main/java/org/apache/storm/flux/FluxBuilder.java @@ -550,7 +550,7 @@ private static Method findCompatibleMethod(List args, Class target, Stri for (Method method : methods) { Class[] paramClasses = method.getParameterTypes(); if (paramClasses.length == args.size() && method.getName().equals(methodName)) { - LOG.debug("found constructor with same number of args.."); + LOG.debug("found method with same number of args."); boolean invokable = false; if (args.size() == 0) { // it's a method with zero args @@ -691,6 +691,10 @@ private static boolean canInvokeWithArgs(List args, Class[] parameterTyp LOG.debug("Yes, assignment is possible."); } else if (isPrimitiveNumber(paramType) || Number.class.isAssignableFrom(paramType) && Number.class.isAssignableFrom(objectType)) { + LOG.debug("Param is a number, checking whether argument can be coerced"); + if (!Number.class.isAssignableFrom(objectType)) { + return false; + } LOG.debug("Yes, assignment is possible."); } else if (paramType.isEnum() && objectType.equals(String.class)) { LOG.debug("Yes, will convert a String to enum"); diff --git a/external/flux/flux-core/src/test/java/org/apache/storm/flux/TCKTest.java b/external/flux/flux-core/src/test/java/org/apache/storm/flux/TCKTest.java index ee8089b4060..5ad465e5b00 100644 --- a/external/flux/flux-core/src/test/java/org/apache/storm/flux/TCKTest.java +++ b/external/flux/flux-core/src/test/java/org/apache/storm/flux/TCKTest.java @@ -33,8 +33,14 @@ import java.util.Collections; import java.util.Properties; +import org.junit.Rule; +import org.junit.rules.ExpectedException; public class TCKTest { + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Test public void testTCK() throws Exception { TopologyDef topologyDef = FluxParser.parseResource("/configs/tck.yaml", false, true, null, false); @@ -55,14 +61,16 @@ public void testShellComponents() throws Exception { topology.validate(); } - @Test(expected = IllegalArgumentException.class) + @Test public void testBadShellComponents() throws Exception { TopologyDef topologyDef = FluxParser.parseResource("/configs/bad_shell_test.yaml", false, true, null, false); Config conf = FluxBuilder.buildConfig(topologyDef); ExecutionContext context = new ExecutionContext(topologyDef, conf); - StormTopology topology = FluxBuilder.buildTopology(context); - assertNotNull(topology); - topology.validate(); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Unable to find configuration method"); + + FluxBuilder.buildTopology(context); } @Test @@ -117,14 +125,16 @@ public void testHbase() throws Exception { topology.validate(); } - @Test(expected = IllegalArgumentException.class) + @Test public void testBadHbase() throws Exception { TopologyDef topologyDef = FluxParser.parseResource("/configs/bad_hbase.yaml", false, true, null, false); Config conf = FluxBuilder.buildConfig(topologyDef); ExecutionContext context = new ExecutionContext(topologyDef, conf); - StormTopology topology = FluxBuilder.buildTopology(context); - assertNotNull(topology); - topology.validate(); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Couldn't find a suitable constructor"); + + FluxBuilder.buildTopology(context); } @Test @@ -268,4 +278,17 @@ public void testVariableSubstitution() throws Exception { is(context.getTopologyDef().getConfig().get("list.property.target"))); } + + @Test + public void testTopologyWithInvalidStaticFactoryArgument() throws Exception { + //STORM-3087. + TopologyDef topologyDef = FluxParser.parseResource("/configs/bad_static_factory_test.yaml", false, true, null, false); + Config conf = FluxBuilder.buildConfig(topologyDef); + ExecutionContext context = new ExecutionContext(topologyDef, conf); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Couldn't find a suitable static method"); + + FluxBuilder.buildTopology(context); + } } diff --git a/external/flux/flux-core/src/test/java/org/apache/storm/flux/test/TestBolt.java b/external/flux/flux-core/src/test/java/org/apache/storm/flux/test/TestBolt.java index 7792111be2d..9f38194d151 100644 --- a/external/flux/flux-core/src/test/java/org/apache/storm/flux/test/TestBolt.java +++ b/external/flux/flux-core/src/test/java/org/apache/storm/flux/test/TestBolt.java @@ -147,4 +147,8 @@ public static TestBolt newInstance() { public static TestBolt newInstance(TestEnum te){ return new TestBolt(te); } + + public static TestBolt newLongInstance(long l) { + return new TestBolt(l); + } } diff --git a/external/flux/flux-core/src/test/resources/configs/bad_static_factory_test.yaml b/external/flux/flux-core/src/test/resources/configs/bad_static_factory_test.yaml new file mode 100644 index 00000000000..5f9b9d80bf7 --- /dev/null +++ b/external/flux/flux-core/src/test/resources/configs/bad_static_factory_test.yaml @@ -0,0 +1,56 @@ +# 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. +--- +name: "yaml-topology" + +# +config: + topology.workers: 1 + # ... + +# spout definitions +spouts: + - id: "spout-1" + className: "org.apache.storm.testing.TestWordSpout" + parallelism: 1 + # ... + +# bolt definitions +bolts: + - id: "bolt-1" + className: "org.apache.storm.flux.test.TestBolt" + factory: "newLongInstance" + factoryArgs: + - "This is not a valid parameter, so should trigger an error" + parallelism: 1 +#stream definitions +# stream definitions define connections between spouts and bolts. +# note that such connections can be cyclical +streams: + - name: "spout-1 --> bolt-1" # name isn't used (placeholder for logging, UI, etc.) +# id: "connection-1" + from: "spout-1" + to: "bolt-1" + grouping: + type: SHUFFLE + + + + + + + +