portletspec3
  1. portletspec3
  2. PORTLETSPEC3-8

Errata: Clarify inconsistencies regarding getting and setting parameters

    Details

      Description

      Reading the spec document on parameters and comparing it with the javadoc on
      "get...Parameter..." and "set...Parameter..." calls on various Response, Request, and
      URL classes, it seems to me that there are a number of inconsistencies that should
      be addressed, for example:

      1. Is a parameter allowed to have a value of null?
      2. Under which cases (if any) should setting a parameter to a value of null remove the parameter?
      3. In which cases exactly are public parameters get and set?
      4. In which cases are public render parameters set or updated?
      5. In which cases are public render parameters removed?
      6. Should the "getParameterMap" methods always return an immutable map?
      7. probably more that I haven't noticed yet ...

      To #6 -

      The javadoc for PortletRequest.getParameterMap states:
      -------------------------
          Returns:
              an immutable Map containing parameter names as keys and parameter values as map 
              values, or an empty Map if no parameters exist. 
              The keys in the parameter map are of type String. 
              The values in the parameter map are of type String array (String[]).
      -------------------------
      
      While the javadoc for BaseURL.getParameterMap states:
      -------------------------
      Returns:
          Map containing parameter names as keys and parameter values as map 
          values, or an empty Map if no parameters exist. 
          The keys in the parameter map are of type String. 
          The values in the parameter map are of type String array (String[]).
      -------------------------
      

      Shouldn't the returned Map values be consistently immutable?

      If not, and if we want to be able to update the actual parameters by updating the
      Map returned by BaseURL.getParameterMap, it needs to be described clearly.

        Activity

        Hide
        andre.hagemeier added a comment - - edited

        Scott,
        i think whether this map is immutable or not depends on the point of view. I assume your point of view is:
        The map is not immutable since you can actually change its values, and once you set this map with setRenderParameters(), it will take effect.

        My point of view:
        The map is immutable, because i cannot change the values of the parameters of this specific map (or at least is has no effect). I need create a new map or use the same map to re-initiate the values of the backed map.

        With your example:

        params.java
        String[] values = { "Fred", "Wilma", "Pebbles" };
        actionResponse.getRenderParameterMap().put("privateRenderParameter3", values);
        // this way, you could directly change the values of the prp map. You will not receive a clone, but the real backed map, so any change to this map is directly reflected in the request
        
        String[] values = { "Fred", "Wilma", "Pebbles" };
        Map<String, String[]> parmMap = actionResponse.getRenderParameterMap();
        parmMap.put("privateRenderParameter3", values);
        actionResponse.setRenderParameters(parmMap);"
        // most likely you will receive a clone of the internal map here. otherwise you would not have to call setRenderParameters, but could just use the first approach.
        // the setRenderParameters method would then iterate over the paramMap and put its names and values to the internal map of the request
        
        //this is basically the same, as if I created a totally new map and copied all existing values. //There is just no reference to the map instance
        String[] values = { "Fred", "Wilma", "Pebbles" };
        Map<String, String[]> parmMap = new HashMap<String, String[]>();
        copy(actionResponse.getRenderParameterMap(), parmMap);
        parmMap.put("privateRenderParameter3", values);
        actionResponse.setRenderParameters(parmMap);"
        

        To me, the second approach is immutable, because changing the returned map has no effect. Only, if I use the returned map and set it using setRenderParameters, the change will take effect. But that would happen due to the fact, that all names and values are copied to the internal map of the request.

        The only thing, I want to make sure, is that existing implementations still work. Since the methods mentioned don't exist in portlet2.0, i assume, that the old ones will be deprecated, but still be available. And with that getParameterMap needs to return a mutable map, so that
        request.getParameterMap().put(x,y) will still work, even if it Eclipse will show a warning, because getParameterMap may be deprecated.

        Apart from that, as I said, I agree, that the second way would be the more accurate.

        Show
        andre.hagemeier added a comment - - edited Scott, i think whether this map is immutable or not depends on the point of view. I assume your point of view is: The map is not immutable since you can actually change its values, and once you set this map with setRenderParameters(), it will take effect. My point of view: The map is immutable, because i cannot change the values of the parameters of this specific map (or at least is has no effect). I need create a new map or use the same map to re-initiate the values of the backed map. With your example: params.java String [] values = { "Fred" , "Wilma" , "Pebbles" }; actionResponse.getRenderParameterMap().put( "privateRenderParameter3" , values); // this way, you could directly change the values of the prp map. You will not receive a clone, but the real backed map, so any change to this map is directly reflected in the request String [] values = { "Fred" , "Wilma" , "Pebbles" }; Map< String , String []> parmMap = actionResponse.getRenderParameterMap(); parmMap.put( "privateRenderParameter3" , values); actionResponse.setRenderParameters(parmMap);" // most likely you will receive a clone of the internal map here. otherwise you would not have to call setRenderParameters, but could just use the first approach. // the setRenderParameters method would then iterate over the paramMap and put its names and values to the internal map of the request // this is basically the same, as if I created a totally new map and copied all existing values. //There is just no reference to the map instance String [] values = { "Fred" , "Wilma" , "Pebbles" }; Map< String , String []> parmMap = new HashMap< String , String []>(); copy(actionResponse.getRenderParameterMap(), parmMap); parmMap.put( "privateRenderParameter3" , values); actionResponse.setRenderParameters(parmMap);" To me, the second approach is immutable, because changing the returned map has no effect. Only, if I use the returned map and set it using setRenderParameters, the change will take effect. But that would happen due to the fact, that all names and values are copied to the internal map of the request. The only thing, I want to make sure, is that existing implementations still work. Since the methods mentioned don't exist in portlet2.0, i assume, that the old ones will be deprecated, but still be available. And with that getParameterMap needs to return a mutable map, so that request.getParameterMap().put(x,y) will still work, even if it Eclipse will show a warning, because getParameterMap may be deprecated. Apart from that, as I said, I agree, that the second way would be the more accurate.
        Hide
        msnicklous added a comment -

        Andre, sorry, don't really follow you. All methods listed currently exist. In fact, the code in the examples is copy/pasted from test portlets that were run with
        varying degrees of success against several portal implementations. Nothing would be deprecated.

        I don't think we should make assumptions about the internal implementation of a portlet container. We just need to describe external behavior.

        Show
        msnicklous added a comment - Andre, sorry, don't really follow you. All methods listed currently exist. In fact, the code in the examples is copy/pasted from test portlets that were run with varying degrees of success against several portal implementations. Nothing would be deprecated. I don't think we should make assumptions about the internal implementation of a portlet container. We just need to describe external behavior.
        Hide
        andre.hagemeier added a comment -

        Sorry Scott, I guess I mixed up some things here.
        The only question to me is, what to do with the spec and the implementations. I think the implementations cannot be forced to return immutable maps.
        So two possibilities would be left:

        • allow mutable maps in the spec
        • deprecate mutable maps in 362 (but still allow them for backward compatibility) and make immutable ones the default. Therefore, a subclassing approach could be used like above stated.

        Anyway, this issue needs to be documented properly, to make clear why there is a problem at all.

        Show
        andre.hagemeier added a comment - Sorry Scott, I guess I mixed up some things here. The only question to me is, what to do with the spec and the implementations. I think the implementations cannot be forced to return immutable maps. So two possibilities would be left: allow mutable maps in the spec deprecate mutable maps in 362 (but still allow them for backward compatibility) and make immutable ones the default. Therefore, a subclassing approach could be used like above stated. Anyway, this issue needs to be documented properly, to make clear why there is a problem at all.
        Hide
        Neil Griffin added a comment -

        @Scott: I received feedback from my colleagues, and the unanimous response was to have StateAwareResponse.getRenderParameterMap() return an immutable map. This would make the API more clear that the only way to set parameters (with a Map) would be to call StateAwareResponse.setRenderParameters(java.util.Map).

        Show
        Neil Griffin added a comment - @Scott: I received feedback from my colleagues, and the unanimous response was to have StateAwareResponse.getRenderParameterMap() return an immutable map. This would make the API more clear that the only way to set parameters (with a Map) would be to call StateAwareResponse.setRenderParameters(java.util.Map) .
        Hide
        msnicklous added a comment -

        implemented updates for descriptions of BaseURL.getParameterMap() and StateAwareResponse.getRenderParameterMap().

        Show
        msnicklous added a comment - implemented updates for descriptions of BaseURL.getParameterMap() and StateAwareResponse.getRenderParameterMap().

          People

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

            Dates

            • Created:
              Updated:
              Resolved: