sashapolo commented on a change in pull request #208:
URL: https://github.com/apache/ignite-3/pull/208#discussion_r668704855
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java
##########
@@ -266,59 +275,54 @@ public void fillFromPrefixMapSuccessfullyWithRemove() {
/** */
@Test
- public void nodeToFlatMap() {
- var parentNode = newParentInstance();
+ public void flattenedUpdatesMap() {
+ var superRoot = new SuperRoot(key -> null,
Map.of(ParentConfiguration.KEY, newParentInstance()));
assertEquals(
- emptyMap(),
- ConfigurationUtil.nodeToFlatMap(null, new SuperRoot(key -> null,
Map.of(
- ParentConfiguration.KEY,
- parentNode
- )))
+ Map.of(),
+ flattenedMap(superRoot, parent -> {})
);
- // No defaults in this test so everything must be initialized
explicitly.
- parentNode.changeElements(elements ->
- elements.create("name", element ->
- element.changeChild(child ->
- child.changeStr("foo")
+ assertEquals(
+ Map.of(
+ "root.elements.name.child.str", "foo",
+ "root.elements.name.<idx>", 0
+ ),
+ flattenedMap(superRoot, parent -> parent
+ .changeElements(elements -> elements
+ .create("name", element -> element
+ .changeChild(child -> child.changeStr("foo"))
+ )
)
)
);
assertEquals(
- singletonMap("root.elements.name.child.str", "foo"),
- ConfigurationUtil.nodeToFlatMap(null, new SuperRoot(key -> null,
Map.of(
- ParentConfiguration.KEY,
- parentNode
- )))
+ Map.of(),
+ flattenedMap(superRoot, parent -> parent
+ .changeElements(elements1 -> elements1.delete("void"))
+ )
);
assertEquals(
- emptyMap(),
- ConfigurationUtil.nodeToFlatMap(new SuperRoot(key -> null, Map.of(
- ParentConfiguration.KEY,
- parentNode
- )), new SuperRoot(key -> null, singletonMap(
- ParentConfiguration.KEY,
- (InnerNode)newParentInstance().changeElements(elements ->
- elements.delete("void")
- )
- )))
+ new HashMap<>() {{
Review comment:
I personally don't like double braces initialization, can we use
matchers instead? For example:
```
assertThat(
flattenedMap(superRoot, parent -> parent.changeElements(elements ->
elements.delete("name"))),
allOf(
hasEntry("root.elements.name.child.str", null),
hasEntry("root.elements.name.<idx>", null)
)
);
```
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/TestConfigurationChanger.java
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.ignite.internal.configuration;
+
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
+import org.apache.ignite.internal.configuration.tree.InnerNode;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+
+public class TestConfigurationChanger extends ConfigurationChanger {
Review comment:
missing javadoc
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
Review comment:
```suggestion
* @throws IllegalArgumentException If an element with the given name
already exists.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
Review comment:
```suggestion
* @param valConsumer Closure to modify the value associated with the
key. Closure parameter must not be leaked outside the scope of the closure.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IndexOutOfBoundsException If index cannot be used for new
element insertion.
+ * @throws IllegalArgumentException If element with given name already
exists.
+ */
+ NamedListChange<Change> create(int index, String key, Consumer<Change>
valConsumer);
+
+ /**
+ * Create new value in named list configuration.
+ *
+ * @param base Name of th preceeding element.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists
+ * or if {@code base} element doesn't exist.
+ */
+ NamedListChange<Change> createAfter(String base, String key,
Consumer<Change> valConsumer);
Review comment:
Looks like `base` is not a very good name, how about `precedingKey`?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -252,14 +255,24 @@ private InnerConfigurationSource(Map<String, ?> map) {
/** {@inheritDoc} */
@Override public void descend(ConstructableTreeNode node) {
+ boolean nodeIsNamedList = node instanceof NamedListNode;
+
+ List<String> orderedKeys = nodeIsNamedList
+ ? new ArrayList<>(((NamedListView<?>)node).namedListKeys())
+ : null;
+
for (Map.Entry<String, ?> entry : map.entrySet()) {
String key = entry.getKey();
Object val = entry.getValue();
assert val == null || val instanceof Map || val instanceof
Serializable;
+ // Ordering indexes must be skipped here bacause they make
no sense in this context.
Review comment:
```suggestion
// Ordering of indexes must be skipped here because they
make no sense in this context.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
Review comment:
If we are mimicking the `Map` interface, then we should return a value
here.
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
Review comment:
```suggestion
* Inserts a value into the map under the specified key. If the key was
not present in the map, it will be ordered last.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param idx Ordering index for the key. Treated as {@code 0} if
negative. Treated as last index if out of bounds.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putByIndex(int idx, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ if (idx >= orderedKeys.size())
+ orderedKeys.add(key);
+ else
+ orderedKeys.add(idx, key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param base Previous key for the new key. Last key will be used if this
one is missing from the map.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putAfter(String base, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ int idx = orderedKeys.indexOf(base);
+
+ putByIndex(idx < 0 ? orderedKeys.size() : idx + 1, key, value);
+ }
+
+ /**
+ * Put value associated with key {@code oldKey} updaer a new key {@code
newKey}. Do nothing if {@code oldKey}
+ * was missing from the map.
+ *
+ * @param oldKey Old key.
+ * @param newKey New key.
+ *
+ * @throws IllegalArgumentException If both {@code oldKey} and {@code
newKey} already exist in the map.
+ */
+ public void rename(String oldKey, String newKey) {
+ if (!map.containsKey(oldKey))
+ return;
+
+ if (map.containsKey(newKey))
+ throw new IllegalArgumentException();
+
+ int idx = orderedKeys.indexOf(oldKey);
+
+ orderedKeys.set(idx, newKey);
+
+ V value = map.remove(oldKey);
+
+ map.put(newKey, value);
+ }
+
+ /**
+ * @return List of keys.
+ */
+ public List<String> keys() {
+ return new ArrayList<>(orderedKeys);
Review comment:
why do you need a copy?
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/tree/NamedListOrderTest.java
##########
@@ -0,0 +1,176 @@
+/*
+ * 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.ignite.internal.configuration.tree;
+
+import java.util.Map;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.Value;
+import org.apache.ignite.internal.configuration.ConfigurationChanger;
+import org.apache.ignite.internal.configuration.TestConfigurationChanger;
+import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
+import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class NamedListOrderTest {
+ /** */
+ @ConfigurationRoot(rootName = "a")
+ public static class AConfigurationSchema {
+ /** */
+ @NamedConfigValue
+ public BConfigurationSchema b;
+ }
+
+ /** */
+ @Config
+ public static class BConfigurationSchema {
+ /** */
+ @Value(hasDefault = true)
+ public String c = "foo";
+
+ @NamedConfigValue
+ public BConfigurationSchema b;
+ }
+
+ private static ConfigurationAsmGenerator cgen;
+
+ private TestConfigurationStorage storage;
+
+ private ConfigurationChanger changer;
+
+ @BeforeAll
+ public static void beforeAll() {
+ cgen = new ConfigurationAsmGenerator();
+ }
+
+ @AfterAll
+ public static void afterAll() {
+ cgen = null;
+ }
+
+ @BeforeEach
+ public void before() {
+ storage = new TestConfigurationStorage();
+
+ changer = new TestConfigurationChanger(cgen);
+ changer.addRootKey(AConfiguration.KEY);
+ changer.register(storage);
+ }
+
+ @AfterEach
+ public void after() {
+ changer.stop();
+ }
+
+ @Test
+ public void test() throws Exception {
Review comment:
please add some comments throughout this method about what are you
testing in particular
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/storage/TestConfigurationStorage.java
##########
@@ -31,7 +30,7 @@
*/
public class TestConfigurationStorage implements ConfigurationStorage {
/** Map to store values. */
- private Map<String, Serializable> map = new ConcurrentHashMap<>();
+ private Map<String, Serializable> map = new HashMap<>();
Review comment:
not related to this PR, but looks like `registerConfigurationListener`
is not thread-safe
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java
##########
@@ -266,59 +275,54 @@ public void fillFromPrefixMapSuccessfullyWithRemove() {
/** */
@Test
- public void nodeToFlatMap() {
- var parentNode = newParentInstance();
+ public void flattenedUpdatesMap() {
+ var superRoot = new SuperRoot(key -> null,
Map.of(ParentConfiguration.KEY, newParentInstance()));
assertEquals(
- emptyMap(),
- ConfigurationUtil.nodeToFlatMap(null, new SuperRoot(key -> null,
Map.of(
- ParentConfiguration.KEY,
- parentNode
- )))
+ Map.of(),
+ flattenedMap(superRoot, parent -> {})
);
- // No defaults in this test so everything must be initialized
explicitly.
- parentNode.changeElements(elements ->
- elements.create("name", element ->
- element.changeChild(child ->
- child.changeStr("foo")
+ assertEquals(
+ Map.of(
+ "root.elements.name.child.str", "foo",
+ "root.elements.name.<idx>", 0
+ ),
+ flattenedMap(superRoot, parent -> parent
+ .changeElements(elements -> elements
+ .create("name", element -> element
+ .changeChild(child -> child.changeStr("foo"))
+ )
)
)
);
assertEquals(
- singletonMap("root.elements.name.child.str", "foo"),
- ConfigurationUtil.nodeToFlatMap(null, new SuperRoot(key -> null,
Map.of(
- ParentConfiguration.KEY,
- parentNode
- )))
+ Map.of(),
+ flattenedMap(superRoot, parent -> parent
+ .changeElements(elements1 -> elements1.delete("void"))
+ )
);
assertEquals(
- emptyMap(),
- ConfigurationUtil.nodeToFlatMap(new SuperRoot(key -> null, Map.of(
- ParentConfiguration.KEY,
- parentNode
- )), new SuperRoot(key -> null, singletonMap(
- ParentConfiguration.KEY,
- (InnerNode)newParentInstance().changeElements(elements ->
- elements.delete("void")
- )
- )))
+ new HashMap<>() {{
+ put("root.elements.name.child.str", null);
+ put("root.elements.name.<idx>", null);
+ }},
+ flattenedMap(superRoot, parent -> parent
+ .changeElements(elements -> elements.delete("name"))
+ )
);
+ }
- assertEquals(
- singletonMap("root.elements.name.child.str", null),
- ConfigurationUtil.nodeToFlatMap(new SuperRoot(key -> null, Map.of(
- ParentConfiguration.KEY,
- parentNode
- )), new SuperRoot(key -> null, singletonMap(
- ParentConfiguration.KEY,
- (InnerNode)newParentInstance().changeElements(elements ->
- elements.delete("name")
- )
- )))
- );
+ @NotNull private Map<String, Serializable> flattenedMap(SuperRoot
superRoot, Consumer<ParentChange> patch) {
Review comment:
missing javadoc
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
Review comment:
```suggestion
* @throws NullPointerException If one of the parameters is null.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
Review comment:
```suggestion
* Creates a new value in the named list configuration.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
Review comment:
```suggestion
* Creates a new value in the named list configuration.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
Review comment:
```suggestion
* @param index Index of the inserted element.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
Review comment:
```suggestion
* @throws NullPointerException If one of the parameters is null.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IndexOutOfBoundsException If index cannot be used for new
element insertion.
+ * @throws IllegalArgumentException If element with given name already
exists.
+ */
+ NamedListChange<Change> create(int index, String key, Consumer<Change>
valConsumer);
+
+ /**
+ * Create new value in named list configuration.
+ *
+ * @param base Name of th preceeding element.
+ * @param key Key for the value to be created.
Review comment:
please update the parameters according to the previous comments
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -18,22 +18,23 @@
package org.apache.ignite.internal.configuration.tree;
import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.List;
import java.util.Objects;
-import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Supplier;
import org.apache.ignite.configuration.NamedListChange;
-import org.apache.ignite.configuration.NamedListView;
+import org.apache.ignite.internal.configuration.util.OrderedMap;
/** */
-public final class NamedListNode<N extends InnerNode> implements
NamedListView<N>, NamedListChange<N>, TraversableTreeNode,
ConstructableTreeNode {
+public final class NamedListNode<N extends InnerNode> implements
NamedListChange<N>, TraversableTreeNode, ConstructableTreeNode {
+ /** Name of synthetic configuration property that describes order of
elements in named list. */
Review comment:
```suggestion
/** Name of a synthetic configuration property that describes the order
of elements in a named list. */
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IndexOutOfBoundsException If index cannot be used for new
element insertion.
+ * @throws IllegalArgumentException If element with given name already
exists.
+ */
+ NamedListChange<Change> create(int index, String key, Consumer<Change>
valConsumer);
+
+ /**
+ * Create new value in named list configuration.
+ *
+ * @param base Name of th preceeding element.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists
+ * or if {@code base} element doesn't exist.
+ */
+ NamedListChange<Change> createAfter(String base, String key,
Consumer<Change> valConsumer);
+
+ /**
+ * Update the value in named list configuration. Create if it doesn't
exist yet.
Review comment:
```suggestion
* Updates a value in the named list configuration. If the value cannot
be found, creates a new one instead.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IndexOutOfBoundsException If index cannot be used for new
element insertion.
+ * @throws IllegalArgumentException If element with given name already
exists.
+ */
+ NamedListChange<Change> create(int index, String key, Consumer<Change>
valConsumer);
+
+ /**
+ * Create new value in named list configuration.
+ *
+ * @param base Name of th preceeding element.
Review comment:
```suggestion
* @param base Name of the preceding element.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IndexOutOfBoundsException If index cannot be used for new
element insertion.
+ * @throws IllegalArgumentException If element with given name already
exists.
Review comment:
```suggestion
* @throws IllegalArgumentException If an element with the given name
already exists.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IndexOutOfBoundsException If index cannot be used for new
element insertion.
Review comment:
what do you mean here?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -18,22 +18,23 @@
package org.apache.ignite.internal.configuration.tree;
import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.List;
import java.util.Objects;
-import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Supplier;
import org.apache.ignite.configuration.NamedListChange;
-import org.apache.ignite.configuration.NamedListView;
+import org.apache.ignite.internal.configuration.util.OrderedMap;
/** */
-public final class NamedListNode<N extends InnerNode> implements
NamedListView<N>, NamedListChange<N>, TraversableTreeNode,
ConstructableTreeNode {
+public final class NamedListNode<N extends InnerNode> implements
NamedListChange<N>, TraversableTreeNode, ConstructableTreeNode {
+ /** Name of synthetic configuration property that describes order of
elements in named list. */
+ public static final String ORDER_IDX = "<idx>";
+
/** */
public final Supplier<N> valSupplier;
Review comment:
can be `private`
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -110,18 +172,8 @@ public void forceDelete(String key) {
map.remove(key);
}
- /** {@inheritDoc} */
- @Override public NamedListChange<N> create(String key, Consumer<N>
valConsumer) {
- Objects.requireNonNull(valConsumer, "valConsumer");
-
- N val = map.get(key);
-
- if (val == null)
- map.put(key, val = valSupplier.get());
-
- valConsumer.accept(val);
-
- return this;
+ public void reorderKeys(List<String> orderedKeys) {
Review comment:
missing javadoc
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -20,37 +20,71 @@
import java.util.function.Consumer;
/** */
-public interface NamedListChange<Change> {
+public interface NamedListChange<Change> extends NamedListView<Change> {
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
*
* @param key Key for the value to be created.
* @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
* passed to the closure, must not be reused anywhere else.
* @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If element with given name already
exists.
*/
- //TODO Replace with "createOrUpdate"
NamedListChange<Change> create(String key, Consumer<Change> valConsumer);
/**
- * Update the value in named list configuration.
+ * Create new value in named list configuration.
+ *
+ * @param index Elements insertion index.
+ * @param key Key for the value to be created.
+ * @param valConsumer Closure to modify value associated with the key.
Object of type {@code T},
+ * passed to the closure, must not be reused anywhere else.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IndexOutOfBoundsException If index cannot be used for new
element insertion.
+ * @throws IllegalArgumentException If element with given name already
exists.
+ */
+ NamedListChange<Change> create(int index, String key, Consumer<Change>
valConsumer);
+
+ /**
+ * Create new value in named list configuration.
Review comment:
I think that this description should be more detailed, since this method
is different from `create`
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -291,87 +340,155 @@ else if (val instanceof Map)
* @param updates Tree with updates.
* @return Map of changes.
*/
- public static Map<String, Serializable> nodeToFlatMap(
- SuperRoot curRoot,
- SuperRoot updates
- ) {
+ public static Map<String, Serializable>
createFlattenedUpdatesMap(SuperRoot curRoot, SuperRoot updates) {
+ // Resulting map.
Map<String, Serializable> values = new HashMap<>();
+ // Current methods traverses two trees at the same time. In order to
reuse visitor object it's decided to
Review comment:
```suggestion
// This method traverses two trees at the same time. In order to
reuse the visitor object it's decided to
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
Review comment:
I would suggest using the `map = new HashMap<>(other.map);` (same below)
notation, it might be more effective
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
Review comment:
> Simplified map interfaces
But this is not an interface...
> with better control over the keys order
What do you mean under `better control`?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -291,87 +340,155 @@ else if (val instanceof Map)
* @param updates Tree with updates.
* @return Map of changes.
*/
- public static Map<String, Serializable> nodeToFlatMap(
- SuperRoot curRoot,
- SuperRoot updates
- ) {
+ public static Map<String, Serializable>
createFlattenedUpdatesMap(SuperRoot curRoot, SuperRoot updates) {
+ // Resulting map.
Map<String, Serializable> values = new HashMap<>();
+ // Current methods traverses two trees at the same time. In order to
reuse visitor object it's decided to
+ // use explicit stack for the left tree. We need to reuse visitor
object because it accumulates full keys.
+ Deque<InnerNode> oldInnerNodesStack = new
ArrayDeque<>(Set.of(curRoot));
+
updates.traverseChildren(new KeysTrackingConfigurationVisitor<>() {
- /** Write nulls instead of actual values. Makes sense for
deletions from named lists. */
- private boolean writeNulls;
+ /** Flag indicates that "old" and "new" trees are literally the
same at the moment. */
+ private boolean singleTreeTraversal;
+
+ /**
+ * Makes sense only if {@link #singleTreeTraversal} is {@code
true}. Helps distinguishing creation from
+ * deletion. Always {@code false} if {@link #singleTreeTraversal}
is {@code false}.
+ */
+ private boolean deletion;
/** {@inheritDoc} */
- @Override public Map<String, Serializable> doVisitLeafNode(String
key, Serializable val) {
- if (val != null)
- values.put(currentKey(), writeNulls ? null : val);
+ @Override public Void doVisitLeafNode(String key, Serializable
newVal) {
+ // Read same value from old tree.
+ Serializable oldVal =
oldInnerNodesStack.peek().traverseChild(key, leafNodeVisitor());
Review comment:
should we use `element()` instead because `peek` may return `null`?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -291,87 +340,155 @@ else if (val instanceof Map)
* @param updates Tree with updates.
* @return Map of changes.
*/
- public static Map<String, Serializable> nodeToFlatMap(
- SuperRoot curRoot,
- SuperRoot updates
- ) {
+ public static Map<String, Serializable>
createFlattenedUpdatesMap(SuperRoot curRoot, SuperRoot updates) {
Review comment:
I would suggest extracting this and similar methods into separate
classes (like `MapFlattener`, for example). All these nested classes make it
hard to read.
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -291,87 +340,155 @@ else if (val instanceof Map)
* @param updates Tree with updates.
* @return Map of changes.
*/
- public static Map<String, Serializable> nodeToFlatMap(
- SuperRoot curRoot,
- SuperRoot updates
- ) {
+ public static Map<String, Serializable>
createFlattenedUpdatesMap(SuperRoot curRoot, SuperRoot updates) {
+ // Resulting map.
Map<String, Serializable> values = new HashMap<>();
+ // Current methods traverses two trees at the same time. In order to
reuse visitor object it's decided to
+ // use explicit stack for the left tree. We need to reuse visitor
object because it accumulates full keys.
Review comment:
```suggestion
// use an explicit stack for the left tree. We need to reuse the
visitor object because it accumulates full keys.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -252,14 +255,24 @@ private InnerConfigurationSource(Map<String, ?> map) {
/** {@inheritDoc} */
@Override public void descend(ConstructableTreeNode node) {
+ boolean nodeIsNamedList = node instanceof NamedListNode;
Review comment:
I think that this method should be split in two: one for handling the
named lists and another for all other values
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -268,14 +281,50 @@ private InnerConfigurationSource(Map<String, ?> map) {
else
node.construct(key, null);
}
- else if (val instanceof Map)
+ else if (val instanceof Map) {
+ if (nodeIsNamedList) {
+ // For every named list entry modification we must
take its index into account.
+ // We do this by modifying "orderedKeys" when
index is explicitly passed.
+ Object idxObj = ((Map<?,
?>)val).get(NamedListNode.ORDER_IDX);
+
+ if (idxObj != null) {
+ assert idxObj instanceof Integer : val;
+
+ int idx = (Integer)idxObj;
+
+ if (idx >= orderedKeys.size()) {
+ // Updates can come in arbitrary order.
This means that array may be too small
+ // during batch creation. In this case we
have to insert enough nulls before
+ // invoking "add" method for actual key.
+
((ArrayList<?>)orderedKeys).ensureCapacity(idx + 1);
Review comment:
alternatively, you can write as following:
```
if (idx >= orderedKeys.size()) {
int amountOfNulls = idx - orderedKeys.size() + 1;
orderedKeys.addAll(Collections.nCopies(amountOfNulls, null));
}
orderedKeys.set(idx, key);
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
Review comment:
```suggestion
* @return Previous value associated with the key or {@code null} if the
map had no such key.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -291,87 +340,155 @@ else if (val instanceof Map)
* @param updates Tree with updates.
* @return Map of changes.
*/
- public static Map<String, Serializable> nodeToFlatMap(
- SuperRoot curRoot,
- SuperRoot updates
- ) {
+ public static Map<String, Serializable>
createFlattenedUpdatesMap(SuperRoot curRoot, SuperRoot updates) {
+ // Resulting map.
Map<String, Serializable> values = new HashMap<>();
+ // Current methods traverses two trees at the same time. In order to
reuse visitor object it's decided to
+ // use explicit stack for the left tree. We need to reuse visitor
object because it accumulates full keys.
+ Deque<InnerNode> oldInnerNodesStack = new
ArrayDeque<>(Set.of(curRoot));
Review comment:
This constructor will also set the the stack size to `2`. Are you sure
that this is intended?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
Review comment:
A slightly more effective way would be to use the value returned by
`put`:
```
V prevValue = map.put(key, value);
if (prevValue == null)
orderedKeys.add(key);
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
Review comment:
```suggestion
* Inserts a value into the map under the specified key. The key will be
positioned at the given index, shifting any existing values at that position to
the right.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
Review comment:
```suggestion
* @return Value associated with the key or {@code null} is it's not
found.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param idx Ordering index for the key. Treated as {@code 0} if
negative. Treated as last index if out of bounds.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
Review comment:
```suggestion
* @throws IllegalArgumentException If the key is already present in the
map.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param idx Ordering index for the key. Treated as {@code 0} if
negative. Treated as last index if out of bounds.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putByIndex(int idx, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ if (idx >= orderedKeys.size())
+ orderedKeys.add(key);
+ else
+ orderedKeys.add(idx, key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
Review comment:
This javadoc should be more verbose
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param idx Ordering index for the key. Treated as {@code 0} if
negative. Treated as last index if out of bounds.
Review comment:
This behavior looks counter-intuitive to me: why would we need to accept
negative and out-of-bound values like this?
##########
File path:
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/OrderedMapTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.List;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+/** Test with basic {@link OrderedMap} invariants. */
+public class OrderedMapTest {
+ /** Map instance. */
+ private OrderedMap<String> map = new OrderedMap<>();
+
+ /** Tests put/get/remove consistency on a single key. */
+ @Test
+ public void putGetRemove() {
+ assertNull(map.get("key1"));
+
+ map.put("key", "value");
+
+ assertEquals(1, map.size());
+ assertEquals("value", map.get("key"));
+
+ map.remove("key");
+
+ assertNull(map.get("key1"));
+ }
+
+ /** Tests that {@link OrderedMap#put(String, Object)} preserves order. */
+ @Test
+ public void keysOrder() {
+ map.put("key1", "value1");
+ map.put("key2", "value2");
+
+ assertEquals(2, map.size());
+ assertEquals(List.of("key1", "key2"), map.keys());
+
+ map.clear();
+
+ assertEquals(0, map.size());
+ assertEquals(List.of(), map.keys());
+
+ map.put("key2", "value2");
+ map.put("key1", "value1");
+
+ assertEquals(2, map.size());
+ assertEquals(List.of("key2", "key1"), map.keys());
+
+ map.put("key2", "value3");
+ assertEquals(List.of("key2", "key1"), map.keys());
+ }
+
+ /** Tests that {@link OrderedMap#putByIndex(int, String, Object)}
preserves order. */
+ @Test
+ public void putByIndex() {
+ map.putByIndex(0, "key1", "value1");
+
+ map.putByIndex(0, "key2", "value2");
+
+ assertEquals(List.of("key2", "key1"), map.keys());
+
+ map.putByIndex(1, "key3", "value3");
+
+ assertEquals(List.of("key2", "key3", "key1"), map.keys());
+
+ map.putByIndex(3, "key4", "value4");
+
+ assertEquals(List.of("key2", "key3", "key1", "key4"), map.keys());
+
+ map.putByIndex(5, "key5", "value5");
+
+ assertEquals(List.of("key2", "key3", "key1", "key4", "key5"),
map.keys());
+ }
+
+ /** Tests that {@link OrderedMap#putAfter(String, String, Object)}
preserves order. */
+ @Test
+ public void putAfter() {
+ map.putAfter("foo", "key1", "value1");
+
+ assertEquals(List.of("key1"), map.keys());
+
+ map.putAfter("key1", "key2", "value2");
+
+ assertEquals(List.of("key1", "key2"), map.keys());
+
+ map.putAfter("key1", "key3", "value3");
+
+ assertEquals(List.of("key1", "key3", "key2"), map.keys());
+
+ map.putAfter("key2", "key4", "value4");
+
+ assertEquals(List.of("key1", "key3", "key2", "key4"), map.keys());
+
+ map.putAfter("foo", "key5", "value5");
+
+ assertEquals(List.of("key1", "key3", "key2", "key4", "key5"),
map.keys());
+ }
+
+ /** Tests basic invariants of {@link OrderedMap#rename(String, String)}
method. */
+ @Test
+ public void rename() {
+ map.put("key1", "value1");
+ map.put("key2", "value2");
+ map.put("key3", "value3");
+
+ map.rename("key2", "key4");
+
+ assertEquals("value2", map.get("key4"));
Review comment:
I would also suggest checking that the map no longer contains a value
for `key2`
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param idx Ordering index for the key. Treated as {@code 0} if
negative. Treated as last index if out of bounds.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putByIndex(int idx, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ if (idx >= orderedKeys.size())
+ orderedKeys.add(key);
+ else
+ orderedKeys.add(idx, key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param base Previous key for the new key. Last key will be used if this
one is missing from the map.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putAfter(String base, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ int idx = orderedKeys.indexOf(base);
+
+ putByIndex(idx < 0 ? orderedKeys.size() : idx + 1, key, value);
+ }
+
+ /**
+ * Put value associated with key {@code oldKey} updaer a new key {@code
newKey}. Do nothing if {@code oldKey}
+ * was missing from the map.
+ *
+ * @param oldKey Old key.
+ * @param newKey New key.
+ *
+ * @throws IllegalArgumentException If both {@code oldKey} and {@code
newKey} already exist in the map.
+ */
+ public void rename(String oldKey, String newKey) {
+ if (!map.containsKey(oldKey))
+ return;
+
+ if (map.containsKey(newKey))
+ throw new IllegalArgumentException();
+
+ int idx = orderedKeys.indexOf(oldKey);
+
+ orderedKeys.set(idx, newKey);
+
+ V value = map.remove(oldKey);
+
+ map.put(newKey, value);
+ }
+
+ /**
+ * @return List of keys.
+ */
+ public List<String> keys() {
+ return new ArrayList<>(orderedKeys);
+ }
+
+ /**
+ * Reorders keys in the map.
+ *
+ * @param orderedKeys List of keys in new order. Must have the same set of
keys in it.
+ */
+ public void reorderKeys(List<String> orderedKeys) {
+ assert map.keySet().equals(new HashSet<>(orderedKeys)) : map.keySet()
+ " : " + orderedKeys;
Review comment:
you can use `Set.copyOf` instead of `new HashSet`, it's a little bit
more clean
##########
File path:
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/OrderedMapTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.List;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+/** Test with basic {@link OrderedMap} invariants. */
+public class OrderedMapTest {
+ /** Map instance. */
+ private OrderedMap<String> map = new OrderedMap<>();
Review comment:
can be `final`
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param idx Ordering index for the key. Treated as {@code 0} if
negative. Treated as last index if out of bounds.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putByIndex(int idx, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ if (idx >= orderedKeys.size())
+ orderedKeys.add(key);
+ else
+ orderedKeys.add(idx, key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param base Previous key for the new key. Last key will be used if this
one is missing from the map.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putAfter(String base, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ int idx = orderedKeys.indexOf(base);
+
+ putByIndex(idx < 0 ? orderedKeys.size() : idx + 1, key, value);
+ }
+
+ /**
+ * Put value associated with key {@code oldKey} updaer a new key {@code
newKey}. Do nothing if {@code oldKey}
Review comment:
```suggestion
* Re-associates the value under the {@code oldKey} to the {@code
newKey}. Does nothing if the {@code oldKey}
* is not present in the map.
```
##########
File path:
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/OrderedMapTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.List;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+/** Test with basic {@link OrderedMap} invariants. */
+public class OrderedMapTest {
+ /** Map instance. */
+ private OrderedMap<String> map = new OrderedMap<>();
+
+ /** Tests put/get/remove consistency on a single key. */
+ @Test
+ public void putGetRemove() {
+ assertNull(map.get("key1"));
+
+ map.put("key", "value");
+
+ assertEquals(1, map.size());
+ assertEquals("value", map.get("key"));
+
+ map.remove("key");
+
+ assertNull(map.get("key1"));
+ }
+
+ /** Tests that {@link OrderedMap#put(String, Object)} preserves order. */
+ @Test
+ public void keysOrder() {
+ map.put("key1", "value1");
+ map.put("key2", "value2");
+
+ assertEquals(2, map.size());
+ assertEquals(List.of("key1", "key2"), map.keys());
+
+ map.clear();
+
+ assertEquals(0, map.size());
+ assertEquals(List.of(), map.keys());
+
+ map.put("key2", "value2");
+ map.put("key1", "value1");
+
+ assertEquals(2, map.size());
+ assertEquals(List.of("key2", "key1"), map.keys());
+
+ map.put("key2", "value3");
+ assertEquals(List.of("key2", "key1"), map.keys());
+ }
+
+ /** Tests that {@link OrderedMap#putByIndex(int, String, Object)}
preserves order. */
+ @Test
+ public void putByIndex() {
+ map.putByIndex(0, "key1", "value1");
+
+ map.putByIndex(0, "key2", "value2");
+
+ assertEquals(List.of("key2", "key1"), map.keys());
+
+ map.putByIndex(1, "key3", "value3");
+
+ assertEquals(List.of("key2", "key3", "key1"), map.keys());
+
+ map.putByIndex(3, "key4", "value4");
+
+ assertEquals(List.of("key2", "key3", "key1", "key4"), map.keys());
+
+ map.putByIndex(5, "key5", "value5");
+
+ assertEquals(List.of("key2", "key3", "key1", "key4", "key5"),
map.keys());
+ }
+
+ /** Tests that {@link OrderedMap#putAfter(String, String, Object)}
preserves order. */
+ @Test
+ public void putAfter() {
+ map.putAfter("foo", "key1", "value1");
+
+ assertEquals(List.of("key1"), map.keys());
+
+ map.putAfter("key1", "key2", "value2");
+
+ assertEquals(List.of("key1", "key2"), map.keys());
+
+ map.putAfter("key1", "key3", "value3");
+
+ assertEquals(List.of("key1", "key3", "key2"), map.keys());
+
+ map.putAfter("key2", "key4", "value4");
+
+ assertEquals(List.of("key1", "key3", "key2", "key4"), map.keys());
+
+ map.putAfter("foo", "key5", "value5");
+
+ assertEquals(List.of("key1", "key3", "key2", "key4", "key5"),
map.keys());
+ }
+
+ /** Tests basic invariants of {@link OrderedMap#rename(String, String)}
method. */
+ @Test
+ public void rename() {
+ map.put("key1", "value1");
+ map.put("key2", "value2");
+ map.put("key3", "value3");
+
+ map.rename("key2", "key4");
+
+ assertEquals("value2", map.get("key4"));
+
+ assertEquals(List.of("key1", "key4", "key3"), map.keys());
+ }
+
+ /** Tests that {@link OrderedMap#reorderKeys(List)} reorders keys
properly. */
+ @Test
+ public void reorderKeys() {
Review comment:
You only test the happy cases but not the error cases. Same applies to
other tests
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/tree/NamedListOrderTest.java
##########
@@ -0,0 +1,176 @@
+/*
+ * 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.ignite.internal.configuration.tree;
+
+import java.util.Map;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.Value;
+import org.apache.ignite.internal.configuration.ConfigurationChanger;
+import org.apache.ignite.internal.configuration.TestConfigurationChanger;
+import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
+import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class NamedListOrderTest {
+ /** */
+ @ConfigurationRoot(rootName = "a")
+ public static class AConfigurationSchema {
+ /** */
+ @NamedConfigValue
+ public BConfigurationSchema b;
+ }
+
+ /** */
+ @Config
+ public static class BConfigurationSchema {
+ /** */
+ @Value(hasDefault = true)
+ public String c = "foo";
+
+ @NamedConfigValue
+ public BConfigurationSchema b;
+ }
+
+ private static ConfigurationAsmGenerator cgen;
Review comment:
missing javadocs everywhere
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -291,87 +340,155 @@ else if (val instanceof Map)
* @param updates Tree with updates.
* @return Map of changes.
*/
- public static Map<String, Serializable> nodeToFlatMap(
- SuperRoot curRoot,
- SuperRoot updates
- ) {
+ public static Map<String, Serializable>
createFlattenedUpdatesMap(SuperRoot curRoot, SuperRoot updates) {
+ // Resulting map.
Map<String, Serializable> values = new HashMap<>();
+ // Current methods traverses two trees at the same time. In order to
reuse visitor object it's decided to
+ // use explicit stack for the left tree. We need to reuse visitor
object because it accumulates full keys.
Review comment:
What is a `left tree`? What are `full keys`?
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/tree/NamedListOrderTest.java
##########
@@ -0,0 +1,176 @@
+/*
+ * 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.ignite.internal.configuration.tree;
+
+import java.util.Map;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.Value;
+import org.apache.ignite.internal.configuration.ConfigurationChanger;
+import org.apache.ignite.internal.configuration.TestConfigurationChanger;
+import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
+import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class NamedListOrderTest {
+ /** */
+ @ConfigurationRoot(rootName = "a")
+ public static class AConfigurationSchema {
+ /** */
+ @NamedConfigValue
+ public BConfigurationSchema b;
+ }
+
+ /** */
+ @Config
+ public static class BConfigurationSchema {
+ /** */
+ @Value(hasDefault = true)
+ public String c = "foo";
+
+ @NamedConfigValue
+ public BConfigurationSchema b;
+ }
+
+ private static ConfigurationAsmGenerator cgen;
+
+ private TestConfigurationStorage storage;
+
+ private ConfigurationChanger changer;
+
+ @BeforeAll
+ public static void beforeAll() {
+ cgen = new ConfigurationAsmGenerator();
+ }
+
+ @AfterAll
+ public static void afterAll() {
+ cgen = null;
+ }
+
+ @BeforeEach
+ public void before() {
+ storage = new TestConfigurationStorage();
+
+ changer = new TestConfigurationChanger(cgen);
+ changer.addRootKey(AConfiguration.KEY);
+ changer.register(storage);
+ }
+
+ @AfterEach
+ public void after() {
+ changer.stop();
+ }
+
+ @Test
+ public void test() throws Exception {
Review comment:
and, as usual, you only test the happy case
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/OrderedMap.java
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ignite.internal.configuration.util;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Simplified map interfaces with better control over the keys order.
+ *
+ * @param <V> Type of the value.
+ */
+public class OrderedMap<V> {
+ /** Underlying hash map. */
+ private final Map<String, V> map = new HashMap<>();
+
+ /** Ordered keys. */
+ private final List<String> orderedKeys = new ArrayList<>();
+
+ /** Default constructor. */
+ public OrderedMap() {
+ }
+
+ /**
+ * Copy constructor.
+ *
+ * @param other Source of keys/values to copy from.
+ */
+ public OrderedMap(OrderedMap<V> other) {
+ map.putAll(other.map);
+ orderedKeys.addAll(other.orderedKeys);
+ }
+
+ /**
+ * Same as {@link Map#containsKey(Object)}.
+ *
+ * @param key Key to check.
+ * @return {@code true} if map contains the key.
+ */
+ public boolean containsKey(String key) {
+ return map.containsKey(key);
+ }
+
+ /**
+ * Same as {@link Map#get(Object)}.
+ *
+ * @param key Key to search.
+ * @return Value assiciated with the key or {@code null} is it's not found.
+ */
+ public V get(String key) {
+ return map.get(key);
+ }
+
+ /**
+ * Same as {@link Map#remove(Object)}.
+ *
+ * @param key Key to remove.
+ * @return Provious value assiciated with the key or {@code null} if map
had no such key.
+ */
+ public V remove(String key) {
+ if (map.containsKey(key))
+ orderedKeys.remove(key);
+
+ return map.remove(key);
+ }
+
+ /**
+ * Put value to the map. Key will be the last if it didn't exist in map
before. If key did exist then the same
+ * ordering index will be used.
+ *
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ */
+ public void put(String key, V value) {
+ if (!map.containsKey(key))
+ orderedKeys.add(key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param idx Ordering index for the key. Treated as {@code 0} if
negative. Treated as last index if out of bounds.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putByIndex(int idx, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ if (idx >= orderedKeys.size())
+ orderedKeys.add(key);
+ else
+ orderedKeys.add(idx, key);
+
+ map.put(key, value);
+ }
+
+ /**
+ * Put value to the map.
+ *
+ * @param base Previous key for the new key. Last key will be used if this
one is missing from the map.
+ * @param key Key to put.
+ * @param value Value associated with the key.
+ *
+ * @throws IllegalArgumentException If key already exists in the map.
+ */
+ public void putAfter(String base, String key, V value) {
+ if (map.containsKey(key))
+ throw new IllegalArgumentException("Key " + key + " already
exists.");
+
+ int idx = orderedKeys.indexOf(base);
+
+ putByIndex(idx < 0 ? orderedKeys.size() : idx + 1, key, value);
+ }
+
+ /**
+ * Put value associated with key {@code oldKey} updaer a new key {@code
newKey}. Do nothing if {@code oldKey}
Review comment:
> Do nothing if
Again, this behavior looks quite cryptic to me, is it necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
