missing encoding in org.apache.pluto.portalImpl.core.PortalControlParameter and simplere implementation of Preference/Ctrl

[Christof Dallermassl <cd...@hyperwave.com>]

Hi!

I stumbled across two things this morning:

in org.apache.pluto.portalImpl.core.PortalControlParameter the method 
String encodeValue(String value)
encodes various characters to "0xy". But what happens, if the value itself 
holds the string "0x". Decoding that value results in a different string that 
it was before). So the value "0x" must be encoded as well (e.g. to "0xx").

example:
decodeValue(encodeValue("abc0x1")) results in "abc_" not in "abc0x1".

I suggest to add the line:
value = StringUtils.replace(value, "0x", "0xx" );
to the _beginning_ of the encode method (as the first replacement):

    private static String encodeValue(String value)
    {
        value = StringUtils.replace(value, "0x","0xx");
        value = StringUtils.replace(value, "_", "0x1" );
        value = StringUtils.replace(value, ".", "0x2" );
        value = StringUtils.replace(value, "/", "0x3" );
        value = StringUtils.replace(value, "\r", "0x4" );
        value = StringUtils.replace(value, "\n", "0x5" );
        value = StringUtils.replace(value, "<", "0x6" );
        value = StringUtils.replace(value, ">", "0x7" );
        value = StringUtils.replace(value, " ", "0x8" );
        return value;
    }


and the line
value = StringUtils.replace(value, "0xx", "0x");
to the _end_ of the decode method (as the last replacement):

    private static String decodeValue(String value)
    {
        value = StringUtils.replace(value, "0x1", "_" );
        value = StringUtils.replace(value, "0x2", "." );
        value = StringUtils.replace(value, "0x3", "/" );
        value = StringUtils.replace(value, "0x4", "\r" );
        value = StringUtils.replace(value, "0x5", "\n" );
        value = StringUtils.replace(value, "0x6", "<" );
        value = StringUtils.replace(value, "0x7", ">" );
        value = StringUtils.replace(value, "0x8", " " );
        value = StringUtils.replace(value, "0xx","0x");
        return value;
    }


so even this should work:
decode(encode("abc0xxxx1"))

P.S.: it would be _very_ helpful, if you add more documentation to the portal 
code!!! (e.g. how the PortalControlParameter stores its values into an url, 
etc.)

P.S/2: The javadoc of the PreferenceCtrl class does not say, that the param 
values in method setValue(Collection values) may be null as well. This is not 
clear and one needs to read the specification to find out (or get a 
NullPointerException during testing :-))

The PreferenceImpl class is quite complicated (using NULL_VALUE and 
NULL_ENTRY_VALUE as markers). Why not using null instead of 
NULL_ENTRY_VALUE??

And why not using null for the values member variable instead of creating a 
List holding the NULL_VALUE??

The only thing to change is then to add a boolean that indicates that the 
values were set (as values == null) does not work anymore.

Additionally, getValues() throws a NullPointerException if the values were 
never set before.

My (hopefully easier and bug-less) implementation of Preference and 
PreferenceCtrl looks like:
=================================================
package com.hyperwave.modules.portal.om.common;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;

import org.apache.pluto.om.common.Preference;
import org.apache.pluto.om.common.PreferenceCtrl;
import org.apache.pluto.util.StringUtils;

/**
 * @author cdaller
 */
public class PreferenceImpl implements Preference, PreferenceCtrl
{
  /** read only */
  protected boolean readOnly_ = false;
  /** the name of the preference */
  protected String name_;
  /** the values of the preference */
  protected Collection values_;
  /** true if values are set (even if set to null) */
  protected boolean valueSet_ = false;

  /**
   * Default Constructor
   */
  public PreferenceImpl()
  {

  }

  /**
   * Creates a new Preference using the given name and values.
   * 
   * @param name
   *          the name
   * @param values
   *          the values
   */
  public PreferenceImpl(String name, Collection values)
  {
    setName(name);
    setValues(values);
  }

  /**
   * Returns the name
   * 
   * @return the name
   * 
   * @see org.apache.pluto.om.common.Preference#getName()
   */
  public String getName()
  {
    return (name_);
  }

  /**
   * Returns the values in a readonly collection. If the value collection was 
set to <code>null</code> this method returns <code>null</code>.
   * 
   * @return the values in a readonly collection.
   * 
   * @see org.apache.pluto.om.common.Preference#getValues()
   */
  public Iterator getValues()
  {
    if (values_ == null)
      return (null);
    else
      return(Collections.unmodifiableCollection(values_).iterator());
  }

  /**
   * Returns <code>true</code> if the preference is readonly.
   * 
   * @return <code>true</code> if the preference is readonly.
   * 
   * @see org.apache.pluto.om.common.Preference#isReadOnly()
   */
  public boolean isReadOnly()
  {
    return (readOnly_);
  }

  /**
   * Return <code>true</code> if the values were set.
   * 
   * @return <code>true</code> if the values were set.
   * 
   * @see org.apache.pluto.om.common.Preference#isValueSet()
   */
  public boolean isValueSet()
  {
    return(valueSet_);
  }

  /**
   * Set the name of the preference.
   * 
   * @param name
   *          the name
   * 
   * @see org.apache.pluto.om.common.PreferenceCtrl#setName(java.lang.String)
   */
  public void setName(String name)
  {
    name_ = name;
  }

  /**
   * Set the values. All values are copied into a new collection.
   * 
   * @param values
   *          the values to set.
   * 
   * @see 
org.apache.pluto.om.common.PreferenceCtrl#setValues(java.util.Collection)
   */
  public void setValues(Collection values)
  {
    valueSet_ = true;
    if(values == null)
    {
      values_ = null;
      return;
    }
    // copy values:
    if (values_ == null)
      values_ = new ArrayList(values);
    else
    {
      values_.clear();
      values_.addAll(values);
    }
  }

  /**
   * Set the preference to read only.
   * 
   * @param readOnly
   *          if set to "true" (case insensitive), the preference will be read
   *          only, otherwise it will be writable.
   * 
   * @see 
org.apache.pluto.om.common.PreferenceCtrl#setReadOnly(java.lang.String)
   */
  public void setReadOnly(String readOnly)
  {
    readOnly_ = Boolean.valueOf(readOnly).booleanValue();
  }

  /**
   * Returns a string representation.
   * 
   * @return a string representation.
   * 
   * @see java.lang.Object#toString()
   */
  public String toString()
  {
    return toString(0);
  }

  /**
   * Returns a string representation indented with the given number of spaces.
   * Taken from pluto portal implementation.
   * 
   * @param indent
   *          the number of space to indent.
   * @return the string representation as described.
   */
  public String toString(int indent)
  {
    StringBuffer buffer = new StringBuffer(50);
    StringUtils.newLine(buffer, indent);
    buffer.append(getClass().toString());
    buffer.append(": name='");
    buffer.append(name_);
    buffer.append("', value='");

    if (values_ == null)
    {
      buffer.append("null");
    }
    else
    {
      StringUtils.newLine(buffer, indent);
      buffer.append("{");
      Iterator iterator = values_.iterator();
      if (iterator.hasNext())
      {
        StringUtils.newLine(buffer, indent);
        buffer.append((String) iterator.next());
      }
      while (iterator.hasNext())
      {
        StringUtils.indent(buffer, indent + 2);
        buffer.append((String) iterator.next());
      }
      StringUtils.newLine(buffer, indent);
      buffer.append("}");
    }

    buffer.append("', isReadOnly='");
    buffer.append(isReadOnly());
    buffer.append("'");
    return (buffer.toString());
  }

}
=============================================


regards,
Christof
-- 
---------------------------------------------------------------------------
Christof Dallermassl, Software Developer
Hyperwave Research & Development, Albrechtgasse 9, A-8010 Graz, Austria
Tel. +43 316 82 09 18 - 634, Fax +43 316 82 09 18 - 99
mailto: christof.dallermassl@hyperwave.com
http://www.hyperwave.com
---------------------------------------------------------------------------