tapestry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hls...@apache.org
Subject [6/6] git commit: TAP5-1736: Overriding a base class abstract event handler method causes the sub-class method to be invoked twice
Date Mon, 01 Sep 2014 23:18:46 GMT
TAP5-1736: Overriding a base class abstract event handler method causes the sub-class method
to be invoked twice


Project: http://git-wip-us.apache.org/repos/asf/tapestry-5/repo
Commit: http://git-wip-us.apache.org/repos/asf/tapestry-5/commit/df93764a
Tree: http://git-wip-us.apache.org/repos/asf/tapestry-5/tree/df93764a
Diff: http://git-wip-us.apache.org/repos/asf/tapestry-5/diff/df93764a

Branch: refs/heads/master
Commit: df93764ad8b5ecdda365f2c205e652692993e084
Parents: 8a10194
Author: Howard M. Lewis Ship <hlship@apache.org>
Authored: Mon Sep 1 16:18:48 2014 -0700
Committer: Howard M. Lewis Ship <hlship@apache.org>
Committed: Mon Sep 1 16:18:48 2014 -0700

----------------------------------------------------------------------
 .../internal/plastic/InheritanceData.java       | 51 ++++++++++++--------
 .../internal/plastic/PlasticClassImpl.java      | 11 +++--
 .../internal/plastic/PlasticMethodImpl.java     |  8 ++-
 .../apache/tapestry5/plastic/PlasticMethod.java | 21 +++++---
 .../tapestry5/plastic/MethodAdviceTests.groovy  | 39 +++++++++++++--
 .../java/testsubjects/AbstractPlaceholder.java  |  6 +++
 .../test/java/testsubjects/PlaceholderImpl.java |  8 +++
 .../integration/app1/CoreBehaviorsTests.java    |  9 +++-
 .../base/OverrideEventHandlerDemoBaseClass.java | 26 ++++++++++
 .../tapestry5/integration/app1/pages/Index.java |  2 +
 .../app1/pages/OverrideEventHandlerDemo.java    | 15 ++++++
 .../base/OverrideEventHandlerDemoBaseClass.tml  | 13 +++++
 12 files changed, 171 insertions(+), 38 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/InheritanceData.java
----------------------------------------------------------------------
diff --git a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/InheritanceData.java
b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/InheritanceData.java
index ab7a17f..44c1734 100644
--- a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/InheritanceData.java
+++ b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/InheritanceData.java
@@ -1,5 +1,3 @@
-// Copyright 2011 The Apache Software Foundation
-//
 // Licensed 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
@@ -52,49 +50,58 @@ public class InheritanceData
      * Returns a new MethodBundle that represents the methods of a child class
      * of this bundle. The returned bundle will always be {@linkplain #isTransformed() transformed}.
      *
-     * @param childClassName name of subclass
      * @return new method bundle
      */
-    public InheritanceData createChild(String childClassName)
+    public InheritanceData createChild()
     {
         return new InheritanceData(this);
     }
 
     /**
-     * Adds a new instance method. Only non-private, non-abstract methods should be added
(that is, methods which might
+     * Adds a new instance method. Only non-private methods should be added (that is, methods
which might
      * be overridden in subclasses). This can later be queried to see if any base class implements
the method.
      *
-     * @param name name of method
-     * @param desc method descriptor
+     * @param name
+     *         name of method
+     * @param desc
+     *         describes the parameters and return value of the method
      */
     public void addMethod(String name, String desc)
     {
-        String value = toValue(name, desc);
-
-        methods.add(value);
+        methods.add(toValue(name, desc));
         methodNames.add(name);
     }
 
+
     /**
-     * Returns true if a transformed parent class contains the indicated method.
+     * Returns true if a transformed parent class contains an implementation of, or abstract
placeholder for, the method.
      *
-     * @param name method name
-     * @param desc method descriptor
-     * @return the <em>internal name</em> of the implementing base class for
this method,
-     *         or null if no base class implements the method
+     * @param name
+     *         method name
+     * @param desc
+     *         method descriptor
+     * @return true if a base class implements the method (including abstract methods)
      */
     public boolean isImplemented(String name, String desc)
     {
         return checkForMethod(toValue(name, desc));
     }
 
-
     private boolean checkForMethod(String value)
     {
-        if (methods.contains(value))
-            return true;
+        InheritanceData cursor = this;
 
-        return parent == null ? false : parent.checkForMethod(value);
+        while (cursor != null)
+        {
+            if (cursor.methods.contains(value))
+            {
+                return true;
+            }
+
+            cursor = cursor.parent;
+        }
+
+        return false;
     }
 
     /**
@@ -118,8 +125,10 @@ public class InheritanceData
         return false;
     }
 
-    public void addInterface(String name) {
-        if (!interfaceNames.contains(name)) {
+    public void addInterface(String name)
+    {
+        if (!interfaceNames.contains(name))
+        {
             interfaceNames.add(name);
         }
     }

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
----------------------------------------------------------------------
diff --git a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
index e4e3707..c07bcaf 100644
--- a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
+++ b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
@@ -198,7 +198,8 @@ public class PlasticClassImpl extends Lockable implements PlasticClass,
Internal
                 pool.createAnnotationAccess(superClassName));
 
         this.parentInheritanceData = parentInheritanceData;
-        inheritanceData = parentInheritanceData.createChild(className);
+
+        inheritanceData = parentInheritanceData.createChild();
 
         for (String interfaceName : classNode.interfaces)
         {
@@ -233,7 +234,7 @@ public class PlasticClassImpl extends Lockable implements PlasticClass,
Internal
              */
             if (Modifier.isStatic(node.access))
             {
-                if (!Modifier.isPrivate(node.access))
+                if (isInheritableMethod(node))
                 {
                     inheritanceData.addMethod(node.name, node.desc);
                 }
@@ -782,8 +783,10 @@ public class PlasticClassImpl extends Lockable implements PlasticClass,
Internal
 
         methodNames.add(methodNode.name);
 
-        if (!Modifier.isPrivate(methodNode.access))
+        if (isInheritableMethod(methodNode))
+        {
             inheritanceData.addMethod(methodNode.name, methodNode.desc);
+        }
     }
 
     private PlasticMethod createNewMethod(MethodDescription description)
@@ -1183,7 +1186,7 @@ public class PlasticClassImpl extends Lockable implements PlasticClass,
Internal
 
     private boolean isInheritableMethod(MethodNode node)
     {
-        return (node.access & (ACC_ABSTRACT | ACC_PRIVATE)) == 0;
+        return !Modifier.isPrivate(node.access);
     }
 
     @Override

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticMethodImpl.java
----------------------------------------------------------------------
diff --git a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticMethodImpl.java
b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticMethodImpl.java
index 980bd84..bc3c14c 100644
--- a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticMethodImpl.java
+++ b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticMethodImpl.java
@@ -1,5 +1,3 @@
-// Copyright 2011 The Apache Software Foundation
-//
 // Licensed 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
@@ -85,6 +83,12 @@ class PlasticMethodImpl extends PlasticMember implements PlasticMethod,
Comparab
     }
 
     @Override
+    public boolean isAbstract()
+    {
+        return Modifier.isAbstract(node.access);
+    }
+
+    @Override
     public String getMethodIdentifier()
     {
         plasticClass.check();

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/plastic/src/main/java/org/apache/tapestry5/plastic/PlasticMethod.java
----------------------------------------------------------------------
diff --git a/plastic/src/main/java/org/apache/tapestry5/plastic/PlasticMethod.java b/plastic/src/main/java/org/apache/tapestry5/plastic/PlasticMethod.java
index f77b441..7ec9342 100644
--- a/plastic/src/main/java/org/apache/tapestry5/plastic/PlasticMethod.java
+++ b/plastic/src/main/java/org/apache/tapestry5/plastic/PlasticMethod.java
@@ -1,5 +1,3 @@
-// Copyright 2011 The Apache Software Foundation
-//
 // Licensed 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
@@ -48,7 +46,8 @@ public interface PlasticMethod extends AnnotationAccess
      * If the method has advice, the advice is <em>not</em> lost but will instead
wrap around the new method
      * implementation.
      *
-     * @param callback passed the InstructionBuilder so that an implementation of the method
can be created
+     * @param callback
+     *         passed the InstructionBuilder so that an implementation of the method can
be created
      * @return this method, for further configuration
      */
     PlasticMethod changeImplementation(InstructionBuilderCallback callback);
@@ -68,7 +67,8 @@ public interface PlasticMethod extends AnnotationAccess
      * Note additionally that a recursive method invocation will still invoke the MethodAdvice
chain on each recursive
      * call (this is an intended side-effect of copying the exact bytecode of the method
implementation.
      *
-     * @param advice advice to add to the method
+     * @param advice
+     *         advice to add to the method
      * @return this method, for further configuration
      */
     PlasticMethod addAdvice(MethodAdvice advice);
@@ -78,7 +78,8 @@ public interface PlasticMethod extends AnnotationAccess
      * correct interface (or extend the correct class). The original implementation of the
method is lost,
      * though (as with {@link #changeImplementation(InstructionBuilderCallback)}), method
advice is retained.
      *
-     * @param field to delegate to
+     * @param field
+     *         to delegate to
      * @return this method, for further configuration
      */
     PlasticMethod delegateTo(PlasticField field);
@@ -88,7 +89,8 @@ public interface PlasticMethod extends AnnotationAccess
      * is dynamically computed by another method of the class. The method should take no
parameters
      * and must not return null, or throw any exceptions not compatible with the method being
proxied.
      *
-     * @param method to provide the dynamic delegate
+     * @param method
+     *         to provide the dynamic delegate
      * @return this method, for further configuration
      */
     PlasticMethod delegateTo(PlasticMethod method);
@@ -107,6 +109,13 @@ public interface PlasticMethod extends AnnotationAccess
     boolean isOverride();
 
     /**
+     * Returns true if the method is abstract.
+     *
+     * @since 5.4
+     */
+    boolean isAbstract();
+
+    /**
      * Returns a short identifier for the method that includes the class name, the method
name,
      * and the types of all parameters. This is often used when producing debugging output
      * about the method.

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/plastic/src/test/groovy/org/apache/tapestry5/plastic/MethodAdviceTests.groovy
----------------------------------------------------------------------
diff --git a/plastic/src/test/groovy/org/apache/tapestry5/plastic/MethodAdviceTests.groovy
b/plastic/src/test/groovy/org/apache/tapestry5/plastic/MethodAdviceTests.groovy
index 727000e..bd20978 100644
--- a/plastic/src/test/groovy/org/apache/tapestry5/plastic/MethodAdviceTests.groovy
+++ b/plastic/src/test/groovy/org/apache/tapestry5/plastic/MethodAdviceTests.groovy
@@ -109,7 +109,7 @@ class MethodAdviceTests extends AbstractPlasticSpecification {
 
     def "setting return value clears checked exceptions"() {
         def mgr = createMgr({ PlasticClass pc ->
-            findMethod(pc, "maybeThrow").addAdvice({  MethodInvocation mi ->
+            findMethod(pc, "maybeThrow").addAdvice({ MethodInvocation mi ->
 
                 mi.proceed()
 
@@ -221,7 +221,9 @@ class MethodAdviceTests extends AbstractPlasticSpecification {
             })
         } as PlasticClassTransformer)
 
-        if (false) { enableBytecodeDebugging(mgr) }
+        if (false) {
+            enableBytecodeDebugging(mgr)
+        }
 
         def o = mgr.getClassInstantiator(testsubjects.FieldConduitInsideAdvisedMethod.name).with(MagicContainer,
container).newInstance()
 
@@ -256,7 +258,9 @@ class MethodAdviceTests extends AbstractPlasticSpecification {
             })
         } as PlasticClassTransformer)
 
-        if (false) { enableBytecodeDebugging(mgr) }
+        if (false) {
+            enableBytecodeDebugging(mgr)
+        }
 
         def o = mgr.getClassInstantiator(testsubjects.FieldConduitAdvisedMethodComplexCase.name).with(MagicContainer,
container).newInstance()
 
@@ -303,4 +307,33 @@ class MethodAdviceTests extends AbstractPlasticSpecification {
         o.magic() == "<<MAGIC!>>"
     }
 
+    def "abstract methods are identified as such by PlasticMethod"() {
+
+        setup:
+
+        PlasticClass pc = mgr.getPlasticClass(testsubjects.AbstractPlaceholder.name)
+
+        PlasticMethod m = findMethod pc, "placeholder"
+
+        expect:
+
+        m.isAbstract() == true
+    }
+
+    def "implementations of abstract methods are considered to be overrides"() {
+        setup:
+
+        def packages = ["testsubjects"] as Set
+
+        PlasticManager mgr = PlasticManager.withContextClassLoader().packages(packages).create()
+
+        PlasticClass pc = mgr.getPlasticClass(testsubjects.PlaceholderImpl.name)
+
+        PlasticMethod m = findMethod pc, "placeholder"
+
+        expect:
+
+        m.isOverride() == true
+    }
+
 }

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/plastic/src/test/java/testsubjects/AbstractPlaceholder.java
----------------------------------------------------------------------
diff --git a/plastic/src/test/java/testsubjects/AbstractPlaceholder.java b/plastic/src/test/java/testsubjects/AbstractPlaceholder.java
new file mode 100644
index 0000000..e18ae37
--- /dev/null
+++ b/plastic/src/test/java/testsubjects/AbstractPlaceholder.java
@@ -0,0 +1,6 @@
+package testsubjects;
+
+public abstract class AbstractPlaceholder
+{
+    public abstract void placeholder();
+}

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/plastic/src/test/java/testsubjects/PlaceholderImpl.java
----------------------------------------------------------------------
diff --git a/plastic/src/test/java/testsubjects/PlaceholderImpl.java b/plastic/src/test/java/testsubjects/PlaceholderImpl.java
new file mode 100644
index 0000000..a9d7ed3
--- /dev/null
+++ b/plastic/src/test/java/testsubjects/PlaceholderImpl.java
@@ -0,0 +1,8 @@
+package testsubjects;
+
+public class PlaceholderImpl extends AbstractPlaceholder
+{
+    public void placeholder()
+    {
+    }
+}

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/CoreBehaviorsTests.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/CoreBehaviorsTests.java
b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/CoreBehaviorsTests.java
index cc93804..bbae36a 100644
--- a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/CoreBehaviorsTests.java
+++ b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/CoreBehaviorsTests.java
@@ -1,5 +1,3 @@
-// Copyright 2009-2013 The Apache Software Foundation
-//
 // Licensed 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
@@ -1796,4 +1794,11 @@ public class CoreBehaviorsTests extends App1TestCase
             assertTrue(selenium.getLocation().endsWith(scenario.expectedUri));
         }
     }
+
+    @Test
+    public void event_handler_that_overrides_abstract_method_invoked_once() {
+        openLinks("Event Handler Override Demo", "Trigger");
+
+        assertTextSeries("//ul[@id='method-names']/li[%d]", 1, "sub-class", "DONE");
+    }
 }

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.java
b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.java
new file mode 100644
index 0000000..babfbb4
--- /dev/null
+++ b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.java
@@ -0,0 +1,26 @@
+package org.apache.tapestry5.integration.app1.base;
+
+import org.apache.tapestry5.annotations.Persist;
+import org.apache.tapestry5.annotations.Property;
+import org.apache.tapestry5.ioc.internal.util.CollectionFactory;
+
+import java.util.List;
+
+public abstract class OverrideEventHandlerDemoBaseClass
+{
+    @Persist
+    @Property
+    private List<String> methodNames;
+
+    protected void add(String methodName)
+    {
+        if (methodNames == null)
+        {
+            methodNames = CollectionFactory.newList();
+        }
+        
+        methodNames.add(methodName);
+    }
+
+    public abstract Object onActionFromTrigger();
+}

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/Index.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/Index.java
b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/Index.java
index ae49e59..f3df68f 100644
--- a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/Index.java
+++ b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/Index.java
@@ -56,6 +56,8 @@ public class Index
 
     private static final List<Item> ITEMS = CollectionFactory
             .newList(
+                    new Item("OverrideEventHandlerDemo", "Event Handler Override Demo", "Event
Handler methods overridden by sub-classes invoke base-class correctly."),
+
                     new Item("LogoSubclass", "Base class Assets in sub-classes", "Assets
are resolved for the parent class if that's where the annotations are."),
 
                     new Item("MissingRequiredARP", "Missing Query Parameter for @ActivationRequestParameter",
"Activating a page with a required @ActivationRequestParameter, but no matching query parameter,
is an error."),

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/OverrideEventHandlerDemo.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/OverrideEventHandlerDemo.java
b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/OverrideEventHandlerDemo.java
new file mode 100644
index 0000000..0d114bf
--- /dev/null
+++ b/tapestry-core/src/test/java/org/apache/tapestry5/integration/app1/pages/OverrideEventHandlerDemo.java
@@ -0,0 +1,15 @@
+package org.apache.tapestry5.integration.app1.pages;
+
+import org.apache.tapestry5.integration.app1.base.OverrideEventHandlerDemoBaseClass;
+
+public class OverrideEventHandlerDemo extends OverrideEventHandlerDemoBaseClass
+{
+    @Override
+    public Object onActionFromTrigger()
+    {
+        add("sub-class");
+        add("DONE");
+
+        return null;
+    }
+}

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/df93764a/tapestry-core/src/test/resources/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.tml
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/resources/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.tml
b/tapestry-core/src/test/resources/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.tml
new file mode 100644
index 0000000..9e8f696
--- /dev/null
+++ b/tapestry-core/src/test/resources/org/apache/tapestry5/integration/app1/base/OverrideEventHandlerDemoBaseClass.tml
@@ -0,0 +1,13 @@
+<html t:type="Border" xmlns:t="http://tapestry.apache.org/schema/tapestry_5_3.xsd">
+
+    <h1>Event Handler Override Demo</h1>
+
+
+    <ul id="method-names">
+        <li t:type="loop" source="methodNames" value="var:methodName">${var:methodName}</li>
+    </ul>
+
+    <t:actionlink class="btn btn-primary" t:id="trigger">Trigger</t:actionlink>
+
+</html>
+


Mime
View raw message