inherited non public listener structure causing NPE

The problem appears to be with processing EventSetDescriptors. The OCI driver uses the T2CConnection whose super class PhysicalConnection contains add/remove event listener methods. The base class is package private and abstract. The thin driver uses T4CConnection and that class overrides the addXSEventListener methods, so that is likely why the thin driver does not exhibit the problem.

I was able to create some sample classes that reproduce the problem (Groovy 1.8.3+ and JDK 6 and 7):

1. unzip GroovyBugEventListeners.zip
2. cd GroovyBugEventListeners/
3. javac example/*
4. groovy AbstractBaseWithEventListeners.groovy

Note that if you either make FooBaseConnection public or change the event listeners from the FooEventListener interface to the java.util.EventListener you no longer get the NPE.

the error seems to appear in code from this:

        EventSetDescriptor[] eventDescriptors = info.getEventSetDescriptors();
        for (EventSetDescriptor descriptor : eventDescriptors) {
            Method[] listenerMethods = descriptor.getListenerMethods();
            for (Method listenerMethod : listenerMethods) {
                final MetaMethod metaMethod = CachedMethod.find(descriptor.getAddListenerMethod());
                addToAllMethodsIfPublic(metaMethod);
                String name = listenerMethod.getName();
                if (listeners.containsKey(name)) {
                    listeners.put(name, AMBIGUOUS_LISTENER_METHOD);
                } else {
                    listeners.put(name, metaMethod);
                }
            }
        }

because in addToAllMethodsIfPublic we do

    private void addToAllMethodsIfPublic(MetaMethod metaMethod) {
        if (Modifier.isPublic(metaMethod.getModifiers()))
            allMethods.add(metaMethod);
    }

And I think it is the metaMethod.getModifiers(), that is causing the NPE. A quick fix certainly would be to say we just skip the iteration step if we don't find the method and then not enter addToAllMethodsIfPublic at all. I guess for a non public listener it might be possible that you don't find such a method, especially in a subclass.

You are right, metaMethod is null so it is the getModifiers() generating the NPE. The descriptor.getAddListenerMethod() returns public void groovy.bugs.example.FooConnection.addFooEventListener(groovy.bugs.example.FooEventListener), but the CacheMethod#find(Method) lookup only includes methods from the declaring class in this case. By the time it reaches here, the method public void groovy.bugs.example.FooBaseConnection.addFooEventListener(groovy.bugs.example.FooEventListener) from the superclass has already been added to the all methods. So with a null check in place it just does not add the method name to the listeners map.

I checked out the GROOVY_1_8_2 tag and found that it works because methods in the code below includes the public methods from the abstract super class.

public static CachedMethod find(Method method) {
   CachedMethod[] methods = ReflectionCache.getCachedClass(method.getDeclaringClass()).getMethods();

So 1.8.2 methods is

1. public void groovy.bugs.example.FooConnection.addFooEventListener(groovy.bugs.example.FooEventListener)
2. public void groovy.bugs.example.FooConnection.removeFooEventListener(groovy.bugs.example.FooEventListener)
3. public java.lang.String groovy.bugs.example.FooConnection.someMethod()

And in master it is

1. public java.lang.String groovy.bugs.example.FooConnection.someMethod()

I updated the title to reflect the issue at hand better, since it is not really SQL specific

should be fixed now

I admit I don't know much about event listener handling and this is probably such an edge case it does not matter much, the null check is probably just fine. However, I was just curious about the problem so ran a git bisect to track the commit that caused this to no longer work (broke sometime after 1.8.2).

Commit 3c47089c9573df7d01227f07d3a6fac5886a61eb introduced this problem by no longer adding these types of methods to the cached class. To get a good view of the change you can run:

git show -w 3c47089c9573df7d01227f07d3a6fac5886a61eb

commit 3c47089c9573df7d01227f07d3a6fac5886a61eb
Author: Guillaume Laforge <glaforge@gmail.com>
Date:   Thu Sep 22 21:51:48 2011 +0200

    GROOVY-5003: Fix the problem in handling bridge methods

diff --git a/src/main/org/codehaus/groovy/reflection/CachedClass.java b/src/main/org/codehaus/groovy/reflection/CachedCl
index 139babb..ec6d4da 100644
--- a/src/main/org/codehaus/groovy/reflection/CachedClass.java
+++ b/src/main/org/codehaus/groovy/reflection/CachedClass.java
@@ -96,7 +96,7 @@ public class CachedClass {
                 final CachedMethod cachedMethod = new CachedMethod(CachedClass.this, declaredMethods[i]);
                 final String name = cachedMethod.getName();
 
-                if (name.indexOf('+') >= 0) {
+                if (declaredMethods[i].isBridge() || name.indexOf('+') >= 0) {
                     // Skip Synthetic methods inserted by JDK 1.5 compilers and later
                     continue;
                 } /*else if (Modifier.isAbstract(reflectionMethod.getModifiers())) {

The current fix that is probably fine, but I found it interesting since the inherited methods are public as part of the subclass and it seems as if they should be part of the cached class. But again, this is territory I'm not very familiar with so just wanted make note of what caused it break between versions 1.8.2 and 1.8.3.

Just read up on bridge methods and guess it does make sense to skip them.

Thanks for the fix.

I have test it with version 1.8.6 successfully. Thank you very much.