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

[Jaikiran Pai]

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?

[1] https://github.com/openjdk/jdk/pull/5372

[2] https://bugs.openjdk.java.net/browse/JDK-5032603

-Jaikiran