Skip to content
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

Closed
wants to merge 28 commits into from
Closed
Changes from 2 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
85748cf
8231640: (prop) Canonical property storage
jaikiran Aug 29, 2021
641864e
use @implNote to explain the use of the environment variable
jaikiran Sep 7, 2021
64e2065
implement review suggestions:
jaikiran Sep 8, 2021
1ded17f
adjust testcases to verify the new semantics
jaikiran Sep 8, 2021
867ec99
update javadoc @implNote to match latest changes
jaikiran Sep 8, 2021
848ded8
Implement Stuart's suggestion on date format:
jaikiran Sep 9, 2021
5326b7c
update the new tests to match the new date format expectations and al…
jaikiran Sep 9, 2021
7736a8f
Implement Stuart's suggestion to use Collections instead of Arrays fo…
jaikiran Sep 9, 2021
c9d3cb8
update javadoc to match latest changes
jaikiran Sep 9, 2021
a29d0f0
dummy commit to trigger GitHub actions job to try and reproduce an un…
jaikiran Sep 9, 2021
a9b71d2
Implement Roger's suggestions:
jaikiran Sep 10, 2021
06ff3bd
jcheck fix - trailing whitespace in test
jaikiran Sep 10, 2021
1d24a3a
allow free-form text for java.util.Properties.storeDate system property
jaikiran Sep 10, 2021
c1dfb18
Update javadoc based on Stuart's inputs
jaikiran Sep 12, 2021
ff34ad5
Implement Roger's and the agreed upon decision to allow free-form tex…
jaikiran Sep 14, 2021
6447f9b
Merge latest from master branch
jaikiran Sep 14, 2021
9237466
unused imports
jaikiran Sep 14, 2021
315f3c8
update the javadoc to clarify how line terminator characters are hand…
jaikiran Sep 14, 2021
14711a9
Add a @implNote to specify the order in which the properties are writ…
jaikiran Sep 14, 2021
6f5f1be
Introduce a test to make sure backslash character in the system prope…
jaikiran Sep 14, 2021
7098a2c
Address review suggestions:
jaikiran Sep 15, 2021
79d1052
- Clarify how overriden Properties#entrySet() method impacts the orde…
jaikiran Sep 15, 2021
e2effb9
(Only doc/comment changes):
jaikiran Sep 16, 2021
eb31d28
- Implement Mark's suggestion in CSR, to rename java.util.Properties.…
jaikiran Sep 17, 2021
458c1fd
Implement Mark's suggestion in CSR to include the java.properties.dat…
jaikiran Sep 17, 2021
e350721
Roger's suggestion to reword the implSpec text
jaikiran Sep 18, 2021
ed5c221
reduce the line length of code comment
jaikiran Sep 22, 2021
944cbf1
Revert "Implement Mark's suggestion in CSR to include the java.proper…
jaikiran Sep 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -827,11 +827,11 @@ public void save(OutputStream out, String comments) {
* after that line separator.
* <p>
* If the {@systemProperty java.util.Properties.storeDate} is set and
* is non-blank (as determined by {@link String#isBlank String.isBlank}),
* is non-empty (as determined by {@link String#isEmpty() String.isEmpty}),
Copy link
Contributor

@RogerRiggs RogerRiggs Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"is set on the command line and non-empty"...

Following from a comment on the CSR, it should be clear that the property value used can only be set on the command line.

Copy link
Member

@stuart-marks stuart-marks Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clever way to detect whether the entrySet() method has been overridden to return something other than the entry-set provided by the Properties class... but I'm wondering if it's too clever. Does anyone who overrides entrySet() care about a specific order, or do they simply sort it in order to get reproducible output? If the latter, then sorting by default hasn't really broken anything.

Also, it was never specified that the properties are written based on what's returned by a self-call to entrySet(). So this was never guaranteed, though we do want to avoid gratuitous breakage.

I would also wager that anybody who overrides entrySet() so that they can control the ordering of the entries is probably breaking the contract of Map::entrySet, which says that it's mutable (a mapping can be removed by removing its entry from the entry-set, or the underlying value can be changed by calling setValue on an entry). This is admittedly pretty obscure, but it tells me that trying to customize the output of Properties::store by overriding entrySet() is a pretty fragile hack.

If people really need to control the order of output, they need to iterate the properties themselves instead of overriding entrySet(). I think the only difficulty in doing so is properly escaping the keys and values, as performed by saveConvert(). If this is a use case we want to support, then maybe we should expose a suitable API for escaping properties keys and values. That would be a separate enhancement, though.

Note some history in this Stack Overflow question and answers:

https://stackoverflow.com/questions/10275862/how-to-sort-properties-in-java

Basically they mostly describe overriding keys(), but we broke that in Java 9, and now the advice is to override entrySet(). But the goal is to sort the properties, which we're doing anyway!

Copy link
Contributor

@RogerRiggs RogerRiggs Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One part of what store() does is annoying to replicate, the encoding that saveConvert does is non-trivial.
Other hacks based on returning a different entrySet might be to filter the set either keep some entries or ignore them.
Both unlikely, but hacks are frequently fragile. And we've been very cautious in this discussion to avoid
breaking things, in part, because there is so little info about how it is used.

Copy link
Member Author

@jaikiran jaikiran Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"is set on the command line and non-empty"...

Following from a comment on the CSR, it should be clear that the property value used can only be set on the command line.

Done. The PR has been updated accordingly.

* 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.
Copy link
Member

@magicus magicus Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should refer to how the comment is written out above, since you use the same method. The spec changes as written above does not specify how newlines in the property would be handled (which is possible to get, I believe, even if it means an intricate command line escape dance)

Copy link
Member Author

@jaikiran jaikiran Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Magnus,

Maybe you should refer to how the comment is written out above, since you use the same method.

I've updated the javadoc to capture this part of the detail.

The spec changes as written above does not specify how newlines in the property would be handled (which is possible to get, I believe, even if it means an intricate command line escape dance)

The new tests that were added in this PR has one specific test to verify how line terminators are dealt with, if the system property value has them https://github.com/openjdk/jdk/pull/5372/files#diff-220f7bcc6d1a7ec33764f81eb95ccb0f69444e1eb923b015025e2140c3ffe145R280

* If the system property is not set or is blank, a comment line is written
* 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 {@link DateTimeFormatter} with the format
@@ -851,12 +851,6 @@ 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.
*
* @implNote Although this method doesn't mandate it, conventionally,
* the value of the {@systemProperty java.util.Properties.storeDate}
* system property, if set, represents a formatted date time value that can be
* parsed back into a {@link Date} using an appropriate
* {@link DateTimeFormatter}
*
* @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
@@ -953,11 +947,12 @@ private static void writeDateComment(BufferedWriter bw) throws IOException {
// and so doesn't need any security manager checks to make the value accessible
// to the callers
String storeDate = StaticProperty.javaUtilPropertiesStoreDate();
String dateComment = (storeDate != null && !storeDate.isBlank())
? "#" + storeDate
: "#" + DateTimeFormatter.ofPattern(dateFormatPattern).format(ZonedDateTime.now());
bw.write(dateComment);
bw.newLine();
if (storeDate != null && !storeDate.isEmpty()) {
writeComments(bw, storeDate);
} else {
bw.write("#" + DateTimeFormatter.ofPattern(dateFormatPattern).format(ZonedDateTime.now()));
bw.newLine();
}
Copy link
Contributor

@RogerRiggs RogerRiggs Sep 14, 2021

Choose a reason for hiding this comment

The 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.
DateTimeFormatterBuilder uses slightly different pattern letter conventions as compared to SimpleDateFormat.

Copy link
Member Author

@jaikiran jaikiran Sep 15, 2021

Choose a reason for hiding this comment

The 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.

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.

}

/**
@@ -68,6 +68,10 @@ public static void main(final String[] args) throws Exception {
testNonDateStoreDateValue();
// blank value for java.util.Properties.storeDate system property
testBlankStoreDateValue();
// empty value for java.util.Properties.storeDate system property
testEmptyStoreDateValue();
// value for java.util.Properties.storeDate system property contains line terminator characters
testMultiLineStoreDateValue();
}

/**
@@ -191,11 +195,11 @@ private static void testWithSecMgrNoSpecificPermission() throws Exception {

/**
* Launches a Java program which is responsible for using Properties.store() to write out the
* properties to a file. The launched Java program is passed a blank value
* properties to a file. The launched Java program is passed a {@link String#isBlank() blank} value
* for the {@code java.util.Properties.storeDate} system property.
* It is expected and verified in this test that such a value for the system property
* will cause the date comment to use the current date time. The launched program is expected to complete
* without any errors.
* will cause a comment line to be written out with only whitespaces.
* The launched program is expected to complete without any errors.
*/
private static void testBlankStoreDateValue() throws Exception {
for (int i = 0; i < 2; i++) {
@@ -205,6 +209,37 @@ private static void testBlankStoreDateValue() throws Exception {
StoreTest.class.getName(),
tmpFile.toString(),
i % 2 == 0 ? "--use-outputstream" : "--use-writer");
executeJavaProcess(processBuilder);
if (!StoreTest.propsToStore.equals(loadProperties(tmpFile))) {
throw new RuntimeException("Unexpected properties stored in " + tmpFile);
}
String blankCommentLine = findNthComment(tmpFile, 2);
if (blankCommentLine == null) {
throw new RuntimeException("Comment line representing the value of "
+ SYS_PROP_JAVA_UTIL_PROPERTIES_STOREDATE + " system property is missing in file " + tmpFile);
}
if (!blankCommentLine.isBlank()) {
throw new RuntimeException("Expected comment line to be blank but was " + blankCommentLine);
}
}
}

/**
* Launches a Java program which is responsible for using Properties.store() to write out the
* properties to a file. The launched Java program is passed a {@link String#isEmpty() empty} value
* for the {@code java.util.Properties.storeDate} system property.
* It is expected and verified in this test that such a value for the system property
* will cause the current date and time to be written out as a comment.
* The launched program is expected to complete without any errors.
*/
private static void testEmptyStoreDateValue() throws Exception {
for (int i = 0; i < 2; i++) {
final Path tmpFile = Files.createTempFile("8231640", ".props");
final ProcessBuilder processBuilder = ProcessTools.createJavaProcessBuilder(
"-D" + SYS_PROP_JAVA_UTIL_PROPERTIES_STOREDATE + "=" + "",
StoreTest.class.getName(),
tmpFile.toString(),
i % 2 == 0 ? "--use-outputstream" : "--use-writer");
Date launchedAt = new Date();
// wait for a second before launching so that we can then expect
// the date written out by the store() APIs to be "after" this launch date
@@ -242,6 +277,44 @@ private static void testNonDateStoreDateValue() throws Exception {
}
}

/**
* Launches a Java program which is responsible for using Properties.store() to write out the
* properties to a file. The launched Java program is passed the {@code java.util.Properties.storeDate}
* system property with a value that has line terminator characters.
* It is expected and verified in this test that such a value for the system property
* will cause the comment written out to be multiple separate comments. The launched program is expected
* to complete without any errors.
*/
private static void testMultiLineStoreDateValue() throws Exception {
final String[] storeDates = {"hello-world\nc=d", "hello-world\rc=d", "hello-world\r\nc=d"};
for (final String storeDate : storeDates) {
for (int i = 0; i < 2; i++) {
final Path tmpFile = Files.createTempFile("8231640", ".props");
final ProcessBuilder processBuilder = ProcessTools.createJavaProcessBuilder(
"-D" + SYS_PROP_JAVA_UTIL_PROPERTIES_STOREDATE + "=" + storeDate,
StoreTest.class.getName(),
tmpFile.toString(),
i % 2 == 0 ? "--use-outputstream" : "--use-writer");
executeJavaProcess(processBuilder);
if (!StoreTest.propsToStore.equals(loadProperties(tmpFile))) {
throw new RuntimeException("Unexpected properties stored in " + tmpFile);
}
// verify this results in 2 separate comment lines in the stored file
String commentLine1 = findNthComment(tmpFile, 2);
String commentLine2 = findNthComment(tmpFile, 3);
if (commentLine1 == null || commentLine2 == null) {
throw new RuntimeException("Did not find the expected multi-line comments in " + tmpFile);
}
if (!commentLine1.equals("hello-world")) {
throw new RuntimeException("Unexpected comment line " + commentLine1 + " in " + tmpFile);
}
if (!commentLine2.equals("c=d")) {
throw new RuntimeException("Unexpected comment line " + commentLine2 + " in " + tmpFile);
}
}
}
}

// launches the java process and waits for it to exit. throws an exception if exit value is non-zero
private static void executeJavaProcess(ProcessBuilder pb) throws Exception {
final OutputAnalyzer outputAnalyzer = ProcessTools.executeProcess(pb);
@@ -276,8 +349,9 @@ private static void assertExpectedStoreDate(final Path destFile,
}

/**
* Verifies that the date comment in the {@code destFile} can be parsed and the time
* represented by it is {@link Date#after(Date) after} the passed {@code date}
* Verifies that the date comment in the {@code destFile} can be parsed using the
* "EEE MMM dd HH:mm:ss zzz yyyy" format and the time represented by it is {@link Date#after(Date) after}
* the passed {@code date}
*/
private static void assertCurrentDate(final Path destFile, final Date date) throws Exception {
final String dateComment = findNthComment(destFile, 2);