Wicket
  1. Wicket
  2. WICKET-647 New Wicket Portlet support
  3. WICKET-649

New Wicket Portlet support: fix appending query parameters

    Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.0-beta2, 1.3.0-beta3
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Activity

      Hide
      Ate Douma added a comment -

      In wicket-ajax.js function createUrl and AbstractAjaxBehavior.getCallbackUrl(boolean), parameters are simply appended to a wicket url using &.
      In a web application environment this is valid as the base wicket url is always (???) using a query string so the required initial ? to mark the start of the query string is already provided.

      But PortletURLs are (and must be) self contained, and are not under control of Wicket.
      And as it is unknown to the portlet how PortletURLs are constructed, one cannot assume the resulting url will already have a query string parameter defined.
      In the default Jetspeed-2 configuration, this indeed won't be the case (the Wicket url will be encoded as path parameter, not query parameter).

      I'm going to provide a simple fix:
      url.append(url.indexOf("?") > -1 ? '&' : '?').append(<parameter>)

      This fix is non-intrusive, so I think it can be applied to the wicket core trunk as well without any harm.

      But there is a more serious problem here: appending parameters to a generated PortletURL is not supported by the Portlet API!
      Jetspeed-2 does allow this optionally though, so for the time being I can get this to work as intended, but in the end and to be really "portable",
      Component.urlFor(IBehavior, RequestListenerInterface) will need to be enhanced/extended to have it embed the WebRequestCodingStrategy.IGNORE_IF_NOT_ACTIVE_PARAMETER_NAME parameter directly into the url so appending it won't be needed anymore.
      That's much more intrusive though, so I've postponed doing that and needs to be discussed further on the dev list first.

      Show
      Ate Douma added a comment - In wicket-ajax.js function createUrl and AbstractAjaxBehavior.getCallbackUrl(boolean), parameters are simply appended to a wicket url using &. In a web application environment this is valid as the base wicket url is always (???) using a query string so the required initial ? to mark the start of the query string is already provided. But PortletURLs are (and must be) self contained, and are not under control of Wicket. And as it is unknown to the portlet how PortletURLs are constructed, one cannot assume the resulting url will already have a query string parameter defined. In the default Jetspeed-2 configuration, this indeed won't be the case (the Wicket url will be encoded as path parameter, not query parameter). I'm going to provide a simple fix: url.append(url.indexOf("?") > -1 ? '&' : '?').append(<parameter>) This fix is non-intrusive, so I think it can be applied to the wicket core trunk as well without any harm. But there is a more serious problem here: appending parameters to a generated PortletURL is not supported by the Portlet API! Jetspeed-2 does allow this optionally though, so for the time being I can get this to work as intended, but in the end and to be really "portable", Component.urlFor(IBehavior, RequestListenerInterface) will need to be enhanced/extended to have it embed the WebRequestCodingStrategy.IGNORE_IF_NOT_ACTIVE_PARAMETER_NAME parameter directly into the url so appending it won't be needed anymore. That's much more intrusive though, so I've postponed doing that and needs to be discussed further on the dev list first.
      Hide
      Ate Douma added a comment -

      Initial fix committed, leaving this issue open though with regard to the "real" issue of appending parameters to a PortletURL

      Show
      Ate Douma added a comment - Initial fix committed, leaving this issue open though with regard to the "real" issue of appending parameters to a PortletURL
      Hide
      Ate Douma added a comment -

      Switching to a new 1.3.0-beta3-portlet-support branch

      Show
      Ate Douma added a comment - Switching to a new 1.3.0-beta3-portlet-support branch
      Hide
      Ate Douma added a comment -

      For Portlet API compliance, (portlet) URL modifications by adding/appending query string parameters really can't be used, even if certain portals like Jetspeed-2 do provide proprietary extension support for it.

      I've identified several area's within Wicket where this is done (and thus needs fixing):

      • AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage)
      • lots of wicket-ajax Wicket.Ajax.Request.get(path) calls which have query parameters appended to the path
      • IOnChangeListener implementations (Check, Checkbox, DropDownChoice, etc.) which when not used within a form assign a modified url to window.location.href
        (example usage of this kind is the wicket-examples DatesPage example)

      I'm going to provide (partial) fixes for the above

      Show
      Ate Douma added a comment - For Portlet API compliance, (portlet) URL modifications by adding/appending query string parameters really can't be used, even if certain portals like Jetspeed-2 do provide proprietary extension support for it. I've identified several area's within Wicket where this is done (and thus needs fixing): AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage) lots of wicket-ajax Wicket.Ajax.Request.get(path) calls which have query parameters appended to the path IOnChangeListener implementations (Check, Checkbox, DropDownChoice, etc.) which when not used within a form assign a modified url to window.location.href (example usage of this kind is the wicket-examples DatesPage example) I'm going to provide (partial) fixes for the above
      Hide
      Ate Douma added a comment -

      Fixing AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage):

      I'm going to add a new interface IActivePageBehaviorListener extends IBehaviorListener which will be used to generate the component.urlFor if parameter onlyTargetActivePage == true.
      WebRequestCodingStrategy.encode(RequestCycle requestCycle, IListenerInterfaceRequestTarget requestTarget) will check for this Listener interface (name)
      and append the <IGNORE_IF_NOT_ACTIVE_PARAMETER_NAME>=true to the url, just as AbstractAjaxBehavior originally did but now before the portlet url is generated.

      Show
      Ate Douma added a comment - Fixing AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage): I'm going to add a new interface IActivePageBehaviorListener extends IBehaviorListener which will be used to generate the component.urlFor if parameter onlyTargetActivePage == true. WebRequestCodingStrategy.encode(RequestCycle requestCycle, IListenerInterfaceRequestTarget requestTarget) will check for this Listener interface (name) and append the <IGNORE_IF_NOT_ACTIVE_PARAMETER_NAME>=true to the url, just as AbstractAjaxBehavior originally did but now before the portlet url is generated.
      Hide
      Ate Douma added a comment -

      Fixing wicket-ajax.js Wicket.Ajax.Request.get(path) calls which have query parameters appended to the path:

      In a lot of places (both in Java like in AjaxEditableLabel.onComponentTag(tag) and in javascript/ajax libraries like wicket-autocomplete.js Wicket.AutoComplete.updateChoices()),
      direct manipulation/appending of the callback url is done before it is invoked through Wicket.Ajax.Request.get().

      But, the only way to send extra parameters to Wicket from the client (javascript, Ajax) from within a portlet context is using a POST using a pre-generated portlet url (the callback url) as action url.

      To solve this most transparently (for now), I will extend Wicket.Ajax.Request.get() as follows:
      If the provided url contains a query String, I'll strip it from the url and then delegate it through Wicket.Ajax.Request.post(queryString).

      This way, no other changes are needed and it works transparently both in a servlet and a portlet context.
      This might not be the best solution and it does mean a technical change for a pure servlet environment, but the behavior and end result stays the same.

      Maybe a "cleaner" solution would be to be able to recognize/know within the client Ajax libraries if it is running in a portlet context and only then do the above.
      But that would require more changes and I don't think it is necessary nor worth the trouble (yet).
      Before merging the portlet-support into Wicket trunk, this solution needs to be reviewed and discussed if its fine as is, or possible needs further refinement.

      Show
      Ate Douma added a comment - Fixing wicket-ajax.js Wicket.Ajax.Request.get(path) calls which have query parameters appended to the path: In a lot of places (both in Java like in AjaxEditableLabel.onComponentTag(tag) and in javascript/ajax libraries like wicket-autocomplete.js Wicket.AutoComplete.updateChoices()), direct manipulation/appending of the callback url is done before it is invoked through Wicket.Ajax.Request.get(). But, the only way to send extra parameters to Wicket from the client (javascript, Ajax) from within a portlet context is using a POST using a pre-generated portlet url (the callback url) as action url. To solve this most transparently (for now), I will extend Wicket.Ajax.Request.get() as follows: If the provided url contains a query String, I'll strip it from the url and then delegate it through Wicket.Ajax.Request.post(queryString). This way, no other changes are needed and it works transparently both in a servlet and a portlet context. This might not be the best solution and it does mean a technical change for a pure servlet environment, but the behavior and end result stays the same. Maybe a "cleaner" solution would be to be able to recognize/know within the client Ajax libraries if it is running in a portlet context and only then do the above. But that would require more changes and I don't think it is necessary nor worth the trouble (yet). Before merging the portlet-support into Wicket trunk, this solution needs to be reviewed and discussed if its fine as is, or possible needs further refinement.
      Hide
      Ate Douma added a comment -

      Fixing IOnChangeListener components with wantOnSelectionChangedNotifications()==true:

      The IOnChangeListener components which have wantOnSelectionChangedNotifications() set (question: why is this method not part of the IOnChangeListener interface?),
      generate a listener callback url send back to Wicket using either:
      a) a hidden field if the component is part of a form
      b) by setting the window.location.href when clicked

      Both these methods are problematic in a portlet context wherein the (base) callback url is encoded as portlet url.

      For method a), the url stored in the hidden field is decoded by Wicket itself and bypasses the portal for decoding the embedded wicket url.
      So, for this method not the generated portlet url needs to be stored in the hidden field but the wicket url used to generate the portlet url...
      Luckily, the PortletRequestContext already saves the last encoded wicket url internally (for redirect checks mainly), so as long as no other portlet url is generated,
      this internally stored wicket url can be retrieved back to be used as (base) callback url in the hidden field.
      And, as in all current implementations (o.a.w.markup.html.form.Check, .CheckBox, .DropDownChoice, .Radio, .RadioChoice) the portlet url is generated just before in the same code block,
      this solution can savely be used.

      For method b) however, I don't have a solution yet
      As already described above for the Wicket.Ajax.Request.get() problem, the only way to solve this is using a POST (form) solution.
      But in this case it cannot done through an Ajax request but needs to be performed on the client main browser window.
      So, to properly solve this, we need to provide or dynamically create a special/extra form!
      I have only found 1 example in wicket-examples which uses method b): DatesPage.
      For now I will "solve" the DatesPage example with a very minor change by moving its LocaleDropDownChoice component inside a new "localeForm" , thereby switching it to use method a).

      A real solution for method b) still needs to be found, but how to do that needs some discussion first.

      Show
      Ate Douma added a comment - Fixing IOnChangeListener components with wantOnSelectionChangedNotifications()==true: The IOnChangeListener components which have wantOnSelectionChangedNotifications() set (question: why is this method not part of the IOnChangeListener interface?), generate a listener callback url send back to Wicket using either: a) a hidden field if the component is part of a form b) by setting the window.location.href when clicked Both these methods are problematic in a portlet context wherein the (base) callback url is encoded as portlet url. For method a), the url stored in the hidden field is decoded by Wicket itself and bypasses the portal for decoding the embedded wicket url. So, for this method not the generated portlet url needs to be stored in the hidden field but the wicket url used to generate the portlet url... Luckily, the PortletRequestContext already saves the last encoded wicket url internally (for redirect checks mainly), so as long as no other portlet url is generated, this internally stored wicket url can be retrieved back to be used as (base) callback url in the hidden field. And, as in all current implementations (o.a.w.markup.html.form.Check, .CheckBox, .DropDownChoice, .Radio, .RadioChoice) the portlet url is generated just before in the same code block, this solution can savely be used. For method b) however, I don't have a solution yet As already described above for the Wicket.Ajax.Request.get() problem, the only way to solve this is using a POST (form) solution. But in this case it cannot done through an Ajax request but needs to be performed on the client main browser window. So, to properly solve this, we need to provide or dynamically create a special/extra form! I have only found 1 example in wicket-examples which uses method b): DatesPage. For now I will "solve" the DatesPage example with a very minor change by moving its LocaleDropDownChoice component inside a new "localeForm" , thereby switching it to use method a). A real solution for method b) still needs to be found, but how to do that needs some discussion first.
      Hide
      Ate Douma added a comment -

      With the exception of the IOnChangeListener problem with wantOnSelectionChangedNotifications() method b) as described above,
      the current (1.3.0-beta3) wicket codebase can now be used for portlet development without relying on a custom, not JSR-168 compliant, portal extension for allowing appending query parameters.
      Of course, it still needs a pre-JSR-286 portals bridges ResourceURLFactory implementation, but once JSR-286 is available the portlet-support can easily and transparently be made 100% JSR-286 compliant.

      If someone does require IOnChangeListener wantOnSelectionChangedNotifications() for components outside a form (e.g. method b)), at least Jetspeed-2 (>= 2.1.2) can be configured to also support that.
      The only Jetspeed-2 configuration change needed for that is defining merge.portal.parameters.with.portlet.parameters=true in Jetspeed-2 WEB-INF/conf/jetspeed.properties (default is false).

      But I just noticed the current IOnChangeListener wantOnSelectionChangedNotifications() method b) implementations in o.a.w.markup.html.form.Check, .CheckBox, .DropDownChoice, .Radio, .RadioChoice also simply append query string parameters using an initial "&" prefix (see first comment of this issue).
      I'll correct those just as described in the first comment just for case someone needs method b) like on Jetspeed-2 with the above described extension.

      Show
      Ate Douma added a comment - With the exception of the IOnChangeListener problem with wantOnSelectionChangedNotifications() method b) as described above, the current (1.3.0-beta3) wicket codebase can now be used for portlet development without relying on a custom, not JSR-168 compliant, portal extension for allowing appending query parameters. Of course, it still needs a pre-JSR-286 portals bridges ResourceURLFactory implementation, but once JSR-286 is available the portlet-support can easily and transparently be made 100% JSR-286 compliant. If someone does require IOnChangeListener wantOnSelectionChangedNotifications() for components outside a form (e.g. method b)), at least Jetspeed-2 (>= 2.1.2) can be configured to also support that. The only Jetspeed-2 configuration change needed for that is defining merge.portal.parameters.with.portlet.parameters=true in Jetspeed-2 WEB-INF/conf/jetspeed.properties (default is false). But I just noticed the current IOnChangeListener wantOnSelectionChangedNotifications() method b) implementations in o.a.w.markup.html.form.Check, .CheckBox, .DropDownChoice, .Radio, .RadioChoice also simply append query string parameters using an initial "&" prefix (see first comment of this issue). I'll correct those just as described in the first comment just for case someone needs method b) like on Jetspeed-2 with the above described extension.
      Hide
      Ate Douma added a comment -

      Most use-cases covered now except for the caveats I described earlier.
      For the initial portlet-support I think it covers enough to be quite usable now.

      Show
      Ate Douma added a comment - Most use-cases covered now except for the caveats I described earlier. For the initial portlet-support I think it covers enough to be quite usable now.
      Hide
      Ate Douma added a comment -

      Turns out I missed out adjusting test case org.apache.wicket.ajax.markup.html.componentMap.SimpleTestPanelTest to the changed behavior of AbstractAjaxBehaviour
      where now IActivePageBehaviorListener is used for generating the url instead of IBehaviorListener when onlyTargetActivePage=true.
      This leads to a change of the url which needs to adjusted in impleTestPageExpectedResult.html and impleTestPageExpectedResult-1.html.

      Note though: I didn't notice this test case now fails as somehow (on my machine) building wicket with tests (by default) turned on didn't fail.

      Show
      Ate Douma added a comment - Turns out I missed out adjusting test case org.apache.wicket.ajax.markup.html.componentMap.SimpleTestPanelTest to the changed behavior of AbstractAjaxBehaviour where now IActivePageBehaviorListener is used for generating the url instead of IBehaviorListener when onlyTargetActivePage=true. This leads to a change of the url which needs to adjusted in impleTestPageExpectedResult.html and impleTestPageExpectedResult-1.html. Note though: I didn't notice this test case now fails as somehow (on my machine) building wicket with tests (by default) turned on didn't fail.
      Hide
      Ate Douma added a comment -

      test case fixed

      Show
      Ate Douma added a comment - test case fixed
      Hide
      Ate Douma added a comment -

      I missed encoding "ignoreIfNotActive=true" for an IActivePageBehavior.LISTENER in UrlCompressingWebCodingStrategy.
      See "Fixing AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage)" above.
      I will commit an update shortly

      Show
      Ate Douma added a comment - I missed encoding "ignoreIfNotActive=true" for an IActivePageBehavior.LISTENER in UrlCompressingWebCodingStrategy. See "Fixing AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage)" above. I will commit an update shortly
      Hide
      Ate Douma added a comment -

      Fixed AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage) for UrlCompressingWebCodingStrategy too

      Show
      Ate Douma added a comment - Fixed AbstractAjaxBehavior.getCallbackUrl(boolean onlyTargetActivePage) for UrlCompressingWebCodingStrategy too

        People

        • Assignee:
          Ate Douma
          Reporter:
          Ate Douma
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development