Wicket
  1. Wicket
  2. WICKET-291

return immutable parameter map or a copy of parameters

    Details

      Description

      WebRequestCodingStrategy#decode and PortletRequestCodingStrategy#decode try to remove values from the request's parameter map. As WicketPortletRequest returns just the immutable request parameters, this fails with an exception like this:

      java.lang.UnsupportedOperationException
      at java.util.Collections$1.remove(Collections.java:1012)
      at wicket.protocol.http.portlet.PortletRequestCodingStrategy.decode (PortletRequestCodingStrategy.java:119)
      at wicket.Request.getRequestParameters(Request.java:163)
      at wicket.RequestCycle.step(RequestCycle.java:992)
      at wicket.RequestCycle.steps(RequestCycle.java :1084)
      at wicket.RequestCycle.request(RequestCycle.java:454)
      at wicket.protocol.http.portlet.WicketPortlet.processAction(WicketPortlet.java:198)

      What we should do:
      1) make parameter map in both ServletWebRequest and WicketPortletRequest immutable
      2) remove the code that tries to remove wicket specific request parameters (why should we care about having them in there anyway)

        Issue Links

          Activity

          Hide
          Eelco Hillenius added a comment -

          Actually, there is code that depends on the parameter map being mutable:

          // Remove the 'x' parameter which contains ALL the encoded params
          this.parameterMap.remove("x");
          String decodedParamReplacement = encodedParamReplacement;
          try

          { decodedParamReplacement = URLDecoder.decode(encodedParamReplacement, Application .get().getRequestCycleSettings().getResponseRequestEncoding()); }

          catch (UnsupportedEncodingException ex)

          { log.error("error decoding url: " + encodedParamReplacement, ex); }

          in CryptedUrlWebRequestCodingStrategy

          Too late to change this now, and we don't really have to align with the servlet spec on this.

          What we should do though is ensure that all parameter accessors use that same translated map.

          Show
          Eelco Hillenius added a comment - Actually, there is code that depends on the parameter map being mutable: // Remove the 'x' parameter which contains ALL the encoded params this.parameterMap.remove("x"); String decodedParamReplacement = encodedParamReplacement; try { decodedParamReplacement = URLDecoder.decode(encodedParamReplacement, Application .get().getRequestCycleSettings().getResponseRequestEncoding()); } catch (UnsupportedEncodingException ex) { log.error("error decoding url: " + encodedParamReplacement, ex); } in CryptedUrlWebRequestCodingStrategy Too late to change this now, and we don't really have to align with the servlet spec on this. What we should do though is ensure that all parameter accessors use that same translated map.
          Hide
          Eelco Hillenius added a comment -

          Interestingly the attached patch doesn't work. So while some code gets the parameter map and makes modifications to it (like removing parameters) other pieces of code depend on the parameter map to be untouched.

          The best fix really is to forget about that difficult copy thing - it serves no purpose - and just return a copy of the map. Shallow as fine as it contains strings anyway.

          Show
          Eelco Hillenius added a comment - Interestingly the attached patch doesn't work. So while some code gets the parameter map and makes modifications to it (like removing parameters) other pieces of code depend on the parameter map to be untouched. The best fix really is to forget about that difficult copy thing - it serves no purpose - and just return a copy of the map. Shallow as fine as it contains strings anyway.
          Hide
          Eelco Hillenius added a comment -

          getParameterMap methods now return a copy of the map

          Show
          Eelco Hillenius added a comment - getParameterMap methods now return a copy of the map
          Hide
          Johan Compagner added a comment -

          so the real bug is in:

          at wicket.Request.getRequestParameters(Request.java:163)

          we should there make a clone instead of relying on that the Request does the clone...

          Show
          Johan Compagner added a comment - so the real bug is in: at wicket.Request.getRequestParameters(Request.java:163) we should there make a clone instead of relying on that the Request does the clone...

            People

            • Assignee:
              Eelco Hillenius
              Reporter:
              Eelco Hillenius
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development