Hi David, Thanks for going through this and coming back to me. I've put a few comments in line below. I've done some more hacking around with the names of the generated proxies so they are more meaningful and also created an AppClassLoader which is a simple extension of URLClassLoader that can have extra classes added to it. I've wired this into ClassLoaderUtil so each app should have an AppClassLoader, and generated proxies are added to the class loader for the relevant application.
I'll attach another patch to the JIRA with these changes. If its ok, I'll get this committed. Currently my build passes with no test failures and the itests pass as well. On Wed, May 5, 2010 at 5:05 AM, David Blevins <[email protected]>wrote: > > > Index: > container/openejb-core/src/main/java/org/apache/openejb/core/CoreDeploymentInfo.java > > =================================================================== > > + public Object getLocalBean() { > > + EjbObjectProxyHandler handler = null; > > + > > + switch(container.getContainerType()){ > > + case STATELESS: > > + handler = new StatelessEjbObjectHandler(this, null, > InterfaceType.LOCALBEAN, new ArrayList<Class>()); > > + break; > > + > > + case STATEFUL: > > + handler = new StatefulEjbObjectHandler(this, null, > InterfaceType.LOCALBEAN, new ArrayList<Class>()); > > + break; > > + > > + case SINGLETON: > > + handler = new StatelessEjbObjectHandler(this, null, > InterfaceType.LOCALBEAN, new ArrayList<Class>()); > > + break; > > + > > + } > > + > > + InvocationHandler invocationHandler = > handler.getInvocationHandler(); > > + Object proxy = > LocalBeanProxyFactory.newProxyInstance(this.getBeanClass(), > invocationHandler); > > + > > + return proxy; > > + } > > Do we need this method anymore? Seems this is covered by "public > BusinessLocalBeanHome getBusinessLocalBeanHome()". > > Certainly this will fail if someone actually calls it for a Stateful bean > as the primary key cannot be null on the proxy. > This is gone now, in favour of the BusinessLocalBeanHome code. > > > Index: > container/openejb-core/src/main/java/org/apache/openejb/core/interceptor/ReflectionInvocationContext.java > > =================================================================== > > --- > container/openejb-core/src/main/java/org/apache/openejb/core/interceptor/ReflectionInvocationContext.java > (revision 940447) > > +++ > container/openejb-core/src/main/java/org/apache/openejb/core/interceptor/ReflectionInvocationContext.java > (working copy) > > @@ -155,10 +155,25 @@ > > this.args = args; > > } > > public Object invoke() throws Exception { > > - Object value = method.invoke(target, args); > > + Method targetMethod = getMethod(target.getClass(), > method.getName(), method.getParameterTypes()); > > + targetMethod.setAccessible(true); > > + Object value = targetMethod.invoke(target, args); > > return value; > > } > > > > + private Method getMethod(Class<?> cls, String methodName, > Class<?>[] parameterTypes) throws NoSuchMethodException { > > + try { > > + Method method = cls.getDeclaredMethod(methodName, > parameterTypes); > > + return method; > > + } catch (NoSuchMethodException e) { > > + if (! (Object.class.equals(cls))) { > > + return getMethod(cls.getSuperclass(), methodName, > parameterTypes); > > + } > > + > > + throw e; > > + } > > + } > > Curious on what motivates the 'getMethod' method. Typically by this time > we've swapped out any "interface" method with the actual bean method by this > point. > I was running into some odd problems with this code where we couldn't find the method with the localbeans, as the method was on the superclass (or any of the classes that the localbean might inherit from), so it seemed to be a reasonable idea to just work up the hierarchy. I guess there might be a better way to do this - let me know if you think there is. > > > Index: > container/openejb-core/src/main/java/org/apache/openejb/core/security/AbstractSecurityService.java > > =================================================================== > > --- > container/openejb-core/src/main/java/org/apache/openejb/core/security/AbstractSecurityService.java > (revision 940447) > > +++ > container/openejb-core/src/main/java/org/apache/openejb/core/security/AbstractSecurityService.java > (working copy) > > @@ -259,6 +259,9 @@ > > String ejbName = deploymentInfo.getEjbName(); > > > > String name = (type == null)? null: type.getSpecName(); > > + if ("LocalBean".equals(name) || > "LocalBeanHome".equals(name)) { > > + name = null; > > + } > > > > Permission permission = new EJBMethodPermission(ejbName, > name, method); > > > > I'm not sure either what the appropriate string should be for LocalBean > views. Hopefully the TCK will let us know :) > There didn't seem to be one as far as I could see at the time - perhaps one is available in the JEE6 api? > > > > > Index: > container/openejb-core/src/main/java/org/apache/openejb/assembler/classic/JndiEncBuilder.java > > =================================================================== > > > + String jndiName = "openejb/Deployment/" + > JndiBuilder.format(referenceInfo.ejbDeploymentId, > referenceInfo.interfaceClassName, referenceInfo.localbean ? > InterfaceType.LOCALBEAN : InterfaceType.BUSINESS_LOCAL); > > Looking through this linearly, so this might be answered later in this > patch :) Do we have any code that validates that if someone has a local > bean ref to a bean, that that actual bean is exposing a localbean view? Not > critical to get in now, but definitely something we'd want. > We probably don't at the moment - I'll make a note and add some validation code. > > > > Index: > container/openejb-core/src/main/java/org/apache/openejb/assembler/classic/EjbResolver.java > > =================================================================== > > + boolean isLocalBean = false; > > + if (bean instanceof StatelessBeanInfo) { > > + StatelessBeanInfo statelessBean = (StatelessBeanInfo) bean; > > + if (statelessBean.localbean) { > > + isLocalBean = true; > > + } > > + } > > + if (bean instanceof StatefulBeanInfo) { > > + StatefulBeanInfo statefulBean = (StatefulBeanInfo) bean; > > + if (statefulBean.localbean) { > > + isLocalBean = true; > > + } > > + } > > + if (bean instanceof SingletonBeanInfo) { > > + SingletonBeanInfo singletonBeanInfo = (SingletonBeanInfo) > bean; > > + if (singletonBeanInfo.localbean) { > > + isLocalBean = true; > > + } > > + } > > Feel free to add a SessionBeanInfo and make these guys subclasses. Really > the ManagedBeanInfo and related should participate in the @LocalBean > concept, but unlike the others it should be "true" by default. > Will do. > > > Index: > container/openejb-core/src/main/java/org/apache/openejb/InterfaceType.java > > =================================================================== > > BUSINESS_LOCAL("Local"), > > + LOCALBEAN("LocalBean"), > > BUSINESS_LOCAL_HOME("LocalHome"), > > BUSINESS_REMOTE("Remote"), > > BUSINESS_REMOTE_HOME("Home"), > > + BUSINESS_LOCALBEAN_HOME("LocalBeanHome"), > > > + case BUSINESS_LOCALBEAN_HOME: return > InterfaceType.LOCALBEAN; > > + case LOCALBEAN: return > InterfaceType.BUSINESS_LOCALBEAN_HOME; > > > Feel free to go with just plain "LocalBeanHome" everywhere vs > "BusinessLocalBeanHome". Some places use "BusinessLocalBean" vs "LocalBean" > as well. Maybe just cut the "Business" part out across the board for > consistency. > Agreed. Think I'll see if we can do this across the board once I've got this committed. > > > Index: > server/openejb-ejbd/src/main/java/org/apache/openejb/server/ejbd/JndiRequestHandler.java > > =================================================================== > > try { > > handler = (BaseEjbProxyHandler) > ProxyManager.getInvocationHandler(object); > > } catch (Exception e) { > > - // Not a proxy. See if it's serializable and send it > > - if (object instanceof java.io.Serializable){ > > - res.setResponseCode(ResponseCodes.JNDI_OK); > > - res.setResult(object); > > - return; > > - } else { > > - > res.setResponseCode(ResponseCodes.JNDI_NAMING_EXCEPTION); > > - NamingException namingException = new > NamingException("Expected an ejb proxy, found unknown object: type=" + > object.getClass().getName() + ", toString=" + object); > > - res.setResult(new ThrowableArtifact(namingException)); > > - return; > > + try { > > + Field field = > object.getClass().getDeclaredField("invocationHandler"); > > + field.setAccessible(true); > > + handler = (BaseEjbProxyHandler) field.get(object); > > + } catch (Exception e1) { > > + // Not a proxy. See if it's serializable and send it > > + if (object instanceof java.io.Serializable){ > > + res.setResponseCode(ResponseCodes.JNDI_OK); > > + res.setResult(object); > > + return; > > Was going to make the comment earlier, but already deleted that chunk. If > we found some way to tuck the LocalBeanProxyManager code underneath the > ProxyManager interface, that'd definitely make code like the above cleaner. > No need to do that now, but maybe in a later revision. > > Will do. > > > Index: > examples/simple-stateless/src/test/java/org/superbiz/calculator/CalculatorTest.java > > =================================================================== > > --- > examples/simple-stateless/src/test/java/org/superbiz/calculator/CalculatorTest.java > (revision 940447) > > +++ > examples/simple-stateless/src/test/java/org/superbiz/calculator/CalculatorTest.java > (working copy) > > Will bring up examples in a new thread. With this functionality in, > there's almost no reason to use an explicit @Local interface. > > Overall looks superb! I'd just check it in (assuming it doesn't break the > build! :) and do the tweaks when you get time. > Thanks! As there's a couple more changes I'll pop those in a patch, but then get this committed if there's no problems. > > A note that you have a few commented out lines in the patch. Definitely > either yank those or add some comments on why it's being left in but > commented out. Something I need to get better at as well. I myself have at > times come across commented out code and thought "why is this still here" > only to find out via svn history that *I'm* the one who commented it out! > Of course at that point I'm far less confident about yanking it as I can't > remember why I left it in the first place. > LOL - thanks for pointing that out, I'll double before I commit to make sure they get removed. > > Anyway, fantastic development! > Thanks for all your feedback David, and thanks for the opportunity to work on this functionality - I've had a lot of fun working on it, and I've learnt an awful lot. Jon
