On May 3, 2010, at 12:29 PM, Jonathan Gallimore wrote:
> I've attached a new patch to Jira issue OPENEJB-1188. This seems to be
> working, and I'd like to get it committed soon. I'm going to change the
> format of the generated proxy name, and have another look at classloader
> code for the proxies over the next few days.
>
> If anyone has a chance to give it a spin, any feedback would be gratefully
> received.
Quick look over. Not digging thought the OpenEJB source and comparing, just
attempting to get at least some feedback using just memory.
> 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.
> 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.
> 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 :)
>
> 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.
> 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.
> 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.
> 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.
> 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.
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.
Anyway, fantastic development!
-David
