Wicket
  1. Wicket
  2. WICKET-3471

WicketTester checkUsability is called before the request has started

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.15
    • Fix Version/s: 1.4.18, 1.5-RC5
    • Component/s: wicket
    • Labels:
      None

      Description

      For example, when clicking a link, the checkUsability method is called with the link before RequestCycle.onBeginRequest has been called. This causes tests to fail when, for example, the link has a security check that requires the request cycle. I've created a quickstart that demonstrates the problem.

      1. WICKET-3471.patch
        4 kB
        Martijn Dashorst
      2. WICKET-3471.patch
        8 kB
        Pedro Santos
      3. quickstart.tar.bz2
        5 kB
        Emond Papegaaij

        Activity

        Emond Papegaaij created issue -
        Emond Papegaaij made changes -
        Field Original Value New Value
        Attachment quickstart.tar.bz2 [ 12471709 ]
        Hide
        Martin Grigorov added a comment -

        This usecase is a bit strange to me - a component is enabled only when there is a request.
        checkUsability() is there to prevent usage of (e.g. clicking on) disabled/invisible components. In real environment the browser cannot fire event on such component.
        I am not sure your use case can be supported in WicketTester. We either need to remove the whole check or make this method protected so you can override it locally to support your need.

        Show
        Martin Grigorov added a comment - This usecase is a bit strange to me - a component is enabled only when there is a request. checkUsability() is there to prevent usage of (e.g. clicking on) disabled/invisible components. In real environment the browser cannot fire event on such component. I am not sure your use case can be supported in WicketTester. We either need to remove the whole check or make this method protected so you can override it locally to support your need.
        Hide
        Emond Papegaaij added a comment -

        Of course, we do not use the check I used for the quickstart, it only serves to demonstrate the problem. In our application we set up a context in onBeginRequest and tear it down in onEndRequest. This context is required for some of the security checks to be performed. For example, this context contains the organisation the current user belongs to and one of the security checks verifies that the link targets a page that is part of a module that is active for that organisation. This security check currently fails, because the organisation has not yet been set on the context.

        Show
        Emond Papegaaij added a comment - Of course, we do not use the check I used for the quickstart, it only serves to demonstrate the problem. In our application we set up a context in onBeginRequest and tear it down in onEndRequest. This context is required for some of the security checks to be performed. For example, this context contains the organisation the current user belongs to and one of the security checks verifies that the link targets a page that is part of a module that is active for that organisation. This security check currently fails, because the organisation has not yet been set on the context.
        Hide
        Martin Grigorov added a comment -

        I really don't see a way to move checkUsability() inside the request cycle. There is no information about the component/behavior at that point (it gets resolved at later point when it is too late to make the check and actually is inside Wicket core core, not WicketTester).

        As I said earlier I see two solutions:
        1) make it 'protected' so you can override it to do what you need if needed
        2) remove it completely

        I prefer 1).
        checkUsability() may not be perfect and doesn't cover all cases but seems to be good enough for most of them.

        Other opinions ?

        Show
        Martin Grigorov added a comment - I really don't see a way to move checkUsability() inside the request cycle. There is no information about the component/behavior at that point (it gets resolved at later point when it is too late to make the check and actually is inside Wicket core core, not WicketTester). As I said earlier I see two solutions: 1) make it 'protected' so you can override it to do what you need if needed 2) remove it completely I prefer 1). checkUsability() may not be perfect and doesn't cover all cases but seems to be good enough for most of them. Other opinions ?
        Hide
        Martijn Dashorst added a comment -

        This fixes our issues, discovers a hidden bug in our test cases, and checks for usability (not-hidden, nor disabled) in even more places

        Show
        Martijn Dashorst added a comment - This fixes our issues, discovers a hidden bug in our test cases, and checks for usability (not-hidden, nor disabled) in even more places
        Martijn Dashorst made changes -
        Attachment WICKET-3471.patch [ 12471828 ]
        Hide
        Martijn Dashorst added a comment -

        A possible way to address this is to proxy the requestcycle and perform the check after onBeginRequest. Problem is that the component to check is not always known in the code paths...

        Looking at this it seems wickettester is a big pain in the behind: a big giant tangled mess.

        Show
        Martijn Dashorst added a comment - A possible way to address this is to proxy the requestcycle and perform the check after onBeginRequest. Problem is that the component to check is not always known in the code paths... Looking at this it seems wickettester is a big pain in the behind: a big giant tangled mess.
        Hide
        Martijn Dashorst added a comment -

        The big question in my opinion is why doesn't Wicket throw an exception from inside core when an invisible/disabled component receives an event during wickettester processing? Why is that checkUsability necessary?

        Show
        Martijn Dashorst added a comment - The big question in my opinion is why doesn't Wicket throw an exception from inside core when an invisible/disabled component receives an event during wickettester processing? Why is that checkUsability necessary?
        Hide
        Pedro Santos added a comment - - edited

        Hi Martjin, iirc it was add because the core was failing in to refuse execute behaviors in not enabled/disable behaviors. But it is fixed now and I'm 1+ to bring this check responsibility back to core request processing. Initial investigations shows that we can remove the checkUsability from clickLink and executeAjaxEvent methods. Sending the patch with tests.

        Show
        Pedro Santos added a comment - - edited Hi Martjin, iirc it was add because the core was failing in to refuse execute behaviors in not enabled/disable behaviors. But it is fixed now and I'm 1+ to bring this check responsibility back to core request processing. Initial investigations shows that we can remove the checkUsability from clickLink and executeAjaxEvent methods. Sending the patch with tests.
        Pedro Santos made changes -
        Attachment WICKET-3471.patch [ 12471835 ]
        Martijn Dashorst made changes -
        Comment [ if no objections arise, I'd like to commit this.

        One objection could be that onBeginRequest is called twice. ]
        Hide
        Martijn Dashorst added a comment -

        Just as a reminder to myself: need to revert r140505 in our svn repository when this issue is resolved.

        Show
        Martijn Dashorst added a comment - Just as a reminder to myself: need to revert r140505 in our svn repository when this issue is resolved.
        Hide
        Martin Grigorov added a comment -

        Hi,

        I have a question for the patch.
        Test 'testBehaviorOnDisabledComponentAllowed()' allows execution of Ajax behavior on disabled component. The next test 'testClickOnDisableAjaxLinkNotAllowed()' checks that clicking on disabled AjaxLink is not allowed. AjaxLink is just a Link with AjaxEventBehavior("onclick").
        It looks to me that both methods contradict to each other. What I'm missing ?

        Show
        Martin Grigorov added a comment - Hi, I have a question for the patch. Test 'testBehaviorOnDisabledComponentAllowed()' allows execution of Ajax behavior on disabled component. The next test 'testClickOnDisableAjaxLinkNotAllowed()' checks that clicking on disabled AjaxLink is not allowed. AjaxLink is just a Link with AjaxEventBehavior("onclick"). It looks to me that both methods contradict to each other. What I'm missing ?
        Hide
        Pedro Santos added a comment -

        AjaxLink has an AjaxEventBehavior that do not override Behavior#canCallListenerInterface, so invoking the behavior when its component is not enabled is not allowed, this is the default policy. TestBehaviorDisabledComponentAllowed override the canCallListenerInterface to allow the use case at 'testBehaviorOnDisabledComponentAllowed()'

        Show
        Pedro Santos added a comment - AjaxLink has an AjaxEventBehavior that do not override Behavior#canCallListenerInterface, so invoking the behavior when its component is not enabled is not allowed, this is the default policy. TestBehaviorDisabledComponentAllowed override the canCallListenerInterface to allow the use case at 'testBehaviorOnDisabledComponentAllowed()'
        Martin Grigorov committed 1128333 (1 file)
        Reviews: none

        WICKET-3471 WicketTester checkUsability is called before the request has started

        Make the method protected so the check can be "disabled" by application that need request data to check whether a component is visible/enabled.

        Martin Grigorov committed 1128334 (1 file)
        Reviews: none

        WICKET-3471 WicketTester checkUsability is called before the request has started

        Make the method protected so the check can be "disabled" by application that need request data to check whether a component is visible/enabled.

        Hide
        Martin Grigorov added a comment -

        Made the method protected so it can be suppressed when the user app really needs this.

        Show
        Martin Grigorov added a comment - Made the method protected so it can be suppressed when the user app really needs this.
        Martin Grigorov made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Martin Grigorov [ mgrigorov ]
        Fix Version/s 1.4.18 [ 12316329 ]
        Fix Version/s 1.5-RC5 [ 12316423 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Emond Papegaaij
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development