RFR: 8231640: (prop) Canonical property storage [v17]

[Jaikiran Pai]

On 16/09/21 4:05 am, Roger Riggs wrote:
> On Wed, 15 Sep 2021 21:46:59 GMT, Stuart Marks <smarks at openjdk.org> wrote:
>
>>> src/java.base/share/classes/java/util/Properties.java line 819:
>>>
>>>> 817:      * <p>
>>>> 818:      * If the {@systemProperty java.util.Properties.storeDate} is set and
>>>> 819:      * is non-empty (as determined by {@link String#isEmpty()  String.isEmpty}),
>>> "is set **on the command line** and non-empty"...
>>>
>>> Following from a comment on the CSR, it should be clear that the property value used can only be set on the command line.
>> This is a clever way to detect whether the `entrySet()` method has been overridden to return something other than the entry-set provided by the Properties class... but I'm wondering if it's too clever. Does anyone who overrides entrySet() care about a specific order, or do they simply sort it in order to get reproducible output? If the latter, then sorting by default hasn't really broken anything.
>>
>> Also, it was never specified that the properties are written based on what's returned by a self-call to `entrySet()`. So this was never guaranteed, though we do want to avoid gratuitous breakage.
>>
>> I would also wager that anybody who overrides entrySet() so that they can control the ordering of the entries is probably breaking the contract of Map::entrySet, which says that it's mutable (a mapping can be removed by removing its entry from the entry-set, or the underlying value can be changed by calling setValue on an entry). This is admittedly pretty obscure, but it tells me that trying to customize the output of Properties::store by overriding entrySet() is a pretty fragile hack.
>>
>> If people really need to control the order of output, they need to iterate the properties themselves instead of overriding entrySet(). I think the only difficulty in doing so is properly escaping the keys and values, as performed by saveConvert(). If this is a use case we want to support, then maybe we should expose a suitable API for escaping properties keys and values. That would be a separate enhancement, though.
>>
>> Note some history in this Stack Overflow question and answers:
>>
>> https://stackoverflow.com/questions/10275862/how-to-sort-properties-in-java
>>
>> Basically they mostly describe overriding `keys()`, but we broke that in Java 9, and now the advice is to override `entrySet()`. But the goal is to sort the properties, which we're doing anyway!
> One part of what store() does is annoying to replicate, the encoding that `saveConvert` does is non-trivial.
> Other hacks based on returning a different entrySet might be to filter the set either keep some entries or ignore them.
> Both unlikely, but hacks are frequently fragile. And we've been very cautious in this discussion to avoid
> breaking things, in part, because there is so little info about how it is used.

To summarize the options that we have discussed for this entrySet() part:

- Do nothing specific for subclasses that override entrySet() method. 
This would mean that the store() method would write out the properties 
in the natural sort order, but also has a tiny possibility that it will 
break backward compatibility if some code out there that was returning a 
differently ordered set. Given how we have tried to prevent backward 
compatibility issues in this PR plus the fact that we might have a 
possible way to prevent this, I think we can rule out this option.

- Check the Properties object instance type to see if it has been 
subclassed. If yes, then don't store the properties in the natural sort 
order. I personally think we should rule this option out because this 
option prevents this enhancement from being usable by any subclass of 
Properties even if those subclasses do nothing related to entrySet() and 
could merely have been subclassed for completely unrelated things.

- Detect that the entrySet() method is overridden by the subclass of 
Properties and might have done something with the returned 
instance/type. It's just a co-incidence that the Properties.entrySet() 
already returns a internal private class from that method. This does 
allow us to introduce a check to decide whether or not to use the new 
natural sort order for writing the properties. It relies on an internal 
knowledge plus the internal impl detail of the Property.entrySet() but, 
IMO, it might be good enough for us to use to be sure that we don't 
break backward compatibility and yet at the same time, introduce this 
natural sorted order by default for most of the subclasses out there. 
The other aspect to consider here is the future maintainability of such 
check that is being proposed here. What this check would effectively 
mean is that the implementation in Property.entrySet() and this store() 
method will have to be closely "held together" so that if we do change 
the implementation of Property.entrySet() to return something else at a 
later point, we would still have to return a type which is private to 
this Properties class (or some internal marker interface type) so that 
the store() methods can continue to employ a check like this one. IMO, 
that shouldn't be too hard to do if/when such a change happens in 
Properties.entrySet(). So I am in favour of this option to get past this 
entrySet() overridding issue.

-Jaikiran