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 1 commit
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
@@ -842,8 +842,12 @@ 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 This method writes out the property list (key and element pairs)
* in the natural sort order of the property keys.
* @implNote This method invokes the {@link #entrySet()} method
* and writes out the returned key and element pairs
* in the natural sort order of those keys. If subclasses override
* the {@code entrySet} method and return a different {@code Set} instance,
* then the property list is written out in the iteration order of
* that returned {@code Set}
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.

Rewording a bit:

"The keys and elements are written in the natural sort order of the keys in the Properties.entrySet unless entrySet is overridden by a subclass to return a different instance."

"different instance" is a bit hard to implement given that entrySet() returns a new synchronized set each time.
typo: missing final "."

Copy link
Member

@dfuch dfuch Sep 15, 2021

Choose a reason for hiding this comment

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

yes - maybe we could work on the wording. Perhaps:

The keys and elements are written in the natural sort order of the keys in the Properties.entrySet unless entrySet is overridden by a subclass to return a different set implementation.

*
Copy link
Contributor

@RogerRiggs RogerRiggs Sep 9, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor

@RogerRiggs RogerRiggs Sep 17, 2021

Choose a reason for hiding this comment

The 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.
The type of an instance is frequently understood to be the "implementation".
The visible type returned from Properties.entrySet() is SynchronizedSet.
Anyone can create one of those.
That statement might mislead a subclass into thinking they can not/must not return a SynchronizedSet if they want the built-in sorting.
I'm thinking that wording it in other term of the subclass might be better:
"...sort order of the keys in the entrySet() unless entrySet() is overridden by a subclass to return a different value than 'super.entrySet'(). "
The existing implementation is fine.

Copy link
Member

@dfuch dfuch Sep 17, 2021

Choose a reason for hiding this comment

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

I agree that this is a better formulation than what I suggested :-)

Copy link
Member Author

@jaikiran jaikiran Sep 18, 2021

Choose a reason for hiding this comment

The 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.
@@ -919,8 +923,15 @@ private void store0(BufferedWriter bw, String comments, boolean escUnicode)

synchronized (this) {
@SuppressWarnings("unchecked")
var entries = new ArrayList<>(((Map<String, String>) (Map) map).entrySet());
entries.sort(Map.Entry.comparingByKey());
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 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 just use the iteration order of the returned instance.
if (entries instanceof Collections.SynchronizedSet<?> ss
Copy link
Contributor

@AlanBateman AlanBateman Sep 22, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

@jaikiran jaikiran Sep 22, 2021

Choose a reason for hiding this comment

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

From the description referring to a 'different instance', I expected to see == or != in this test.
Since Properties.entrySet() always returns a new synchronized set of a new EntrySet,
specifying that the underlying map is the same instance is more difficult.
Perhaps either the SynchronizedSet or the EntrySet instance could be cached and compared.

(typo: missing space after period... ".If yes")

Copy link
Member

@dfuch dfuch Sep 15, 2021

Choose a reason for hiding this comment

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

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.

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 SynchronizedSet whose underlying collection is a private class EntrySet then we know that it's the one created by Properties.entrySet(). Like Daniel says, subclasses won't be able to create or use this internal private class and if they did override the entrySet() to change the order of the entries they would have to do it by returning us a different "type" from that method.

((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();
@@ -36,8 +36,11 @@
import java.time.format.DateTimeParseException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.TreeSet;

/*
@@ -70,48 +73,61 @@ private Object[][] createProps() {
simple.setProperty("3", "three");
simple.setProperty("0", "zero");

final Properties overrideCallsSuper = new OverridesEntrySetCallsSuper();
overrideCallsSuper.putAll(simple);

final OverridesEntrySet overridesEntrySet = new OverridesEntrySet();
overridesEntrySet.putAll(simple);

final Properties doesNotOverrideEntrySet = new DoesNotOverrideEntrySet();
doesNotOverrideEntrySet.putAll(simple);

return new Object[][]{
{simple},
{specialChars}
{simple, naturalOrder(simple)},
{specialChars, naturalOrder(specialChars)},
{overrideCallsSuper, naturalOrder(overrideCallsSuper)},
{overridesEntrySet, overridesEntrySet.expectedKeyOrder()},
{doesNotOverrideEntrySet, naturalOrder(doesNotOverrideEntrySet)}
};
}

/**
* Tests that the {@link Properties#store(Writer, String)} API writes out the properties
* in the natural order of the property keys
* in the expected order
*/
@Test(dataProvider = "propsProvider")
public void testStoreWriterKeyOrder(final Properties props) throws Exception {
public void testStoreWriterKeyOrder(final Properties props, final String[] expectedOrder) throws Exception {
// Properties.store(...) to a temp file
final Path tmpFile = Files.createTempFile("8231640", "props");
try (final Writer writer = Files.newBufferedWriter(tmpFile)) {
props.store(writer, null);
}
testStoreKeyOrder(props, tmpFile);
testStoreKeyOrder(props, tmpFile, expectedOrder);
}

/**
* Tests that the {@link Properties#store(OutputStream, String)} API writes out the properties
* in the natural order of the property keys
* in the expected order
*/
@Test(dataProvider = "propsProvider")
public void testStoreOutputStreamKeyOrder(final Properties props) throws Exception {
public void testStoreOutputStreamKeyOrder(final Properties props, final String[] expectedOrder) throws Exception {
// Properties.store(...) to a temp file
final Path tmpFile = Files.createTempFile("8231640", "props");
try (final OutputStream os = Files.newOutputStream(tmpFile)) {
props.store(os, null);
}
testStoreKeyOrder(props, tmpFile);
testStoreKeyOrder(props, tmpFile, expectedOrder);
}

/**
* {@link Properties#load(InputStream) Loads a Properties instance} from the passed
* {@code Path} and then verifies that:
* - the loaded properties instance "equals" the passed (original) "props" instance
* - the order in which the properties appear in the file represented by the path
* is the expected natural order of the property keys.
* is the same as the passed "expectedOrder"
*/
private void testStoreKeyOrder(final Properties props, final Path storedProps) throws Exception {
private void testStoreKeyOrder(final Properties props, final Path storedProps,
final String[] expectedOrder) throws Exception {
// Properties.load(...) from that stored file and verify that the loaded
// Properties has expected content
final Properties loaded = new Properties();
@@ -126,7 +142,6 @@ private void testStoreKeyOrder(final Properties props, final Path storedProps) t
try (final BufferedReader reader = Files.newBufferedReader(storedProps)) {
actualOrder = readInOrder(reader);
}
final String[] expectedOrder = expectedKeyOrder(props);
Assert.assertEquals(actualOrder.size(), expectedOrder.length,
"Unexpected number of keys read from stored properties");
if (!Arrays.equals(actualOrder.toArray(new String[0]), expectedOrder)) {
@@ -191,7 +206,7 @@ private void testDateComment(Path file) throws Exception {
}

// returns the property keys in their natural order
private static String[] expectedKeyOrder(final Properties props) {
private static String[] naturalOrder(final Properties props) {
return new TreeSet<>(props.stringPropertyNames()).toArray(new String[0]);
}

@@ -219,4 +234,42 @@ private static List<String> readInOrder(final BufferedReader reader) throws IOEx
return readKeys;
}

// Extends java.util.Properties and overrides entrySet() to return a reverse
// sorted entries set
private static class OverridesEntrySet extends Properties {
@Override
@SuppressWarnings("unchecked")
public Set<Map.Entry<Object, Object>> entrySet() {
// return a reverse sorted entries set
var entries = super.entrySet();
Comparator<Map.Entry<String, String>> comparator = Map.Entry.comparingByKey(Comparator.reverseOrder());
TreeSet<Map.Entry<String, String>> reverseSorted = new TreeSet<>(comparator);
reverseSorted.addAll((Set) entries);
return (Set) reverseSorted;
}

String[] expectedKeyOrder() {
// returns in reverse order of the property keys' natural ordering
var keys = new ArrayList<>(stringPropertyNames());
keys.sort(Comparator.reverseOrder());
return keys.toArray(new String[0]);
}
}

// Extends java.util.Properties and overrides entrySet() to just return "super.entrySet()"
private static class OverridesEntrySetCallsSuper extends Properties {
@Override
public Set<Map.Entry<Object, Object>> entrySet() {
return super.entrySet();
}
}

// Extends java.util.Properties but doesn't override entrySet() method
private static class DoesNotOverrideEntrySet extends Properties {

@Override
public String toString() {
return "DoesNotOverrideEntrySet - " + super.toString();
}
}
}