Code Review Request for 6578042
darryl.mocek at oracle.com
Fri Nov 11 11:37:14 PST 2011
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.
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.
More information about the core-libs-dev