New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8231640: (prop) Canonical property storage #5372
Changes from all commits
85748cf
641864e
64e2065
1ded17f
867ec99
848ded8
5326b7c
7736a8f
c9d3cb8
a29d0f0
a9b71d2
06ff3bd
1d24a3a
c1dfb18
ff34ad5
6447f9b
9237466
315f3c8
14711a9
6f5f1be
7098a2c
79d1052
e2effb9
eb31d28
458c1fd
e350721
ed5c221
944cbf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -46,6 +46,7 @@ | ||
import java.util.function.BiFunction; | ||
import java.util.function.Function; | ||
|
||
import jdk.internal.util.StaticProperty; | ||
import sun.nio.cs.ISO_8859_1; | ||
import sun.nio.cs.UTF_8; | ||
|
||
@@ -807,17 +808,25 @@ public void save(OutputStream out, String comments) { | ||
* If the comments argument is not null, then an ASCII {@code #} | ||
* character, the comments string, and a line separator are first written | ||
* to the output stream. Thus, the {@code comments} can serve as an | ||
* identifying comment. Any one of a line feed ('\n'), a carriage | ||
* return ('\r'), or a carriage return followed immediately by a line feed | ||
* in comments is replaced by a line separator generated by the {@code Writer} | ||
* and if the next character in comments is not character {@code #} or | ||
* character {@code !} then an ASCII {@code #} is written out | ||
* after that line separator. | ||
* identifying comment. Any one of a line feed ({@code \n}), a carriage | ||
* return ({@code \r}), or a carriage return followed immediately by a line feed | ||
* ({@code \r\n}) in comments is replaced by a | ||
* {@link System#lineSeparator() line separator} and if the next | ||
* character in comments is not character {@code #} or character {@code !} then | ||
* an ASCII {@code #} is written out after that line separator. | ||
* <p> | ||
* Next, a comment line is always written, consisting of an ASCII | ||
* {@code #} character, the current date and time (as if produced | ||
* by the {@code toString} method of {@code Date} for the | ||
* current time), and a line separator as generated by the {@code Writer}. | ||
* If the {@systemProperty java.properties.date} is set on the command line | ||
* and is non-empty (as determined by {@link String#isEmpty() String.isEmpty}), | ||
* 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. Any line terminator characters | ||
* in the value of the system property are treated the same way as noted above | ||
* for the comments argument. | ||
* If the system property is not set or is empty, a comment line is written | ||
* as follows. | ||
* First, a {@code #} character is written, followed by the current date and time | ||
* formatted as if by the {@link Date#toString() Date.toString} method, | ||
* followed by a line separator. | ||
* <p> | ||
* Then every entry in this {@code Properties} table is | ||
* written out, one per line. For each entry the key string is | ||
@@ -833,6 +842,10 @@ public void save(OutputStream out, String comments) { | ||
* After the entries have been written, the output stream is flushed. | ||
* The output stream remains open after this method returns. | ||
* | ||
* @implSpec The keys and elements are written in the natural sort order | ||
* of the keys in the {@code entrySet()} unless {@code entrySet()} is | ||
* overridden by a subclass to return a different value than {@code super.entrySet()}. | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update to refer to the property and add the {@systemProperty ...} javadoc tag so the property is listed in the system property reference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "different set implementation" as part of the spec may challenge the compatibility test developers to prove or disprove that statement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this is a better formulation than what I suggested :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I've updated the PR to use Roger's suggested text and yes it's much more precise than what we had so far. |
||
* @param writer an output character stream writer. | ||
* @param comments a description of the property list. | ||
* @throws IOException if writing this property list to the specified | ||
@@ -903,12 +916,25 @@ private void store0(BufferedWriter bw, String comments, boolean escUnicode) | ||
if (comments != null) { | ||
writeComments(bw, comments); | ||
} | ||
bw.write("#" + new Date().toString()); | ||
bw.newLine(); | ||
writeDateComment(bw); | ||
|
||
synchronized (this) { | ||
for (Map.Entry<Object, Object> e : entrySet()) { | ||
String key = (String)e.getKey(); | ||
String val = (String)e.getValue(); | ||
@SuppressWarnings("unchecked") | ||
Collection<Map.Entry<String, String>> entries = (Set<Map.Entry<String, String>>) (Set) entrySet(); | ||
// entrySet() can be overridden by subclasses. Here we check to see if | ||
// the returned instance type is the one returned by the Properties.entrySet() | ||
// implementation. If yes, then we sort those entries in the natural order | ||
// of their key. Else, we consider that the subclassed implementation may | ||
// potentially have returned a differently ordered entries and so we just | ||
// use the iteration order of the returned instance. | ||
if (entries instanceof Collections.SynchronizedSet<?> ss | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind re-formatting the comment to keep the line length a bit more consistent with the rest of the code, is's just a bit annoying to have it wrapping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I've updated the PR to reduce the line length of that code comment. |
||
&& ss.c instanceof EntrySet) { | ||
entries = new ArrayList<>(entries); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the description referring to a 'different instance', I expected to see == or != in this test. (typo: missing space after period... ".If yes") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A custom subclass couldn't create an instance of EntrySet directly. Therefore if it wants to reorder the entries, it would have to return a new TreeSet or LinkedHashSet - or some other custom set implementation. If the result of calling entrySet() is an EntrySet wrapped in an UnmodifiableSet it's therefore legitimate to assume that the set hasn't been reordered, and that we can reorder it. Or am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the PR to clarify what this part of the code is doing. I took Daniel's input to reword the javadoc as well as tried to improve the code comment where this check is done. Essentially, we check for the "type" of the returned intstance and if we see that it's a |
||
((List<Map.Entry<String, String>>) entries).sort(Map.Entry.comparingByKey()); | ||
} | ||
for (Map.Entry<String, String> e : entries) { | ||
String key = e.getKey(); | ||
String val = e.getValue(); | ||
key = saveConvert(key, true, escUnicode); | ||
/* No need to escape embedded and trailing spaces for value, hence | ||
* pass false to flag. | ||
@@ -921,6 +947,19 @@ private void store0(BufferedWriter bw, String comments, boolean escUnicode) | ||
bw.flush(); | ||
} | ||
|
||
private static void writeDateComment(BufferedWriter bw) throws IOException { | ||
// value of java.properties.date system property isn't sensitive | ||
// and so doesn't need any security manager checks to make the value accessible | ||
// to the callers | ||
String sysPropVal = StaticProperty.javaPropertiesDate(); | ||
if (sysPropVal != null && !sysPropVal.isEmpty()) { | ||
writeComments(bw, sysPropVal); | ||
} else { | ||
bw.write("#" + new Date()); | ||
bw.newLine(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there is no longer a need to format an arbitrary date, I'd suggest going back to the original Date.toString() code. It removes the need to replicate the format using DateTimeBuilder and is known to be the same as before. If you keep the DateTimeFormat version, you would want "uuuu" instead of "yyyy". In java.time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. I pushed an update to the PR which switches back to using Date.toString() for the date comment. It also does a minor adjustment to the javadoc to clarify this behaviour. |
||
} | ||
|
||
/** | ||
* Loads all of the properties represented by the XML document on the | ||
* specified input stream into this properties table. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should something be done for comments ending with \ (backslash) ? It might otherwise suppress the first property assignment that follows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has been no change in how we deal with this aspect. The existing specification (stated in the
load
method) says:(emphasis on that last sentence).
I'll anyway go ahead and add new tests around this to be sure that this works as advertised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - great - thanks - verifying by a test that it also applies to the comment specified by
java.util.Properties.storeDate
would be good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. A new test
StoreReproducibilityTest#testBackSlashInStoreDateValue
has been added in the latest updated version of this PR. This test passes. Plus I checked the written out properties file, from these tests, for such values injava.util.Properties.storeDate
and the content matches what the spec says.Just for quick reference - a run of that test case with the "newline-plus-backslash...." system property value (cannot paste that exact string value from that test case because GitHub editor is messing up the special characters) generates output like below: