Author: shawkins Date: 2010-09-27 16:39:44 -0400 (Mon, 27 Sep 2010) New Revision: 2616 Modified: branches/7.1.x/engine/src/main/java/org/teiid/query/function/FunctionTree.java branches/7.1.x/engine/src/main/java/org/teiid/query/function/SystemFunctionManager.java branches/7.1.x/engine/src/main/java/org/teiid/query/metadata/TransformationMetadata.java branches/7.1.x/engine/src/main/resources/org/teiid/query/i18n.properties branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionDescriptorImpl.java branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionTree.java Log: TEIID-1279 clarifying errors when loading udfs Modified: branches/7.1.x/engine/src/main/java/org/teiid/query/function/FunctionTree.java =================================================================== --- branches/7.1.x/engine/src/main/java/org/teiid/query/function/FunctionTree.java 2010-09-27 20:03:53 UTC (rev 2615) +++ branches/7.1.x/engine/src/main/java/org/teiid/query/function/FunctionTree.java 2010-09-27 20:39:44 UTC (rev 2616) @@ -72,46 +72,27 @@ * Function lookup and invocation use: Function name (uppercase) to Map (recursive tree) */ private Map treeRoot = new HashMap(); + private boolean validateClass; /** * Construct a new tree with the given source of function metadata. * @param source The metadata source */ public FunctionTree(FunctionMetadataSource source) { - // Load data structures - addSource(source); + this(source, false); } - + /** - * Construct a new tree with the given collection of sources. - * @param sources The collection of function metadata sources ({@link org.teiid.query.function.FunctionMetadataSource}) + * Construct a new tree with the given source of function metadata. + * @param source The metadata source */ - FunctionTree(Collection sources) { + public FunctionTree(FunctionMetadataSource source, boolean validateClass) { // Load data structures - addSources(sources); + this.validateClass = validateClass; + addSource(source); } /** - * Add a collection of functions to the data structures. - * @param sources The function metadata sources ({@link org.teiid.query.function.FunctionMetadataSource}) - */ - private void addSources(Collection sources) { - if(sources == null) { - return; - } - - Iterator sourceIter = sources.iterator(); - while(sourceIter.hasNext()) { - Object sourceObj = sourceIter.next(); - if(sourceObj instanceof FunctionMetadataSource) { - addSource((FunctionMetadataSource) sourceObj); - } else { - Assertion.failed(QueryPlugin.Util.getString("ERR.015.001.0044", sourceObj.getClass().getName())); //$NON-NLS-1$ - } - } - } - - /** * Add all functions from a metadata source to the data structures. * @param source The source of the functions */ @@ -294,7 +275,7 @@ // Defect 20007 - Ignore the invocation method if pushdown is not required. if (method.getPushdown() == FunctionMethod.CAN_PUSHDOWN || method.getPushdown() == FunctionMethod.CANNOT_PUSHDOWN) { try { - Class methodClass = source.getInvocationClass(method.getInvocationClass()); + Class<?> methodClass = source.getInvocationClass(method.getInvocationClass()); ReflectionHelper helper = new ReflectionHelper(methodClass); try { invocationMethod = helper.findBestMethodWithSignature(method.getInvocationMethod(), inputTypes); @@ -304,15 +285,31 @@ requiresContext = true; } } catch (ClassNotFoundException e) { - // Failed to load class, so can't load method - this will fail at invocation time. - // We don't fail here because this situation can occur in the modeler, which does - // not have the function jar files. The modeler never invokes, so this isn't a - // problem. + if (validateClass) { + throw new TeiidRuntimeException(e, "ERR.015.001.0047", QueryPlugin.Util.getString("FunctionTree.no_class", method.getName(), method.getInvocationClass())); //$NON-NLS-1$ //$NON-NLS-2$ + } + } catch (NoSuchMethodException e) { + throw new TeiidRuntimeException(e, "ERR.015.001.0047", QueryPlugin.Util.getString("FunctionTree.no_method", method, method.getInvocationClass(), method.getInvocationMethod())); //$NON-NLS-1$ //$NON-NLS-2$ } catch (Exception e) { - throw new TeiidRuntimeException(e, "ERR.015.001.0047", QueryPlugin.Util.getString("ERR.015.001.0047", new Object[]{method.getInvocationClass(), invocationMethod, inputTypes})); //$NON-NLS-1$ //$NON-NLS-2$ + throw new TeiidRuntimeException(e, "ERR.015.001.0047", QueryPlugin.Util.getString("ERR.015.001.0047", method, method.getInvocationClass(), method.getInvocationMethod())); //$NON-NLS-1$ //$NON-NLS-2$ } - if(invocationMethod != null && !FunctionTree.isValidMethod(invocationMethod)) { - throw new TeiidRuntimeException("ERR.015.001.0047", QueryPlugin.Util.getString("ERR.015.001.0047", new Object[]{method.getInvocationClass(), invocationMethod, inputTypes})); //$NON-NLS-1$ //$NON-NLS-2$ + if (invocationMethod != null) { + // Check return type is non void + Class<?> methodReturn = invocationMethod.getReturnType(); + if(methodReturn.equals(Void.TYPE)) { + throw new TeiidRuntimeException("ERR.015.001.0047", QueryPlugin.Util.getString("FunctionTree.not_void", method.getName(), invocationMethod)); //$NON-NLS-1$ //$NON-NLS-2$ + } + + // Check that method is public + int modifiers = invocationMethod.getModifiers(); + if(! Modifier.isPublic(modifiers)) { + throw new TeiidRuntimeException("ERR.015.001.0047", QueryPlugin.Util.getString("FunctionTree.not_public", method.getName(), invocationMethod)); //$NON-NLS-1$ //$NON-NLS-2$ + } + + // Check that method is static + if(! Modifier.isStatic(modifiers)) { + throw new TeiidRuntimeException("ERR.015.001.0047", QueryPlugin.Util.getString("FunctionTree.not_static", method.getName(), invocationMethod)); //$NON-NLS-1$ //$NON-NLS-2$ + } } } else { inputTypes.add(0, CommandContext.class); @@ -342,33 +339,6 @@ node.put(DESCRIPTOR_KEY, descriptor); } - /** - * Validate a method looked up by reflection. The method should have a non-void return type - * and be a public static method. - * @param method Method to validate - * @return True if valid - */ - static boolean isValidMethod(Method method) { - // Check return type is non void - Class methodReturn = method.getReturnType(); - if(methodReturn.equals(Void.TYPE)) { - return false; - } - - // Check that method is public - int modifiers = method.getModifiers(); - if(! Modifier.isPublic(modifiers)) { - return false; - } - - // Check that method is static - if(! Modifier.isStatic(modifiers)) { - return false; - } - - return true; - } - /** * Look up a function descriptor by signature in the tree. If none is * found, null is returned. Modified: branches/7.1.x/engine/src/main/java/org/teiid/query/function/SystemFunctionManager.java =================================================================== --- branches/7.1.x/engine/src/main/java/org/teiid/query/function/SystemFunctionManager.java 2010-09-27 20:03:53 UTC (rev 2615) +++ branches/7.1.x/engine/src/main/java/org/teiid/query/function/SystemFunctionManager.java 2010-09-27 20:39:44 UTC (rev 2616) @@ -46,7 +46,7 @@ System.err.println(QueryPlugin.Util.getString("ERR.015.001.0005", report)); //$NON-NLS-1$ } - systemFunctionTree = new FunctionTree(systemSource); + systemFunctionTree = new FunctionTree(systemSource, true); } Modified: branches/7.1.x/engine/src/main/java/org/teiid/query/metadata/TransformationMetadata.java =================================================================== --- branches/7.1.x/engine/src/main/java/org/teiid/query/metadata/TransformationMetadata.java 2010-09-27 20:03:53 UTC (rev 2615) +++ branches/7.1.x/engine/src/main/java/org/teiid/query/metadata/TransformationMetadata.java 2010-09-27 20:39:44 UTC (rev 2616) @@ -157,7 +157,7 @@ if (udfMethods == null) { udfMethods = Collections.emptyList(); } - this.functionLibrary = new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(udfMethods))); + this.functionLibrary = new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(udfMethods), true)); } //================================================================================== Modified: branches/7.1.x/engine/src/main/resources/org/teiid/query/i18n.properties =================================================================== --- branches/7.1.x/engine/src/main/resources/org/teiid/query/i18n.properties 2010-09-27 20:03:53 UTC (rev 2615) +++ branches/7.1.x/engine/src/main/resources/org/teiid/query/i18n.properties 2010-09-27 20:39:44 UTC (rev 2616) @@ -41,7 +41,12 @@ ERR.015.001.0044 = Function metadata source is of invalid type: {0} ERR.015.001.0045 = Function method is of invalid type: {0} ERR.015.001.0046 = The function "{0}" will not be added because a function with the same name and signature already exists. -ERR.015.001.0047 = Unexpected exception while loading {0}.{1} with args= {2} +ERR.015.001.0047 = Unexpected exception while loading "{1}.{2}" for UDF "{0}" +FunctionTree.not_void = UDF "{0}" method "{1}" must not return void. +FunctionTree.not_public = UDF "{0}" method "{1}" must be public. +FunctionTree.not_static = UDF "{0}" method "{1}" must be static. +FunctionTree.no_class = Could not load UDF "{0}", since its invocation class "{1}" could not be found. +FunctionTree.no_method = UDF "{0}" could not loaded, since no method on class "{1}" with name "{2}" has a matching type signature. ERR.015.001.0048 = Unable to represent average value from {0} / {1} ERR.015.001.0050 = Unable to compute aggregate function {0} on data of type {1} ERR.015.001.0052 = {0} must be non-null. Modified: branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionDescriptorImpl.java =================================================================== --- branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionDescriptorImpl.java 2010-09-27 20:03:53 UTC (rev 2615) +++ branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionDescriptorImpl.java 2010-09-27 20:39:44 UTC (rev 2616) @@ -24,15 +24,13 @@ import java.lang.reflect.Method; +import junit.framework.TestCase; + import org.teiid.core.types.DataTypeManager; import org.teiid.core.util.UnitTestUtil; -import org.teiid.query.function.FunctionDescriptor; -import org.teiid.query.function.FunctionTree; import org.teiid.query.function.metadata.FunctionMethod; -import junit.framework.TestCase; - public class TestFunctionDescriptorImpl extends TestCase { /** @@ -72,10 +70,6 @@ return null; } - // Validate method - if(! FunctionTree.isValidMethod(method)) { - return null; - } return method; } Modified: branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionTree.java =================================================================== --- branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionTree.java 2010-09-27 20:03:53 UTC (rev 2615) +++ branches/7.1.x/engine/src/test/java/org/teiid/query/function/TestFunctionTree.java 2010-09-27 20:39:44 UTC (rev 2616) @@ -22,96 +22,117 @@ package org.teiid.query.function; -import java.util.ArrayList; +import static org.junit.Assert.*; + import java.util.Arrays; import java.util.Collection; -import java.util.Iterator; -import java.util.List; -import junit.framework.TestCase; - +import org.junit.Test; import org.mockito.Mockito; +import org.teiid.core.TeiidRuntimeException; import org.teiid.core.types.DataTypeManager; -import org.teiid.logging.LogManager; -import org.teiid.query.function.FunctionForm; -import org.teiid.query.function.FunctionLibrary; -import org.teiid.query.function.FunctionMetadataSource; -import org.teiid.query.function.FunctionTree; -import org.teiid.query.function.SystemFunctionManager; -import org.teiid.query.function.UDFSource; import org.teiid.query.function.metadata.FunctionCategoryConstants; import org.teiid.query.function.metadata.FunctionMethod; import org.teiid.query.function.metadata.FunctionParameter; import org.teiid.query.function.source.SystemSource; +@SuppressWarnings("nls") +public class TestFunctionTree { -public class TestFunctionTree extends TestCase { - - // ################################## FRAMEWORK ################################ - - public TestFunctionTree(String name) { - super(name); - } - - // ################################## TEST HELPERS ################################ - - // ################################## ACTUAL TESTS ################################ - /** * Walk through all functions by metadata and verify that we can look * each one up by signature */ - public void testWalkTree() { + @Test public void testWalkTree() { SystemSource source = new SystemSource(); FunctionTree ft = new FunctionTree(source); - Collection categories = ft.getCategories(); - Iterator catIter = categories.iterator(); - while(catIter.hasNext()) { - String category = (String) catIter.next(); - LogManager.logInfo("test", "Category: " + category); //$NON-NLS-1$ //$NON-NLS-2$ - - Collection functions = ft.getFunctionForms(category); - Iterator functionIter = functions.iterator(); - while(functionIter.hasNext()) { - FunctionForm form = (FunctionForm) functionIter.next(); - LogManager.logInfo("test", "\tFunction: " + form.getDisplayString()); //$NON-NLS-1$ //$NON-NLS-2$ - } + Collection<String> categories = ft.getCategories(); + for (String category : categories) { + Collection<FunctionForm> functions = ft.getFunctionForms(category); + assertTrue(functions.size() > 0); } } - /** - * Test what happens when a function is loaded that does not have a class in the - * classpath. This *should* be ok as long as the function is not invoked. - */ - public void testUnloadableFunction() { - // Create dummy source - FunctionMetadataSource dummySource = new FunctionMetadataSource() { - public Collection getFunctionMethods() { - // Build dummy method - FunctionMethod method = new FunctionMethod( - "dummy", null, "no category", "nonexistentClass", "noMethod", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ - new FunctionParameter[0], - new FunctionParameter("output", DataTypeManager.DefaultDataTypes.STRING) ); //$NON-NLS-1$ + public String z() { + return null; + } + + protected static String x() { + return null; + } + + public static String y() { + return null; + } + + @Test public void testLoadErrors() { + FunctionMethod method = new FunctionMethod( + "dummy", null, null, FunctionMethod.CAN_PUSHDOWN, "nonexistentClass", "noMethod", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + new FunctionParameter[0], + new FunctionParameter("output", DataTypeManager.DefaultDataTypes.STRING)); //$NON-NLS-1$ + + //allowed, since we're not validating the class + new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(Arrays.asList(method)))); + + //should fail, no class + try { + new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(Arrays.asList(method)), true)); + fail(); + } catch (TeiidRuntimeException e) { + + } + + method.setInvocationClass(TestFunctionTree.class.getName()); + + //should fail, no method + try { + new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(Arrays.asList(method)), true)); + fail(); + } catch (TeiidRuntimeException e) { + + } + + method.setInvocationMethod("testLoadErrors"); + + //should fail, not void + try { + new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(Arrays.asList(method)), true)); + fail(); + } catch (TeiidRuntimeException e) { + + } + + method.setInvocationMethod("x"); + + //should fail, not public + try { + new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(Arrays.asList(method)), true)); + fail(); + } catch (TeiidRuntimeException e) { + + } + + method.setInvocationMethod("z"); + + //should fail, not static + try { + new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(Arrays.asList(method)), true)); + fail(); + } catch (TeiidRuntimeException e) { + + } - // Wrap method in a list - List methods = new ArrayList(); - methods.add(method); - return methods; - } - - public Class getInvocationClass(String className) throws ClassNotFoundException { - throw new ClassNotFoundException("Could not find class " + className); //$NON-NLS-1$ - } - }; + method.setInvocationMethod("y"); - new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(dummySource.getFunctionMethods()))); + //valid! + new FunctionLibrary(SystemFunctionManager.getSystemFunctions(), new FunctionTree(new UDFSource(Arrays.asList(method)), true)); } - public void testNullCategory() { + @Test public void testNullCategory() { FunctionMetadataSource fms = Mockito.mock(FunctionMetadataSource.class); Mockito.stub(fms.getFunctionMethods()).toReturn(Arrays.asList(new FunctionMethod( - "dummy", null, null, FunctionMethod.MUST_PUSHDOWN, "nonexistentClass", "noMethod", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ + "dummy", null, null, FunctionMethod.MUST_PUSHDOWN, "nonexistentClass", "noMethod", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ new FunctionParameter[0], new FunctionParameter("output", DataTypeManager.DefaultDataTypes.STRING) //$NON-NLS-1$ )));