Wicket
  1. Wicket
  2. WICKET-3819

Ensure page id don't get increased inside an AJAX request cycle

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC5
    • Fix Version/s: 1.5-RC6
    • Component/s: wicket
    • Labels:
      None

      Description

      The subject started being discussed at WICKET-3751 despite not being related to. Currently the page id gets frozen if AjaxRequestTarget or RequestListenerInterface identify the page as the target of the AJAX request.
      As user can be using more than 1 page at the same time, opening popups, modal windows, frames, etc, would make sense move this spread test and frozen logic to a central place: the page itself. The benefit is to make sure that no page id will be changed inside an AJAX request compromising every already rendered URL in browser, and the removal of duplicated code in the framework to a central place.
      We can even consider to simple remove the freeze page id flag from page and have less complexity in the framework, but I don't know the impact an would be more inclined to simple remove duplicated code and centralize it in Page.

        Activity

        Hide
        Pedro Santos added a comment -

        Patch simple moving the spread id increase logic to Page.

        Show
        Pedro Santos added a comment - Patch simple moving the spread id increase logic to Page.
        Hide
        Martin Grigorov added a comment -

        I don't like that Page class uses/knows about *Web*Response.
        The patch for AjaxRequestTarger is hard to read. There are too many formatting diffs which hide the actual changes.

        Show
        Martin Grigorov added a comment - I don't like that Page class uses/knows about *Web*Response. The patch for AjaxRequestTarger is hard to read. There are too many formatting diffs which hide the actual changes.
        Hide
        Pedro Santos added a comment - - edited

        Good point, this logic is belongs to WebPage, not to Page.

        About changes in AjaxRequestTarget see https://fisheye6.atlassian.com/browse/wicket/trunk/wicket-core/src/main/java/org/apache/wicket/ajax/AjaxRequestTarget.java?r=1138191#to593
        the patch is just removing:

        • boolean frozen = page.setFreezePageId(true);
        • try
          (...)
        • finally
        • page.setFreezePageId(frozen);

        everything else is the new formating.

        Show
        Pedro Santos added a comment - - edited Good point, this logic is belongs to WebPage, not to Page. About changes in AjaxRequestTarget see https://fisheye6.atlassian.com/browse/wicket/trunk/wicket-core/src/main/java/org/apache/wicket/ajax/AjaxRequestTarget.java?r=1138191#to593 the patch is just removing: boolean frozen = page.setFreezePageId(true); try (...) finally page.setFreezePageId(frozen); everything else is the new formating.
        Hide
        Martin Grigorov added a comment -

        The change is good. We just need to move it to WebPage.

        We can try to use Checkstyle for 1.6. It will prevent such architectural mistakes (using WebRequest in Page).

        Show
        Martin Grigorov added a comment - The change is good. We just need to move it to WebPage. We can try to use Checkstyle for 1.6. It will prevent such architectural mistakes (using WebRequest in Page).
        Hide
        Pedro Santos added a comment -

        Also we can move some classes from wicket-core to wicket-web (a possible new module)

        Show
        Pedro Santos added a comment - Also we can move some classes from wicket-core to wicket-web (a possible new module)

          People

          • Assignee:
            Pedro Santos
            Reporter:
            Pedro Santos
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development