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.
