Skip to content

Commit 415a24f

Browse files
garydgregorygarydgregory
committed
SortedProperties doesn't always process keys in sorted sequence
- Fix SortedProperties.keySet() returned an unsorted Set. - Fix SortedProperties.propertyNames() returned an unsorted Enumeration. - Fix SortedProperties.stringPropertyNames() returned an unsorted Set. - Fix SortedProperties.forEach() called its consumer out of order.
1 parent 19119be commit 415a24f

File tree

3 files changed

+306
-1
lines changed

3 files changed

+306
-1
lines changed

‎src/changes/changes.xml‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,11 @@
3535
<action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz, Joerg Budischewski" issue="COLLECTIONS-838">Calling SetUtils.union on multiple instances of SetView causes JVM to hang</action>
3636
<action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz, Joerg Budischewski" issue="COLLECTIONS-722">Improve IteratorUtils.chainedIterator() performance.</action>
3737
<action type="fix" dev="ggregory" due-to="Gary Gregory, PMD">Fix UselessOverridingMethod in org.apache.commons.collections4.properties.PropertiesFactory.</action>
38-
<action type="fix" dev="ggregory" due-to="Adam Rauch, Gary Gregory" issue="COLLECTIONS-693">OrderedProperties.stringPropertyNames() returns an unordered set.</action>
38+
<action type="fix" dev="ggregory" due-to="Adam Rauch, Gary Gregory" issue="COLLECTIONS-693">OrderedProperties.stringPropertyNames() returns an unordered Set.</action>
39+
<action type="fix" dev="ggregory" due-to="Gary Gregory" >Fix SortedProperties.keySet() returned an unsorted Set.</action>
40+
<action type="fix" dev="ggregory" due-to="Gary Gregory" >Fix SortedProperties.propertyNames() returned an unsorted Enumeration.</action>
41+
<action type="fix" dev="ggregory" due-to="Gary Gregory" >Fix SortedProperties.stringPropertyNames() returned an unsorted Set.</action>
42+
<action type="fix" dev="ggregory" due-to="Gary Gregory" >Fix SortedProperties.forEach() called its consumer out of order.</action>
3943
<!-- ADD -->
4044
<action type="add" dev="ggregory" due-to="Gary Gregory">Add generics to UnmodifiableIterator for the wrapped type.</action>
4145
<!-- UPDATE -->

‎src/main/java/org/apache/commons/collections4/properties/SortedProperties.java‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,14 @@
1919

2020
import java.util.AbstractMap;
2121
import java.util.AbstractMap.SimpleEntry;
22+
import java.util.Collections;
2223
import java.util.Enumeration;
2324
import java.util.LinkedHashSet;
2425
import java.util.Map;
2526
import java.util.Properties;
2627
import java.util.Set;
28+
import java.util.TreeSet;
29+
import java.util.function.BiConsumer;
2730
import java.util.stream.Collectors;
2831
import java.util.stream.Stream;
2932

@@ -55,12 +58,52 @@ public Set<Map.Entry<Object, Object>> entrySet() {
5558
return stream.collect(Collectors.toCollection(LinkedHashSet::new));
5659
}
5760

61+
/**
62+
* Enumerates all key/value pairs in the specified TreeSet and omits the property if the key or value is not a string.
63+
*
64+
* @param result The result set to populate.
65+
* @return The given set.
66+
*/
67+
private synchronized TreeSet<String> enumerateStringProperties(final TreeSet<String> result) {
68+
if (defaults != null) {
69+
result.addAll(defaults.stringPropertyNames());
70+
}
71+
for (final Enumeration<?> e = keys(); e.hasMoreElements();) {
72+
final Object k = e.nextElement();
73+
final Object v = get(k);
74+
if (k instanceof String && v instanceof String) {
75+
result.add((String) k);
76+
}
77+
}
78+
return result;
79+
}
80+
81+
@Override
82+
public synchronized void forEach(BiConsumer<? super Object, ? super Object> action) {
83+
keySet().stream().forEach(k -> action.accept(k, get(k)));
84+
}
85+
5886
@Override
5987
public synchronized Enumeration<Object> keys() {
6088
return new IteratorEnumeration<>(sortedKeys().collect(Collectors.toList()).iterator());
6189
}
6290

91+
@Override
92+
public Set<Object> keySet() {
93+
return new TreeSet<>(super.keySet());
94+
}
95+
96+
@Override
97+
public Enumeration<?> propertyNames() {
98+
return Collections.enumeration(keySet());
99+
}
100+
63101
private Stream<String> sortedKeys() {
64102
return keySet().stream().map(Object::toString).sorted();
65103
}
104+
105+
@Override
106+
public Set<String> stringPropertyNames() {
107+
return enumerateStringProperties(new TreeSet<>());
108+
}
66109
}

‎src/test/java/org/apache/commons/collections4/properties/SortedPropertiesTest.java‎

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,116 @@
1717
package org.apache.commons.collections4.properties;
1818

1919
import static org.junit.jupiter.api.Assertions.assertEquals;
20+
import static org.junit.jupiter.api.Assertions.assertFalse;
21+
import static org.junit.jupiter.api.Assertions.assertTrue;
2022

23+
import java.io.FileNotFoundException;
24+
import java.io.FileReader;
25+
import java.io.IOException;
26+
import java.util.Collections;
2127
import java.util.Enumeration;
2228
import java.util.Iterator;
2329
import java.util.Map;
30+
import java.util.Map.Entry;
31+
import java.util.Set;
32+
import java.util.concurrent.atomic.AtomicInteger;
2433

2534
import org.junit.jupiter.api.Test;
2635

2736
class SortedPropertiesTest {
2837

38+
private SortedProperties assertAscendingOrder(final SortedProperties sortedProperties) {
39+
final String[] keys = { "1", "10", "11", "2", "3", "4", "5", "6", "7", "8", "9" };
40+
final Enumeration<Object> enumObjects = sortedProperties.keys();
41+
for (int i = 0; i < keys.length; i++) {
42+
assertEquals("key" + keys[i], enumObjects.nextElement());
43+
}
44+
final Iterator<Object> iterSet = sortedProperties.keySet().iterator();
45+
for (int i = 0; i < keys.length; i++) {
46+
assertEquals("key" + keys[i], iterSet.next());
47+
}
48+
final Iterator<Entry<Object, Object>> iterEntrySet = sortedProperties.entrySet().iterator();
49+
for (int i = 0; i < keys.length; i++) {
50+
final Entry<Object, Object> next = iterEntrySet.next();
51+
assertEquals("key" + keys[i], next.getKey());
52+
assertEquals("value" + keys[i], next.getValue());
53+
}
54+
final Enumeration<?> propertyNames = sortedProperties.propertyNames();
55+
for (int i = 0; i < keys.length; i++) {
56+
assertEquals("key" + keys[i], propertyNames.nextElement());
57+
}
58+
final Set<String> propertyNameSet = sortedProperties.stringPropertyNames();
59+
final AtomicInteger i = new AtomicInteger(0);
60+
propertyNameSet.forEach(e -> assertEquals("key" + keys[i.getAndIncrement()], e));
61+
return sortedProperties;
62+
}
63+
64+
private SortedProperties loadOrderedKeysReverse() throws FileNotFoundException, IOException {
65+
final SortedProperties sortedProperties = new SortedProperties();
66+
try (FileReader reader = new FileReader("src/test/resources/org/apache/commons/collections4/properties/test-reverse.properties")) {
67+
sortedProperties.load(reader);
68+
}
69+
return assertAscendingOrder(sortedProperties);
70+
}
71+
72+
@Test
73+
void testCompute() {
74+
final SortedProperties sortedProperties = new SortedProperties();
75+
int first = 1;
76+
int last = 11;
77+
for (int i = first; i <= last; i++) {
78+
final AtomicInteger aInt = new AtomicInteger(i);
79+
sortedProperties.compute("key" + i, (k, v) -> "value" + aInt.get());
80+
}
81+
assertAscendingOrder(sortedProperties);
82+
sortedProperties.clear();
83+
first = 11;
84+
last = 1;
85+
for (int i = first; i >= last; i--) {
86+
final AtomicInteger aInt = new AtomicInteger(i);
87+
sortedProperties.compute("key" + i, (k, v) -> "value" + aInt.get());
88+
}
89+
assertAscendingOrder(sortedProperties);
90+
}
91+
92+
@Test
93+
void testComputeIfAbsent() {
94+
final SortedProperties sortedProperties = new SortedProperties();
95+
int first = 1;
96+
int last = 11;
97+
for (int i = first; i <= last; i++) {
98+
final AtomicInteger aInt = new AtomicInteger(i);
99+
sortedProperties.computeIfAbsent("key" + i, k -> "value" + aInt.get());
100+
}
101+
assertAscendingOrder(sortedProperties);
102+
sortedProperties.clear();
103+
first = 11;
104+
last = 1;
105+
for (int i = first; i >= last; i--) {
106+
final AtomicInteger aInt = new AtomicInteger(i);
107+
sortedProperties.computeIfAbsent("key" + i, k -> "value" + aInt.get());
108+
}
109+
assertAscendingOrder(sortedProperties);
110+
}
111+
29112
@Test
30113
void testEntrySet() {
114+
final SortedProperties sortedProperties = new SortedProperties();
115+
final char first = 'Z';
116+
final char last = 'A';
117+
for (char ch = first; ch >= last; ch--) {
118+
sortedProperties.put(String.valueOf(ch), "Value" + ch);
119+
}
120+
final Iterator<Map.Entry<Object, Object>> entries = sortedProperties.entrySet().iterator();
121+
for (char ch = first; ch <= last; ch++) {
122+
final Map.Entry<Object, Object> entry = entries.next();
123+
assertEquals(String.valueOf(ch), entry.getKey());
124+
assertEquals("Value" + ch, entry.getValue());
125+
}
126+
}
127+
128+
@Test
129+
void testEntrySet2() {
31130
final SortedProperties sortedProperties = new SortedProperties();
32131
for (char ch = 'Z'; ch >= 'A'; ch--) {
33132
sortedProperties.put(String.valueOf(ch), "Value" + ch);
@@ -40,8 +139,38 @@ void testEntrySet() {
40139
}
41140
}
42141

142+
@Test
143+
void testForEach() {
144+
final SortedProperties sortedProperties = new SortedProperties();
145+
final char first = 'Z';
146+
final char last = 'A';
147+
for (char ch = first; ch >= last; ch--) {
148+
sortedProperties.put(String.valueOf(ch), "Value" + ch);
149+
}
150+
final AtomicInteger aCh = new AtomicInteger(last);
151+
sortedProperties.forEach((k, v) -> {
152+
final char ch = (char) aCh.getAndIncrement();
153+
assertEquals(String.valueOf(ch), k);
154+
assertEquals("Value" + ch, v);
155+
});
156+
}
157+
43158
@Test
44159
void testKeys() {
160+
final SortedProperties sortedProperties = new SortedProperties();
161+
final char first = 'Z';
162+
final char last = 'A';
163+
for (char ch = first; ch >= last; ch--) {
164+
sortedProperties.put(String.valueOf(ch), "Value" + ch);
165+
}
166+
final Enumeration<Object> keys = sortedProperties.keys();
167+
for (char ch = first; ch <= last; ch++) {
168+
assertEquals(String.valueOf(ch), keys.nextElement());
169+
}
170+
}
171+
172+
@Test
173+
void testKeys2() {
45174
final SortedProperties sortedProperties = new SortedProperties();
46175
for (char ch = 'Z'; ch >= 'A'; ch--) {
47176
sortedProperties.put(String.valueOf(ch), "Value" + ch);
@@ -52,4 +181,133 @@ void testKeys() {
52181
}
53182
}
54183

184+
@Test
185+
void testLoadOrderedKeys() throws IOException {
186+
final SortedProperties sortedProperties = new SortedProperties();
187+
try (FileReader reader = new FileReader("src/test/resources/org/apache/commons/collections4/properties/test.properties")) {
188+
sortedProperties.load(reader);
189+
}
190+
assertAscendingOrder(sortedProperties);
191+
}
192+
193+
@Test
194+
void testLoadOrderedKeysReverse() throws IOException {
195+
loadOrderedKeysReverse();
196+
}
197+
198+
@Test
199+
void testMerge() {
200+
final SortedProperties sortedProperties = new SortedProperties();
201+
int first = 1;
202+
int last = 11;
203+
for (int i = first; i <= last; i++) {
204+
sortedProperties.merge("key" + i, "value" + i, (k, v) -> v);
205+
}
206+
assertAscendingOrder(sortedProperties);
207+
sortedProperties.clear();
208+
first = 11;
209+
last = 1;
210+
for (int i = first; i >= last; i--) {
211+
sortedProperties.merge("key" + i, "value" + i, (k, v) -> v);
212+
}
213+
assertAscendingOrder(sortedProperties);
214+
}
215+
216+
@Test
217+
void testPut() {
218+
final SortedProperties sortedProperties = new SortedProperties();
219+
int first = 1;
220+
int last = 11;
221+
for (int i = first; i <= last; i++) {
222+
sortedProperties.put("key" + i, "value" + i);
223+
}
224+
assertAscendingOrder(sortedProperties);
225+
sortedProperties.clear();
226+
first = 11;
227+
last = 1;
228+
for (int i = first; i >= last; i--) {
229+
sortedProperties.put("key" + i, "value" + i);
230+
}
231+
assertAscendingOrder(sortedProperties);
232+
}
233+
234+
@Test
235+
void testPutAll() {
236+
final SortedProperties sourceProperties = new SortedProperties();
237+
int first = 1;
238+
int last = 11;
239+
for (int i = first; i <= last; i++) {
240+
sourceProperties.put("key" + i, "value" + i);
241+
}
242+
final SortedProperties sortedProperties = new SortedProperties();
243+
sortedProperties.putAll(sourceProperties);
244+
assertAscendingOrder(sortedProperties);
245+
sortedProperties.clear();
246+
first = 11;
247+
last = 1;
248+
for (int i = first; i >= last; i--) {
249+
sortedProperties.put("key" + i, "value" + i);
250+
}
251+
assertAscendingOrder(sortedProperties);
252+
}
253+
254+
@Test
255+
void testPutIfAbsent() {
256+
final SortedProperties sortedProperties = new SortedProperties();
257+
int first = 1;
258+
int last = 11;
259+
for (int i = first; i <= last; i++) {
260+
sortedProperties.putIfAbsent("key" + i, "value" + i);
261+
}
262+
assertAscendingOrder(sortedProperties);
263+
sortedProperties.clear();
264+
first = 11;
265+
last = 1;
266+
for (int i = first; i >= last; i--) {
267+
sortedProperties.putIfAbsent("key" + i, "value" + i);
268+
}
269+
assertAscendingOrder(sortedProperties);
270+
}
271+
272+
@Test
273+
void testRemoveKey() throws FileNotFoundException, IOException {
274+
final SortedProperties props = loadOrderedKeysReverse();
275+
final String k = "key1";
276+
props.remove(k);
277+
assertFalse(props.contains(k));
278+
assertFalse(props.containsKey(k));
279+
assertFalse(Collections.list(props.keys()).contains(k));
280+
assertFalse(Collections.list(props.propertyNames()).contains(k));
281+
}
282+
283+
@Test
284+
void testRemoveKeyValue() throws FileNotFoundException, IOException {
285+
final SortedProperties props = loadOrderedKeysReverse();
286+
final String k = "key1";
287+
props.remove(k, "value1");
288+
assertFalse(props.contains(k));
289+
assertFalse(props.containsKey(k));
290+
assertFalse(Collections.list(props.keys()).contains(k));
291+
assertFalse(Collections.list(props.propertyNames()).contains(k));
292+
}
293+
294+
@Test
295+
void testStringPropertyName() {
296+
final SortedProperties sortedProperties = new SortedProperties();
297+
assertTrue(sortedProperties.stringPropertyNames().isEmpty());
298+
}
299+
300+
@Test
301+
void testToString() {
302+
final SortedProperties sortedProperties = new SortedProperties();
303+
final char first = 'Z';
304+
final char last = 'A';
305+
for (char ch = first; ch >= last; ch--) {
306+
sortedProperties.put(String.valueOf(ch), "Value" + ch);
307+
}
308+
assertEquals(
309+
"{A=ValueA, B=ValueB, C=ValueC, D=ValueD, E=ValueE, F=ValueF, G=ValueG, H=ValueH, I=ValueI, J=ValueJ, K=ValueK, L=ValueL, M=ValueM, N=ValueN, O=ValueO, P=ValueP, Q=ValueQ, R=ValueR, S=ValueS, T=ValueT, U=ValueU, V=ValueV, W=ValueW, X=ValueX, Y=ValueY, Z=ValueZ}",
310+
sortedProperties.toString());
311+
}
312+
55313
}

0 commit comments

Comments
 (0)