Re: Code Review Request for 6578042
On 15/11/2011 19:51, Darryl Mocek wrote: I've modified the fix per feedback (thanks all). System.clearProperty now attempts to get the property with the specified key. If there is such a property, and the value is a String, remove the property and return the value removed, otherwise return null (if it is null) or throw CCE (if it's not null and is not a String...do not remove the property here). Webrev can be found here: http://cr.openjdk.java.net/~sherman/darryl/6578042/webrev Thanks for changing the approach, I think this is much better. On the implementation then I assume you need to synchronize around the get+remove. The regression tests isn't in this webrev but you might want to go back over the comments on the original test. Also didn't we agree to also update Properties.getProperty to make it clear that it returns null when the value is not a String? -Alan
Re: Code Review Request for 6578042
On 16/11/2011 8:29 AM, Alan Bateman wrote: On 15/11/2011 19:51, Darryl Mocek wrote: I've modified the fix per feedback (thanks all). System.clearProperty now attempts to get the property with the specified key. If there is such a property, and the value is a String, remove the property and return the value removed, otherwise return null (if it is null) or throw CCE (if it's not null and is not a String...do not remove the property here). Webrev can be found here: http://cr.openjdk.java.net/~sherman/darryl/6578042/webrev Thanks for changing the approach, I think this is much better. On the implementation then I assume you need to synchronize around the get+remove. Yes - as it stands this is not thread-safe. Ideally I'd prefer to see the desired behaviour encapsulated in the Properties class (override Hashtable.remove) so that the synchronization is handled there too - but that may be too big an ask. Also needs a CCC for the spec change. Thanks, David The regression tests isn't in this webrev but you might want to go back over the comments on the original test. Also didn't we agree to also update Properties.getProperty to make it clear that it returns null when the value is not a String? -Alan
Re: Code Review Request for 6578042
On Mon, 2011-11-14 at 10:28 +1000, David Holmes wrote: On 12/11/2011 10:14 PM, Alan Bateman wrote: On 11/11/2011 19:37, Darryl Mocek wrote: 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. That's a fair point and probably enough to conclude that this approach should be dismissed. The toString approach is inconsistent so I think this brings us back to the original suggestion which is to specify that CCE is thrown (as it always has) but without side effects (meaning it doesn't remove the property). I think this is the best we can do with this broken API. I tend to agree. If the value is not a string then leave it alone and throw CCE. Of course, there are certain situations where the Java documentation guides you to specify property values which are not String objects. One that springs to my mind is the setting of java.naming.corba.orb to point to the ORB instance to be used by the JNDI/COS Naming Service Provider [1]. (For those using this provider, this setting tends to be important in limiting the resources it consumes). So I'm not sure it's correct to assume that it is an error for Property settings to have non-String values. Regards, Neil [1] http://download.oracle.com/javase/7/docs/technotes/guides/jndi/jndi-cos.html#PROP -- Unless stated above: IBM email: neil_richards at uk.ibm.com IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: Code Review Request for 6578042
On 14/11/2011 13:19, Neil Richards wrote: : Of course, there are certain situations where the Java documentation guides you to specify property values which are not String objects. One that springs to my mind is the setting of java.naming.corba.orb to point to the ORB instance to be used by the JNDI/COS Naming Service Provider [1]. (For those using this provider, this setting tends to be important in limiting the resources it consumes). So I'm not sure it's correct to assume that it is an error for Property settings to have non-String values. Neil - I think this is a property that is specified in the JNDI environment (so Hashtable?,? rather than Properties). The only method that is impacted by the discussion here is System.clearProperty which has always thrown CastClassException if invoked to remove a property that doesn't have a String value. I think we're concluding that the only thing we do can is fix it so that it doesn't remove the property when it fails (and clarify the javadoc accordingly). -Alan.
Re: Code Review Request for 6578042
On 12/11/2011 10:14 PM, Alan Bateman wrote: On 11/11/2011 19:37, Darryl Mocek wrote: 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. That's a fair point and probably enough to conclude that this approach should be dismissed. The toString approach is inconsistent so I think this brings us back to the original suggestion which is to specify that CCE is thrown (as it always has) but without side effects (meaning it doesn't remove the property). I think this is the best we can do with this broken API. I tend to agree. If the value is not a string then leave it alone and throw CCE. David - : 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 HashtableString, String, but I digress. Properties goes back to JDK1.0 and has a warning in its javadoc for many years to discourage inserting keys or values that are not Strings. I'm not sure whether we can do much about it now without risking compatibility issues. -Alan.
Re: Code Review Request for 6578042
On 11/11/2011 19:37, Darryl Mocek wrote: 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. That's a fair point and probably enough to conclude that this approach should be dismissed. The toString approach is inconsistent so I think this brings us back to the original suggestion which is to specify that CCE is thrown (as it always has) but without side effects (meaning it doesn't remove the property). I think this is the best we can do with this broken API. : 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 HashtableString, String, but I digress. Properties goes back to JDK1.0 and has a warning in its javadoc for many years to discourage inserting keys or values that are not Strings. I'm not sure whether we can do much about it now without risking compatibility issues. -Alan.
Re: Code Review Request for 6578042
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 HashtableString, 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.
Re: Code Review Request for 6578042
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.
Code Review Request for 6578042
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 Thanks, Darryl
