Proposal: JDK-8231640 - (prop) Canonical property storage

[Magnus Ihse Bursie]

On 2021-09-09 10:27, Jaikiran Pai wrote:
>
>
> On 31/08/21 7:09 pm, Jaikiran Pai wrote:
>> Hello Magnus,
>>
>> On 30/08/21 5:29 pm, Magnus Ihse Bursie wrote:
>>>
>>>
>>> On 2021-08-28 17:16, Alan Bateman wrote:
>>>> On 28/08/2021 05:45, Jaikiran Pai wrote:
>>>>> I hadn't considered the system property approach to switch to old 
>>>>> behaviour in my proposals, so this is a very good input and I 
>>>>> personally think the most logical proposals so far.
>>>>
>>>> Roger may be right that few would care but it would be changing 
>>>> behavior that goes back to JDK 1.0. In your list (and thanks for 
>>>> writing down the options with pros/cons) then your 1a where 
>>>> SOURCE_DATE_EPOCH changes to write the epoch date rather than the 
>>>> current date seems to be most consistent with other tools and the 
>>>> wider eco system. It seems the least disruptive in that it doesn't 
>>>> change existing behavior and would be opt-in when reproducibility 
>>>> is required. Previous discussion was leading towards your option 2 
>>>> (or variants of) but isn't option doesn't help the cases where 
>>>> build tools use libraries that rely on the older methods.
>>>
>>> If I understood the numbering and alternatives correctly, I don't 
>>> think there is a conflict between 1a and 2, and I think both should 
>>> be implemented, for different reasons.
>>>
>>> This is my understanding of the options, with the rationale I see 
>>> for choosing them:
>>>
>>> * 1a says that we change the store() method to write the date from 
>>> SOURCE_DATE_EPOCH, if it is set -- otherwise it keeps the old 
>>> behavior. This allows users who want to use old Java code (or code 
>>> they can't modify) to produce output in a reproducible way to do so 
>>> without changing the source code of the product.
>>>
>>> * 2 says that we add a new override to store() which also takes an 
>>> Instant. This way programmers who write new code and want to make 
>>> sure it is always reproducible can send in Instant.EPOCH, and not 
>>> rely on the user to configure SOURCE_DATE_EPOCH.
>>>
>>> (In fact, when thinking of it, this seems overly complex. There is 
>>> no real need to send in a generic Instant, just an override with a 
>>> boolean skipTimestamp, or something like that. Basically, any 
>>> solution which allows programmers who write new code to get property 
>>> files without timestamps easily, is what's needed to fulfill this spot.
>>>
>> I do agree that it would be good to just get rid of the date comment 
>> and let callers control what exact comments (if any) get written out. 
>> However, I think that having both - a new method (overloaded or named 
>> differently) and at the same time changing the implementation of the 
>> existing store(...) methods to make the date comment reproducible is 
>> a bit of a overkill for a comment that doesn't even play a functional 
>> role in that API. I listed that option of a new overloaded API and 
>> would have been in favour of it, if that was the only addition/change 
>> that we did to support the date comment disabling/reproducibility.
>>
>> Having said that, I will gladly go ahead and include/work on that 
>> additional new API, if there is some general agreement on doing so.
>>
> The PR[1] related to the changes that have been agreed upon in this 
> discussion has reached a decent state, I think. There's however still 
> a valid question from Magnus. I am pasting that comment from Magnus, 
> from the PR, here:
>
>     "Am I the only one thinking there should also be a way for
>     developers to explicitly disable timestamps from the API?
>
>     I think the current iteration looks okay (but the core-libs guys
>     of course has the say in this), but I still think we need a new
>     method (or overload) to allow for a timestamp-free to be
>     generated, always, independent of environment variables."
>
> Magnus, I too think that there should be a way to completely get rid 
> of the date comment. It's my opinion that this date comment is almost 
> useless for Java programs because the Properties.load() doesn't load 
> any comments. So whatever/whoever is relying on these properties is 
> most likely going to be external (non-Java) scripts/tooling or some 
> really specific/odd Java code which reads this stored properties file, 
> per line and does something with the date comment. Highly unlikely but 
> still possible. So far, from what I have followed, no one has stated 
> that they use the date comment in some form. I even searched JBS 
> issues to see if there was any hint of this usage. On the contrary, I 
> found an (old) issue where someone asked for disabling the date 
> comment altogether and they explained a use case where they think the 
> current default of writing out that comment isn't helpful[2].
>
> At the same time, I think the concern for backward compatibility is 
> valid, plus the concern that introducing a new store API (either 
> overloaded or differently named) which allows reproducibility plus the 
> ability to disable date comment, will require existing code to migrate 
> to this new ones, which realistically takes many years.
>
> So, if callers/users still want to get rid of the date comment when 
> dealing with the Properties.store() APIs, we could ask them to come up 
> with a custom implementation which extends the java.util.Properties 
> class. That is easier said than done, because the store() APIs have 
> very involved logic when it comes to formatting and writing out the 
> key/values of that instance. That logic is private to the 
> java.util.Properties, which effectively means these custom extended 
> classes can't use it (unless they copy it over and make these 
> implementations very brittle). So extending/overriding the entirety of 
> these store() APIs is realistically ruled out.
>
> Having said all that and reviewing again the potential options that I 
> listed in my earlier mail, I think there's probably a more easier 
> middle ground here, which allows callers/tools/libraries to control 
> whether or not the date comment gets written out. So here's a proposal 
> which might help - how about, *in addition to* what we have already 
> done in this PR, we also introduce a new protected method in 
> java.util.Properties which looks like:
>
> /**
>  * Returns {@code true} if the {@link #store} methods of this properties
>  * instance must write out the date comment. Returns {@code false} 
> otherwise.
>  * @since 18
>  */
> protected boolean mustStoreDateComment() {
>   // by default we store a date comment
>   return true;
> }
>
> (ignore any javadoc issues, please :))
>
> Then the implementation of existing store() APIs in 
> java.util.Properties, can call this method to decide whether or not to 
> write out a date comment. By default, the implementation of this new 
> mustStoreDateComment() in java.util.Properties will return true, which 
> means there shouldn't be any backward compatibility issues. At the 
> same time this will allow tools/libraries/callers to extend the 
> java.util.Properties class and (only) override this method to return 
> false, if they want to disable writing out the date comment. The 
> advantage of this would be:
>
>  - Ability to fully disable the date comment and have full control on 
> what comments (if any) get written out by the existing store() APIs
>  - These custom extended classes don't have to worry or deal with the 
> formatting rules of key/values that the existing store() APIs will 
> continue to take care off.
>  - No system property or additional environment variable necessary for 
> disabling the date comment altogether
> - The JDK will still have full control on how the date comment is 
> written out and will continue to write it out in a reproducible manner 
> when SOURCE_DATE_EPOCH is set.
>  - The new "protected" API, IMO, doesn't introduce a big new API 
> surface, unlike the previously proposed overloaded or differently 
> named variants of the store() APIs.
>
> The name of this new method of course is something that can be decided 
> - I just came up with that name just to explain what I mean.
>
> Is this something that is acceptable and worth adding?

Why is this complexity needed?

Let me be more concrete: I suggest adding a new overloaded method,

publicvoidstore(Writerout, Stringcomments, boolean includeTimestamp)

(As Roger says, there's no point in duplicating the OutputStream version 
of the API as well, since it's trivial to convert an OutputStream to a 
Writer.)

If called with includeTimestamp = true, it will be identical to 
store(Writer out, String comments). But if called with false, the 
timestamp line will be omitted. SOURCE_DATE_EPOCH has no relevance in 
this case, since no timestamp is produced.

This way someone who writes a new tool (or whatever) can decide upfront 
to generate reproducible output.

The method will also be "discoverable" for developers, since an IDE will 
typically show the overloaded method as an alternative for code 
completion, which will immediately signal to programmers that there is a 
way to disable timestamps.

I can't say I understood any arguments against a solution like this. Are 
there any?

/Magnus
>
> [1] https://github.com/openjdk/jdk/pull/5372
>
> [2] https://bugs.openjdk.java.net/browse/JDK-5032603
>
> -Jaikiran
>