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

[Stuart Marks]

On Fri, 10 Sep 2021 15:51:30 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> The commit in this PR implements the proposal for enhancement that was discussed in the core-libs-dev mailing list recently[1], for https://bugs.openjdk.java.net/browse/JDK-8231640
>> 
>> At a high level - the `store()` APIs in `Properties` have been modified to now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env variable is set, then instead of writing out the current date time as a date comment, the `store()` APIs instead will use the value set for this env variable to parse it to a `Date` and write out the string form of such a date. The implementation here uses the `d MMM yyyy HH:mm:ss 'GMT'` date format and `Locale.ROOT` to format and write out such a date. This should provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, intentionally, no changes in the date format of the "current date" have been done.
>> 
>> These  modified `store()` APIs work in the presence of the `SecurityManager` too. The caller is expected to have a read permission on the `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that permission, then the implementation of these `store()` APIs will write out the "current date" and will ignore any value that has been set for the `SOURCE_DATE_EPOCH` env variable. This should allow for backward compatibility of existing applications, where, when they run under a `SecurityManager` and perhaps with an existing restrictive policy file, the presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` APIs.
>> 
>> The modified `store()` APIs will also ignore any value for `SOURCE_DATE_EPOCH` that  cannot be parsed to an `long` value. In such cases, the `store()` APIs will write out the "current date" and ignore the value set for this environment variable. No exceptions will be thrown for such invalid values. This is an additional backward compatibility precaution to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking applications.
>> 
>> An additional change in the implementation of these `store()` APIs and unrelated to the date comment, is that these APIs will now write out the property keys in a deterministic order. The keys will be written out in the natural ordering as specified by `java.lang.String#compareTo()` API.
>> 
>> The combination of the ordering of the property keys when written out and the usage of `SOURCE_DATE_EPOCH` environment value to determine the date comment should together allow for reproducibility of the output generated by these `store()` APIs.
>> 
>> New jtreg test classes have been introduced to verify these changes. The primary focus of `PropertiesStoreTest` is the ordering aspects of the property keys that are written out. On the other hand `StoreReproducibilityTest` focuses on the reproducibility of the output generated by these APIs.  The `StoreReproducibilityTest` runs these tests both in the presence and absence of `SecurityManager`. Plus, in the presence of SecurityManager, it tests both the scenarios where the caller is granted the requisite permission and in other case not granted that permission.
>> 
>> These new tests and existing tests under `test/jdk/java/util/Properties/` pass with these changes.
>> 
>> [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html
>> [2] https://reproducible-builds.org/specs/source-date-epoch/
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   allow free-form text for java.util.Properties.storeDate system property

src/java.base/share/classes/java/util/Properties.java line 832:

> 830:      * {@code #} character, the current date and time (formatted using the
> 831:      * {@code EEE MMM dd HH:mm:ss zzz yyyy} {@link DateTimeFormatter date format}),
> 832:      * and a line separator as generated by the {@code Writer}.

Since this behavior is affected by a system property, and that property name is in the standard `java.*` namespace, that should be documented here. In addition, `Writer` has no notion of a line separator; it just comes from `System.lineSeparator`. I'd suggest something like this, replacing the paragraph starting with "Next":


If the {@systemProperty java.util.Properties.storeDate} is set and is non-blank (as determined by
{@link String#isBlank String.isBlank}, a comment line is written as follows. First, a {@code #} character is written, followed by the contents of the property, followed by a line separator.

If the system property is not set or is blank, a comment line is written as follows. First, a {@code #} character is written, followed by the current date and time formatted as if by {@link DateTimeFormatter} with the format
{@code "EEE MMM dd HH:mm:ss zzz yyyy"}, followed by a line separator.


This was discussed elsewhere, but after writing that, I'm now thinking that we **should** honor the property even if it is blank. It would be useful to disable the timestamp simply by setting the property to the empty string; this will simply result in an empty comment line. This would simplify the code (and the spec) by removing conditions related to String::isBlank.

Side question: do we want to do any escaping or vetting of the property value? If for example it contains embedded newlines, this could result in a malformed property file. Or, if carefully constructed, it could contain additional property values. I think this is an unintended consequence of changing the property value from a numeric time value to a free-form string. We may want to reconsider this.

src/java.base/share/classes/java/util/Properties.java line 855:

> 853:      * the value of this system property represents a formatted
> 854:      * date time value that can be parsed back into a {@link Date} using an appropriate
> 855:      * {@link DateTimeFormatter}

With the property behavior added to normative text above, I don't think we need this note anymore. We might want to leave something here about the convention of putting a timestamp here, and an implicit(?) requirement that it be formatted properly.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5372