Code Review Request for 6578042

[David Holmes]

Hi Darryl,

On 2/11/2011 3:51 AM, Darryl Mocek wrote:
> Hello. Please review this patch to fix Bug #6578042
> (System.clearProperty() throws ClassCastException if property value
> isn't a String). The issue is that through the Hashtable methods, it's
> possible to add a Property which isn't a String and set it through
> System.setProperties. clearProperty cast the returned removed object as
> a String, even if it wasn't a String. Test case provided.
>
> Webrev: http://cr.openjdk.java.net/~sherman/darryl/6578042/webrev

This fix seems inconsistent with how this non-String problem is handled 
elsewhere. Eg Properties itself does:

   public String getProperty(String key) {
         Object oval = super.get(key);
         String sval = (oval instanceof String) ? (String)oval : null;
         return ((sval == null) && (defaults != null)) ? 
defaults.getProperty(key) : sval;
     }

This will only return actual String values or else null - it won't 
convert non-String values to Strings.

Seems to me that Properties should override remove(Object key) to 
perform similar logic as getProperty(String key). That would fix the 
System.clearProperty issue.

Or, if you want to confine the change to System do:

   synchronized(props) {
     String ret = props.getProperty(key);
     props.remove(key);
     return ret;
   }

Not quite as efficient of course.

That said, I'd also support a spec change for clearProperty that states 
that if the property has been set to a non-String value then 
ClassCastException is thrown. I'm somewhat bemused that Properties 
didn't override the necessary Hashtable methods to enforce Strings-only 
in the first place.

Cheers,
David Holmes