Code Review Request for 6578042

[Darryl Mocek]

The reason I used the Object's toString method is that there may indeed 
be an object which is not a String which is stored as a value (this was 
the reason for the bug report).  In this case, the remove method will in 
fact remove the property.  Returning null if the value is not a String 
gives the impression that there was no property with that key when the 
property may have been there and may in fact have been removed.  Using 
toString gives a best effort to notify the caller that the property was 
removed, even if the value is not a String.

This to me is a bit different from getProperty.  Returning null from 
getProperty gives the impression there is no property with the specified 
key, which may be false if the value is not a String, but you could 
argue that a non-String value is not a valid property anyway and should 
not be returned from getProperty.  However, clearProperty will actually 
remove an invalid property.

I will add to the Javadoc comment for the getProperty(String key) method 
that null may be returned if the property value is not a String.

I would prefer it if Properties didn't allow non-String keys and values 
at all since they're supposed to be Strings (by API implication) and 
that Properties not extend Hashtable or extends Hashtable<String, 
String>, but I digress.

Darryl

On Sun 06 Nov 2011 02:45:14 AM PST, Alan Bateman wrote:
>
> On 02/11/2011 19:59, David Holmes wrote:
>>
>>
>> I'm not seeing a distinction in those two statements. Both expect to
>> return the property value for a given key; both assume a valid value
>> is a String. clearProperty throws ClassCastException if the
>> assumption doesn't hold; getProperty instead returns null.
>
> The distinction is "value" vs. "string value", where the latter is
> interpreted by Darryl's patch to be the String representation. While
> that avoids the CCE, it is clearly inconsistent with
> System.getProperty that I see is also specified to return the "string
> value". So I think we have to dismiss this approach. I think this
> leaves two options:
>
> 1. Document existing behavior, meaning @throws CCE (several of
> Properties methods already have this).
>
> 2. Change the method so that it is consistent with getProperty and so
> returns null if the property value is not a String.
>
> While option 2 is clearly a change then I think it's worth exploring
> to understand the impact. Properties has always had a note in its
> javadoc to warn users that it inherits from Hashtable and it has a
> note to "strongly discourage" inserting entries that have a key or
> value that is not a String. My guess (and this is purely a guess) is
> that System.clearProperty is not used very often and so the likelihood
> of someone abusing the system properties and using clearProperty is
> probably low. I see the bug on bugs.sun.com doesn't have any votes or
> comments which suggests that this one doesn't have an angry mob either.
>
> Darryl - since you have decided to tackle this one then I would
> suggest looking into this further and coming back with a
> recommendation. I would also suggest that as part of the patch that
> the javadoc for System.getProperty and Properties.getProperty be
> clarified so that it's clear that they return null when the properties
> have been compromised.
>
> -Alan.