Tapestry 5
  1. Tapestry 5
  2. TAP5-1510

The @Advise annotation limits advice to just a specific interface type

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.3, 5.2.5
    • Fix Version/s: 5.3
    • Component/s: tapestry-ioc
    • Labels:
      None

      Description

      @Advise requires that you specify a service interface (there's no default value). This is much more limiting than the advise method naming prefix, which will match all services (subject to the use of @Match), without regard to service interface.

      Further, inside ModuleImpl:

      private boolean markerMatched(ServiceDef serviceDef, Markable markable)
      {
      if (!serviceDef.getServiceInterface().equals(markable.getServiceInterface()))
      return false;;

      here, the Markable is the AdvisorDef2 instance generated from the @Advise annotation. This is an exact comparison; I believe this should be:

      if (! markable.getServiceInterface().isAssignableFrom(serviceDef.getServiceInterface())) return false;

      That, combined with a default of Object.class for @Advisor.serviceInterface would do the trick ... the @Advise.serviceInterface acts as an umbrella over any services' service interface.

      ..... ok, did more research and more stepping with the debugger. The above should be fixed, but it's only the second case of matching, the primary match should be based on the @Match annotation ... but that's broken too:

      Frrom DefaultModuleDefImpl:

      private <T extends Annotation> String[] extractPatterns(T annotation, String id, Method method)
      {
      if(annotation != null)
      return new String[]{};

      Match match = method.getAnnotation(Match.class);

      if (match == null)
      return new String[]

      { id }

      ;

      return match.value();
      }

      Here, the annotation is the @Advise annotation; I don't get why it returns empty string array; we should still see if there's a @Match annotation. Looking at the code, I can't see any reason why we would return that empty string array, the presense of the @Advise annotation (or for a decorator method, the @Decorate annotation) has no purpose I can figure out.

      In my situation, my advise method was not invoked because
      a) Primary check (by service id) failed, because the @Match annotation was ignored
      b) Secondary check (by service type and marker annotations) failed, because of inexact match on service interface

      So, the end result is the @Advise is only useful to advise a specific service interface, which is the opposite of what method advice is about ... it's supposed to match against a swath of services, adapting the advise to whatever methods are present in those services.

        Activity

        Howard M. Lewis Ship made changes -
        Affects Version/s 5.3 [ 12316024 ]
        Affects Version/s 5.3.0 [ 12316023 ]
        Fix Version/s 5.3 [ 12316024 ]
        Fix Version/s 5.3.0 [ 12316023 ]
        Hide
        Hudson added a comment -

        Integrated in tapestry-trunk-freestyle #353 (See https://builds.apache.org/hudson/job/tapestry-trunk-freestyle/353/)
        TAP5-1510: Added tests for decorators

        drobiazko : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1129336
        Files :

        • /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/internal/DecorateByMarkerModule2.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/IntegrationTest.java
        Show
        Hudson added a comment - Integrated in tapestry-trunk-freestyle #353 (See https://builds.apache.org/hudson/job/tapestry-trunk-freestyle/353/ ) TAP5-1510 : Added tests for decorators drobiazko : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1129336 Files : /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/internal/DecorateByMarkerModule2.java /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/IntegrationTest.java
        Igor Drobiazko made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Igor Drobiazko made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 5.3.0 [ 12316023 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in tapestry-trunk-freestyle #352 (See https://builds.apache.org/hudson/job/tapestry-trunk-freestyle/352/)
        TAP5-1510: The @Advise annotation limits advice to just a specific interface type

        drobiazko : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1128997
        Files :

        • /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/DefaultModuleDefImpl.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/internal/ModuleImplTest.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/annotations/Advise.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/test/IOCTestCase.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/internal/AdviseByMarkerModule2.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/IntegrationTest.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/ModuleImpl.java
        • /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/annotations/Decorate.java
        Show
        Hudson added a comment - Integrated in tapestry-trunk-freestyle #352 (See https://builds.apache.org/hudson/job/tapestry-trunk-freestyle/352/ ) TAP5-1510 : The @Advise annotation limits advice to just a specific interface type drobiazko : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1128997 Files : /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/DefaultModuleDefImpl.java /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/internal/ModuleImplTest.java /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/annotations/Advise.java /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/test/IOCTestCase.java /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/internal/AdviseByMarkerModule2.java /tapestry/tapestry5/trunk/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/IntegrationTest.java /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/ModuleImpl.java /tapestry/tapestry5/trunk/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/annotations/Decorate.java
        Igor Drobiazko made changes -
        Assignee Igor Drobiazko [ igor.drobiazko ]
        Hide
        Igor Drobiazko added a comment -

        It makes absoutely sense to change the logic of matching the service iterface to use:

        markable.getServiceInterface().isAssignableFrom(serviceDef.getServiceInterface()

        Regarding @Match: At the time of implementing @Advice or @Decorate annotations it made sense to me to ignore the @Match annotation when @Advice or @Decorate is present. For example in the following code you need to decide how to match the service: either by its service id

        @Match("Bla")
        @Advice(serviceInterface=Foo.class)
        public static void bla(...)

        { ...}

        But if we change the serviceInterfac() attribute of @Advice or @Decorate annotations to default to Object.class, then we could come up with something like:

        DefaultModuleDefImpl:
        private <T extends Annotation> String[] extractPatterns(Class serviceInterface, String id, Method method)
        {
        if(serviceInterface != Object.class)
        return new String[]{};

        Match match = method.getAnnotation(Match.class);

        if (match == null)
        return new String[]
        { id };

        return match.value();
        }

        What does it mean? If you didn't provide the service interface for @Advise or @Decorate, then you probably want to match by id. On the other hand, if you want to match a specific service interface than, @Match should be ignored as it doesn't make sense. The alternatives would be:

        @Match("Bla")
        @Advice
        public static void bla(...) { ...}

        @Advice(serviceInterface=Foo.class)
        public static void bla(...)

        { ...}

        Does it make sense?

        Show
        Igor Drobiazko added a comment - It makes absoutely sense to change the logic of matching the service iterface to use: markable.getServiceInterface().isAssignableFrom(serviceDef.getServiceInterface() Regarding @Match: At the time of implementing @Advice or @Decorate annotations it made sense to me to ignore the @Match annotation when @Advice or @Decorate is present. For example in the following code you need to decide how to match the service: either by its service id @Match("Bla") @Advice(serviceInterface=Foo.class) public static void bla(...) { ...} But if we change the serviceInterfac() attribute of @Advice or @Decorate annotations to default to Object.class, then we could come up with something like: DefaultModuleDefImpl: private <T extends Annotation> String[] extractPatterns(Class serviceInterface, String id, Method method) { if(serviceInterface != Object.class) return new String[]{}; Match match = method.getAnnotation(Match.class); if (match == null) return new String[] { id }; return match.value(); } What does it mean? If you didn't provide the service interface for @Advise or @Decorate, then you probably want to match by id. On the other hand, if you want to match a specific service interface than, @Match should be ignored as it doesn't make sense. The alternatives would be: @Match("Bla") @Advice public static void bla(...) { ...} @Advice(serviceInterface=Foo.class) public static void bla(...) { ...} Does it make sense?
        Howard M. Lewis Ship made changes -
        Field Original Value New Value
        Summary The @Advise annotation limits advise to just a specific interface type The @Advise annotation limits advice to just a specific interface type
        Howard M. Lewis Ship created issue -

          People

          • Assignee:
            Igor Drobiazko
            Reporter:
            Howard M. Lewis Ship
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development