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

[Jaikiran Pai]

Hello Roger,

I've now updated the PR to implement these suggested changes. I think at 
this point all suggestions have been implemented and I don't think there 
are any open questions.

If the latest PR looks fine, I think the next step will be a CSR creation.

-Jaikiran

On 13/09/21 7:13 pm, Roger Riggs wrote:
> Hi Jaikiran,
>
> "Editing" the value of the comment property, to remove or ignore 
> blanks for other special
> characters makes the code more complex and adds a bunch of conditions. 
> Dropping
> the comment if it doesn't have the allowed format is hard to track 
> down, because there's
> no way to report it was dropped or why.
>
> I would write the value of the comment property using the 
> writeComments method
> so it is encoded the same as the other comment passed as an store 
> argument.
> That will handle all special characters and multi-line comments. It is 
> simpler to specify
> and has the same predictable output as other comments.
>
> if the property is non-empty
>
>
>
> On 9/12/21 10:29 AM, Jaikiran Pai wrote:
>> ...
>>
>>> 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.
>>
>> OK. I don't see any obvious issues with interpreting 
>> empty/whitespace-only value for the system property to translate to 
>> an empty comment line. To be clear, an empty comment line that gets 
>> written out in such cases is *always* going to be the "#" character 
>> followed by a line separator right? Irrespective of what or how many 
>> whitespace characters are present in the property value? Or do you 
>> want those characters to be carried over into that comment line that 
>> gets written out? The reason I ask this is because I think we should 
>> always write just the "#" followed by the line separator character in 
>> such cases, which effectively means we will still need the 
>> String::isBlank check which would then look something like:
>>
>> if (propVal.isBlank()) {
>>   bw.write("#");
>>   bw.newLine();
>> }
>>> Side question: do we want to do any escaping or vetting of the 
>>> property value?
>>
>> One of the reasons why we didn't want to use the date in epoch 
>> seconds or a formatted date string was to avoid the complexities that 
>> come with managing that property value. So a free-form property value 
>> seemed more appropriate and I think a free-form value still seems 
>> appropriate, but I think we should keep any vetting to minimal. More 
>> details below.
>>
>>> 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.
>>
>> The specification on the load(Reader reader) method of the 
>> java.util.Properties class has this to say about comment lines (and 
>> lines in general).
>>
>> (snipped relevant content):
>>
>>    There are two kinds of line, <i>natural lines</i> and <i>logical
>>    lines</i>.
>>    A natural line is defined as a line of characters that is terminated
>>    either
>>    by a set of line terminator characters ({@code \n} or {@code \r} or
>>    {@code \r\n}) or by the end of the stream. A natural line may be
>>    either a blank line, a comment line, or hold all or some of a
>>    key-element pair. A logical
>>    line holds all the data of a key-element pair, which may be spread
>>    out across several adjacent natural lines by escaping
>>    the line terminator sequence with a backslash character
>>    {@code \}.  Note that a comment line cannot be extended
>>    in this manner; every natural line that is a comment must have
>>    its own comment indicator, as described below.
>>
>>
>> With that knowledge about comment lines, I think what we should do is 
>> disallow system property values that contain any form of line 
>> terminator characters (\n or \r or \r\n). If the system property 
>> value is _not_ blank (as specified by String::isBlank) and contains 
>> any of these line terminator characters, we should simply ignore it 
>> and write out the current date as the date comment. That, IMO, should 
>> prevent any of these sneaky/rogue value that can end up either 
>> creating additional property key/values to be written out or causing 
>> any malformed properties file. Plus, that would help us keep the 
>> vetting to minimal without involving too much complexity.
>>
>>> 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.
>>
>> The newly updated PR has updated this @implNote to remove some of the 
>> previous text and yet retain some hints on what would be an "ideal" 
>> value for the system property value. But I think we should perhaps 
>> just get rid of this @implNote since we are now proposing to allow 
>> empty comment lines and free form text and this no longer is a "let's 
>> use this system property to allow users to specify a fixed date" 
>> enhancement.
> good
>>
>> -Jaikiran
>>
>