This is an automated email from the ASF dual-hosted git repository.
gnodet pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven.git
The following commit(s) were added to refs/heads/master by this push:
new 9d307f0a59 [MNG-8746] Preserve property insertion order in
WrapperProperties (#2404)
9d307f0a59 is described below
commit 9d307f0a5952c9255a2f7365f134d447436425a3
Author: Guillaume Nodet <gno...@gmail.com>
AuthorDate: Wed Jun 4 11:10:08 2025 +0200
[MNG-8746] Preserve property insertion order in WrapperProperties (#2404)
* Fix MNG-8746: Preserve property insertion order in WrapperProperties
This commit implements order preservation for properties in the Maven compat
layer's WrapperProperties class, ensuring that properties maintain their
insertion order when accessed through the Properties API.
Key changes:
1. Enhanced WrapperProperties template
(src/mdo/java/WrapperProperties.java):
- Added internal OrderedProperties class that maintains insertion order
- Implemented caching mechanism to preserve order across read/write
operations
- Modified all read operations to use the ordered cache
- Simplified write operations to work directly with the ordered
properties
2. OrderedProperties implementation:
- Uses ArrayList to track key insertion order
- Overrides put/setProperty/remove to maintain order
- Custom KeySet and EntrySet implementations that iterate in insertion
order
- Removed unnecessary synchronized wrappers that interfered with ordering
3. Updated model-v3.vm template:
- Ensured LinkedHashMap is used when converting properties to v4 API
- This preserves order when properties are synced between compat and v4
layers
4. Added comprehensive tests:
- PropertiesOrderTest: Tests v4 API model order preservation
- WrapperPropertiesOrderTest: Tests compat layer order preservation
- Covers various scenarios including modification and initial properties
The implementation ensures that properties accessed through
model.getProperties()
maintain their insertion order, which is important for consistent behavior
and reproducible builds.
Fixes: MNG-8746
* Update src/mdo/java/WrapperProperties.java
Co-authored-by: Pankraz76 <8830888+pankra...@users.noreply.github.com>
* Update src/mdo/java/WrapperProperties.java
Co-authored-by: Pankraz76 <8830888+pankra...@users.noreply.github.com>
---------
Co-authored-by: Pankraz76 <8830888+pankra...@users.noreply.github.com>
---
.../apache/maven/model/PropertiesOrderTest.java | 97 ++++++++
.../maven/model/WrapperPropertiesOrderTest.java | 63 +++++
src/mdo/java/WrapperProperties.java | 260 +++++++++++++++------
src/mdo/model-v3.vm | 5 +-
4 files changed, 352 insertions(+), 73 deletions(-)
diff --git
a/impl/maven-core/src/test/java/org/apache/maven/model/PropertiesOrderTest.java
b/impl/maven-core/src/test/java/org/apache/maven/model/PropertiesOrderTest.java
new file mode 100644
index 0000000000..2a0bd9e3ba
--- /dev/null
+++
b/impl/maven-core/src/test/java/org/apache/maven/model/PropertiesOrderTest.java
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.model;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * Test for MNG-8746: Properties order is preserved in model
+ */
+public class PropertiesOrderTest {
+
+ @Test
+ public void testPropertiesOrderPreservedInImmutableModel() {
+ // Create properties with specific insertion order using LinkedHashMap
+ Map<String, String> orderedMap = new LinkedHashMap<>();
+ orderedMap.put("third", "3");
+ orderedMap.put("first", "1");
+ orderedMap.put("second", "2");
+
+ // Create model and set properties
+ Model model = new Model();
+ Properties props = model.getProperties();
+
+ // Create properties and populate from map to maintain order
+ orderedMap.forEach(props::setProperty);
+
+ // Get the immutable delegate (v4 API model is already immutable)
+ org.apache.maven.api.model.Model immutable = model.getDelegate();
+
+ // Verify order is preserved
+ Map<String, String> resultProps = immutable.getProperties();
+ assertNotNull(resultProps);
+
+ // Check order by collecting keys in iteration order
+ List<String> keys = new ArrayList<>(resultProps.keySet());
+
+ // Verify the original insertion order is maintained
+ assertEquals(3, keys.size());
+ assertEquals("third", keys.get(0));
+ assertEquals("first", keys.get(1));
+ assertEquals("second", keys.get(2));
+ }
+
+ @Test
+ public void testPropertiesOrderPreservedInMutableModel() {
+ // Create ordered map to simulate properties with specific order
+ Map<String, String> orderedMap = new LinkedHashMap<>();
+ orderedMap.put("z-property", "z");
+ orderedMap.put("a-property", "a");
+ orderedMap.put("m-property", "m");
+
+ // Create and populate model
+ Model model = new Model();
+ Properties props = model.getProperties();
+
+ // Create properties and populate from map to maintain order
+ orderedMap.forEach(props::setProperty);
+
+ // Get properties back and verify order
+ Properties resultProps = model.getProperties();
+
+ // Check order by collecting keys in iteration order
+ List<String> keys = new ArrayList<>();
+ resultProps.keySet().forEach(k -> keys.add(k.toString()));
+
+ // Verify the original insertion order is maintained
+ assertEquals(3, keys.size());
+ assertEquals("z-property", keys.get(0));
+ assertEquals("a-property", keys.get(1));
+ assertEquals("m-property", keys.get(2));
+ }
+}
diff --git
a/impl/maven-core/src/test/java/org/apache/maven/model/WrapperPropertiesOrderTest.java
b/impl/maven-core/src/test/java/org/apache/maven/model/WrapperPropertiesOrderTest.java
new file mode 100644
index 0000000000..34c87bedfa
--- /dev/null
+++
b/impl/maven-core/src/test/java/org/apache/maven/model/WrapperPropertiesOrderTest.java
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.model;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Properties;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Test for order preservation in WrapperProperties used by Model
+ */
+public class WrapperPropertiesOrderTest {
+
+ @Test
+ public void testOrderPreservationAfterModification() {
+ // Create a model with properties
+ Model model = new Model();
+ Properties modelProps = model.getProperties();
+
+ // Add properties in specific order
+ modelProps.setProperty("first", "1");
+ modelProps.setProperty("second", "2");
+
+ // Modify existing property
+ modelProps.setProperty("first", "modified");
+
+ // Add new property
+ modelProps.setProperty("third", "3");
+
+ // Collect keys in iteration order
+ List<String> keys = new ArrayList<>();
+ modelProps.keySet().forEach(k -> keys.add(k.toString()));
+
+ // Verify order is preserved (first should still be first since it was
modified, not re-added)
+ assertEquals(3, keys.size());
+ assertEquals("first", keys.get(0));
+ assertEquals("second", keys.get(1));
+ assertEquals("third", keys.get(2));
+
+ // Verify value was updated
+ assertEquals("modified", modelProps.getProperty("first"));
+ }
+}
diff --git a/src/mdo/java/WrapperProperties.java
b/src/mdo/java/WrapperProperties.java
index 4a8641a8b9..8e1af9a716 100644
--- a/src/mdo/java/WrapperProperties.java
+++ b/src/mdo/java/WrapperProperties.java
@@ -27,14 +27,19 @@
import java.io.Reader;
import java.io.Writer;
import java.util.AbstractSet;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Enumeration;
import java.util.InvalidPropertiesFormatException;
import java.util.Iterator;
+import java.util.LinkedHashSet;
+import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Properties;
import java.util.Set;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.function.BiConsumer;
import java.util.function.BiFunction;
import java.util.function.Consumer;
@@ -45,30 +50,43 @@ class WrapperProperties extends Properties {
final Supplier<Map<String, String>> getter;
final Consumer<Properties> setter;
+ private final OrderedProperties orderedProps = new OrderedProperties();
+ private boolean initialized;
WrapperProperties(Supplier<Map<String, String>> getter,
Consumer<Properties> setter) {
this.getter = getter;
this.setter = setter;
}
+ private synchronized void ensureInitialized() {
+ if (!initialized) {
+ orderedProps.putAll(getter.get());
+ initialized = true;
+ }
+ }
+
@Override
public String getProperty(String key) {
- return getter.get().get(key);
+ ensureInitialized();
+ return orderedProps.getProperty(key);
}
@Override
public String getProperty(String key, String defaultValue) {
- return getter.get().getOrDefault(key, defaultValue);
+ ensureInitialized();
+ return orderedProps.getProperty(key, defaultValue);
}
@Override
public Enumeration<?> propertyNames() {
- return Collections.enumeration(getter.get().keySet());
+ ensureInitialized();
+ return orderedProps.propertyNames();
}
@Override
public Set<String> stringPropertyNames() {
- return getter.get().keySet();
+ ensureInitialized();
+ return orderedProps.stringPropertyNames();
}
@Override
@@ -83,123 +101,102 @@ public void list(PrintWriter out) {
@Override
public int size() {
- return getter.get().size();
+ ensureInitialized();
+ return orderedProps.size();
}
@Override
public boolean isEmpty() {
- return getter.get().isEmpty();
+ ensureInitialized();
+ return orderedProps.isEmpty();
}
@Override
public Enumeration<Object> keys() {
- return Collections.enumeration((Set) getter.get().keySet());
+ ensureInitialized();
+ return orderedProps.keys();
}
@Override
public Enumeration<Object> elements() {
- return Collections.enumeration((Collection) getter.get().values());
+ ensureInitialized();
+ return orderedProps.elements();
}
@Override
public boolean contains(Object value) {
- return getter.get().containsKey(value != null ? value.toString() :
null);
+ ensureInitialized();
+ return orderedProps.contains(value);
}
@Override
public boolean containsValue(Object value) {
- return getter.get().containsValue(value);
+ ensureInitialized();
+ return orderedProps.containsValue(value);
}
@Override
public boolean containsKey(Object key) {
- return getter.get().containsKey(key);
+ ensureInitialized();
+ return orderedProps.containsKey(key);
}
@Override
public Object get(Object key) {
- return getter.get().get(key);
+ ensureInitialized();
+ return orderedProps.get(key);
}
@Override
public synchronized String toString() {
- return getter.get().toString();
+ ensureInitialized();
+ return orderedProps.toString();
}
@Override
public Set<Object> keySet() {
- return (Set) getter.get().keySet();
+ ensureInitialized();
+ return orderedProps.keySet();
}
@Override
public Collection<Object> values() {
- return (Collection) getter.get().values();
+ ensureInitialized();
+ return orderedProps.values();
}
@Override
public Set<Map.Entry<Object, Object>> entrySet() {
- Set<Map.Entry<String, String>> set = getter.get().entrySet();
- return new AbstractSet<Map.Entry<Object, Object>>() {
- @Override
- public Iterator<Map.Entry<Object, Object>> iterator() {
- Iterator<Map.Entry<String, String>> it = set.iterator();
- return new Iterator<Map.Entry<Object, Object>>() {
- @Override
- public boolean hasNext() {
- return it.hasNext();
- }
-
- @Override
- public Map.Entry<Object, Object> next() {
- Map.Entry<String, String> entry = it.next();
- return new Map.Entry<Object, Object>() {
- @Override
- public Object getKey() {
- return entry.getKey();
- }
-
- @Override
- public Object getValue() {
- return entry.getValue();
- }
-
- @Override
- public Object setValue(Object value) {
- return writeOperation(p ->
p.put(entry.getKey(), value));
- }
- };
- }
- };
- }
-
- @Override
- public int size() {
- return set.size();
- }
- };
+ ensureInitialized();
+ return orderedProps.entrySet();
}
@Override
public synchronized boolean equals(Object o) {
+ ensureInitialized();
if (o instanceof WrapperProperties wrapperProperties) {
- o = wrapperProperties.getter.get();
+ wrapperProperties.ensureInitialized();
+ return orderedProps.equals(wrapperProperties.orderedProps);
}
- return getter.get().equals(o);
+ return orderedProps.equals(o);
}
@Override
public synchronized int hashCode() {
- return getter.get().hashCode();
+ ensureInitialized();
+ return orderedProps.hashCode();
}
@Override
public Object getOrDefault(Object key, Object defaultValue) {
- return getter.get().getOrDefault(key, defaultValue != null ?
defaultValue.toString() : null);
+ ensureInitialized();
+ return orderedProps.getOrDefault(key, defaultValue);
}
@Override
public synchronized void forEach(BiConsumer<? super Object, ? super
Object> action) {
- getter.get().forEach(action);
+ ensureInitialized();
+ orderedProps.forEach(action);
}
interface WriteOp<T> {
@@ -211,22 +208,16 @@ interface WriteOpVoid {
}
private <T> T writeOperation(WriteOp<T> runner) {
- Properties props = new Properties();
- props.putAll(getter.get());
- T ret = runner.perform(props);
- if (!props.equals(getter.get())) {
- setter.accept(props);
- }
+ ensureInitialized();
+ T ret = runner.perform(orderedProps);
+ setter.accept(orderedProps);
return ret;
}
private void writeOperationVoid(WriteOpVoid runner) {
- Properties props = new Properties();
- props.putAll(getter.get());
- runner.perform(props);
- if (!props.equals(getter.get())) {
- setter.accept(props);
- }
+ ensureInitialized();
+ runner.perform(orderedProps);
+ setter.accept(orderedProps);
}
@Override
@@ -377,4 +368,131 @@ private Object writeReplace() throws
java.io.ObjectStreamException {
props.putAll(getter.get());
return props;
}
+
+ private class OrderedProperties extends Properties {
+ private final List<Object> keyOrder = new CopyOnWriteArrayList<>();
+
+ @Override
+ public synchronized void putAll(Map<?, ?> t) {
+ t.forEach(this::put);
+ }
+
+ @Override
+ public Set<Object> keySet() {
+ return new KeySet();
+ }
+
+ @Override
+ public Set<Map.Entry<Object, Object>> entrySet() {
+ return new EntrySet();
+ }
+
+ @Override
+ public synchronized Object put(Object key, Object value) {
+ if (!keyOrder.contains(key)) {
+ keyOrder.add(key);
+ }
+ return super.put(key, value);
+ }
+
+ @Override
+ public synchronized Object setProperty(String key, String value) {
+ if (!keyOrder.contains(key)) {
+ keyOrder.add(key);
+ }
+ return super.setProperty(key, value);
+ }
+
+ @Override
+ public synchronized Object remove(Object key) {
+ keyOrder.remove(key);
+ return super.remove(key);
+ }
+
+ @Override
+ public synchronized void forEach(BiConsumer<? super Object, ? super
Object> action) {
+ entrySet().forEach(e -> action.accept(e.getKey(), e.getValue()));
+ }
+
+ private class EntrySet extends AbstractSet<Map.Entry<Object, Object>> {
+ @Override
+ public Iterator<Map.Entry<Object, Object>> iterator() {
+ return new Iterator<Map.Entry<Object, Object>>() {
+ Iterator<Object> keyIterator = keyOrder.iterator();
+ @Override
+ public boolean hasNext() {
+ return keyIterator.hasNext();
+ }
+
+ @Override
+ public Map.Entry<Object, Object> next() {
+ Object key = keyIterator.next();
+ return new Map.Entry<>() {
+ @Override
+ public Object getKey() {
+ return key;
+ }
+
+ @Override
+ public Object getValue() {
+ return get(key);
+ }
+
+ @Override
+ public Object setValue(Object value) {
+ return WrapperProperties.this.put(key, value);
+ }
+ };
+ }
+ };
+ }
+
+ @Override
+ public int size() {
+ return keyOrder.size();
+ }
+ }
+
+ private class KeySet extends AbstractSet<Object> {
+ public Iterator<Object> iterator() {
+ final Iterator<Object> iter = keyOrder.iterator();
+ return new Iterator<Object>() {
+ Object lastRet = null;
+ @Override
+ public boolean hasNext() {
+ return iter.hasNext();
+ }
+
+ @Override
+ public Object next() {
+ lastRet = iter.next();
+ return lastRet;
+ }
+
+ @Override
+ public void remove() {
+ WrapperProperties.super.remove(lastRet);
+ }
+ };
+ }
+
+ public int size() {
+ return keyOrder.size();
+ }
+
+ public boolean contains(Object o) {
+ return containsKey(o);
+ }
+
+ public boolean remove(Object o) {
+ boolean b = WrapperProperties.this.containsKey(o);
+ WrapperProperties.this.remove(o);
+ return b;
+ }
+
+ public void clear() {
+ WrapperProperties.this.clear();
+ }
+ }
+ }
}
diff --git a/src/mdo/model-v3.vm b/src/mdo/model-v3.vm
index b2a21f7473..a31196bf8c 100644
--- a/src/mdo/model-v3.vm
+++ b/src/mdo/model-v3.vm
@@ -50,6 +50,7 @@
#set ( $dummy = $imports.add( "java.util.AbstractList" ) )
#set ( $dummy = $imports.add( "java.util.Collections" ) )
#set ( $dummy = $imports.add( "java.util.HashMap" ) )
+ #set ( $dummy = $imports.add( "java.util.LinkedHashMap" ) )
#set ( $dummy = $imports.add( "java.util.List" ) )
#set ( $dummy = $imports.add( "java.util.Map" ) )
#set ( $dummy = $imports.add( "java.util.Set" ) )
@@ -202,8 +203,8 @@ public class ${class.name}
throw new IllegalArgumentException("Expected an Xpp3Dom object but
received a " + ${field.name}.getClass() + ": " + ${field.name});
}
#elseif( $field.type == "java.util.Properties" )
- Map<String, String> map = ${field.name}.entrySet().stream()
- .collect(Collectors.toMap(e -> e.getKey().toString(), e ->
e.getValue().toString()));
+ LinkedHashMap<String, String> map = new LinkedHashMap<>();
+ ${field.name}.forEach((key, value) -> map.put(key.toString(),
value.toString()));
if (!Objects.equals(map, getDelegate().get${cap}())) {
update(getDelegate().with${cap}(map));
}
