MyFaces Core
  1. MyFaces Core
  2. MYFACES-2509

@ListenerFor not processed for global system events

    Details

    • Type: New Feature New Feature
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: None
    • Component/s: JSR-314
    • Labels:
      None

      Description

      We currently don't process @ListenerFor and @ListenerFor annotations on non-component types.

      This is specified in the spec for global system events, see:
      http://java.sun.com/javaee/javaserverfaces/2.0/docs/api/index.html?javax/faces/event/ListenerFor.html

      It would be nice to have it in the beta.

      1. MYFACES-2509-1.patch
        7 kB
        Leonardo Uribe
      2. ListenerFor_support.patch
        3 kB
        Jan-Kees van Andel

        Issue Links

          Activity

          Jan-Kees van Andel created issue -
          Hide
          Jan-Kees van Andel added a comment -

          the patch with the fix

          Show
          Jan-Kees van Andel added a comment - the patch with the fix
          Jan-Kees van Andel made changes -
          Field Original Value New Value
          Attachment ListenerFor_support.patch [ 12431332 ]
          Hide
          Leonardo Uribe added a comment -

          The javadoc says:

          "....The default implementation must support attaching this annotation to UIComponent or Renderer classes. In both cases, the annotation processing described herein must commence during the implementation of any variant of Application.createComponent() and must complete before the UIComponent instance is returned from createComponent(). The annotation processing must proceed according to an algorithm semantically equivalent to the following...."

          We are doing that right now, but the code that scan for it is in other place (see ApplicationImpl class). It is not necessary to apply this patch (or at least I cannot see a valid use case based on the javadocs). It could be good to know if there are other valid cases different to the described ones in the javadoc.

          Show
          Leonardo Uribe added a comment - The javadoc says: "....The default implementation must support attaching this annotation to UIComponent or Renderer classes. In both cases, the annotation processing described herein must commence during the implementation of any variant of Application.createComponent() and must complete before the UIComponent instance is returned from createComponent(). The annotation processing must proceed according to an algorithm semantically equivalent to the following...." We are doing that right now, but the code that scan for it is in other place (see ApplicationImpl class). It is not necessary to apply this patch (or at least I cannot see a valid use case based on the javadocs). It could be good to know if there are other valid cases different to the described ones in the javadoc.
          Hide
          Jan-Kees van Andel added a comment -

          Hrm, it looks like the JavaDoc can be interpreted in multiple ways.

          What I read, is that it should be possible for classes that implement SystemEventListener, to listen to SystemEvents.

          For example: http://java.sun.com/javaee/javaserverfaces/2.0/docs/api/javax/faces/event/PostConstructApplicationEvent.html

          The current implementation cannot deal with this event, because it occurs before the ApplicationImpl code is invoked (there is no component tree at that time). For this reason, we need to check it @startup.

          But because its so ambiguous, I suggest to leave it out of the beta release.

          Show
          Jan-Kees van Andel added a comment - Hrm, it looks like the JavaDoc can be interpreted in multiple ways. What I read, is that it should be possible for classes that implement SystemEventListener, to listen to SystemEvents. For example: http://java.sun.com/javaee/javaserverfaces/2.0/docs/api/javax/faces/event/PostConstructApplicationEvent.html The current implementation cannot deal with this event, because it occurs before the ApplicationImpl code is invoked (there is no component tree at that time). For this reason, we need to check it @startup. But because its so ambiguous, I suggest to leave it out of the beta release.
          Hide
          Michael Concini added a comment -

          If there are no objections Jan-Kees, can you go ahead and commit this fix now that we are past the beta? My personal opinion after reading through the javadocs is that we should support this. Even if it is a little ambiguous, if it is possible that users may interpret this as valid then I'd rather err on the side of supporting it.

          I'd also be happy to commit the change as well if you'd rather.

          Show
          Michael Concini added a comment - If there are no objections Jan-Kees, can you go ahead and commit this fix now that we are past the beta? My personal opinion after reading through the javadocs is that we should support this. Even if it is a little ambiguous, if it is possible that users may interpret this as valid then I'd rather err on the side of supporting it. I'd also be happy to commit the change as well if you'd rather.
          Hide
          Jan-Kees van Andel added a comment -

          I don't mind to commit it, but I currently don't have a proper local build so I can't do a proper pre-commit test with the latest updates...

          If you would like to commit it Mike, that would be great.

          Also, Leonardo had a good point. And this might impact the TCK, so we might put it on the (short) TODO list or ask it on the EG list....

          Show
          Jan-Kees van Andel added a comment - I don't mind to commit it, but I currently don't have a proper local build so I can't do a proper pre-commit test with the latest updates... If you would like to commit it Mike, that would be great. Also, Leonardo had a good point. And this might impact the TCK, so we might put it on the (short) TODO list or ask it on the EG list....
          Hide
          Jan-Kees van Andel added a comment -

          Patch committed

          Show
          Jan-Kees van Andel added a comment - Patch committed
          Jan-Kees van Andel made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 2.0.0-beta-3 [ 12314776 ]
          Resolution Fixed [ 1 ]
          Hide
          Leonardo Uribe added a comment -

          I have to reopen this issue. The reason is really I don't believe this code works, or anyone has tested it fully. Look this code carefully:

          private void processListenerFor(Application application, Class clazz, ListenerFor listenerFor)
          {
          try
          {
          if (SystemEventListener.class.isAssignableFrom(clazz))
          {
          Class sourceClass = listenerFor.sourceClass();
          Class<? extends SystemEvent> systemEventClass = listenerFor.systemEventClass();
          if (Void.class.equals(sourceClass))

          { application.subscribeToEvent(systemEventClass, (SystemEventListener) clazz.newInstance()); }

          else

          { application.subscribeToEvent(systemEventClass, sourceClass, (SystemEventListener) clazz.newInstance()); }

          }
          }
          catch (InstantiationException e)

          { throw new FacesException(e); }
          catch (IllegalAccessException e)
          { throw new FacesException(e); }

          }

          Do you see the clazz.newInstance()? This works for classes that does not have state, but what happen if we need some variable. Since the "real" is created somewhere else, the value will not be right.

          If we want to make this work, we have to scan for annotations after the instantiation and if we have a instance wrapping, we should scan properly. What happen if some user has a @ListenerFor for a PhaseListener? What happen is some user has a @ListenerFor for a Application/ViewHandler/StateManager instance? Do we need to do something for CDI?

          The spec is silent about that, so the right way to solve this one is propose an algorithm and submit is first to jsr-314-open mail list, so the experts can take a look and decide what to do. Maybe we can propose something and do it on myfaces directly, since we are on beta yet, but the intention about do all this bureaucracy is keep as close with the spec as possible or in other words, make applications interchageable between jsf implementations.

          Unfortunately, I have to revert this code, because I just can't do another beta release with this code on it.

          Show
          Leonardo Uribe added a comment - I have to reopen this issue. The reason is really I don't believe this code works, or anyone has tested it fully. Look this code carefully: private void processListenerFor(Application application, Class clazz, ListenerFor listenerFor) { try { if (SystemEventListener.class.isAssignableFrom(clazz)) { Class sourceClass = listenerFor.sourceClass(); Class<? extends SystemEvent> systemEventClass = listenerFor.systemEventClass(); if (Void.class.equals(sourceClass)) { application.subscribeToEvent(systemEventClass, (SystemEventListener) clazz.newInstance()); } else { application.subscribeToEvent(systemEventClass, sourceClass, (SystemEventListener) clazz.newInstance()); } } } catch (InstantiationException e) { throw new FacesException(e); } catch (IllegalAccessException e) { throw new FacesException(e); } } Do you see the clazz.newInstance()? This works for classes that does not have state, but what happen if we need some variable. Since the "real" is created somewhere else, the value will not be right. If we want to make this work, we have to scan for annotations after the instantiation and if we have a instance wrapping, we should scan properly. What happen if some user has a @ListenerFor for a PhaseListener? What happen is some user has a @ListenerFor for a Application/ViewHandler/StateManager instance? Do we need to do something for CDI? The spec is silent about that, so the right way to solve this one is propose an algorithm and submit is first to jsr-314-open mail list, so the experts can take a look and decide what to do. Maybe we can propose something and do it on myfaces directly, since we are on beta yet, but the intention about do all this bureaucracy is keep as close with the spec as possible or in other words, make applications interchageable between jsf implementations. Unfortunately, I have to revert this code, because I just can't do another beta release with this code on it.
          Leonardo Uribe made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Leonardo Uribe made changes -
          Fix Version/s 2.0.0-beta-3 [ 12314776 ]
          Hide
          Jan-Kees van Andel added a comment - - edited

          I think you're right.

          I initially only considered it to be used for startup listeners, and I don't think those classes should contain state, but on the other hand, we shouldn't have such a silent, undocumented requirement. This will indeed cause weird bugs.

          Show
          Jan-Kees van Andel added a comment - - edited I think you're right. I initially only considered it to be used for startup listeners, and I don't think those classes should contain state, but on the other hand, we shouldn't have such a silent, undocumented requirement. This will indeed cause weird bugs.
          Leonardo Uribe made changes -
          Attachment MYFACES-2509-1.patch [ 12456078 ]
          Hide
          Leonardo Uribe added a comment -

          Attached patch with some changes and sent to jsr-314-open list the question under the topic:

          [jsr-314-open] Cannot register listener for PostConstructApplicationEvent and PreDestroyApplicationEvent using @ListenerFor annotation

          Show
          Leonardo Uribe added a comment - Attached patch with some changes and sent to jsr-314-open list the question under the topic: [jsr-314-open] Cannot register listener for PostConstructApplicationEvent and PreDestroyApplicationEvent using @ListenerFor annotation
          Show
          Leonardo Uribe added a comment - spec issue here: http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1020
          Leonardo Uribe made changes -
          Link This issue is duplicated by MYFACES-3559 [ MYFACES-3559 ]
          Hide
          Leonardo Uribe added a comment -

          Additional info with patch in MYFACES-3559

          Show
          Leonardo Uribe added a comment - Additional info with patch in MYFACES-3559
          Hide
          dennis hoersch added a comment -

          My use case is: I have system event listeners to

          • manipulate component tree before rendering
          • apply some default values to components
            that I want to plug in and not to overwrite some MyFaces (base) classes / components.

          By now I have list them all in the faces-config.xml but it would be nice if they are automatically detected.

          Show
          dennis hoersch added a comment - My use case is: I have system event listeners to manipulate component tree before rendering apply some default values to components that I want to plug in and not to overwrite some MyFaces (base) classes / components. By now I have list them all in the faces-config.xml but it would be nice if they are automatically detected.

            People

            • Assignee:
              Jan-Kees van Andel
              Reporter:
              Jan-Kees van Andel
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development