ibessonov commented on code in PR #2016:
URL: https://github.com/apache/ignite-3/pull/2016#discussion_r1185926420
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -318,11 +319,11 @@ private ClassDefinition createNodeClass() {
}
MethodDefinition classInitializer =
innerNodeClassDef.getClassInitializer();
- dieldToDieldDefinitionMap.forEach((k, v) -> {
- // get declared field
- BytecodeExpression invoke = constantClass(k.getDeclaringClass())
+ fieldToFieldDefinitionMap.forEach((k, v) -> {
+ // Get declared field
Review Comment:
You forgot the dot at the end.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java:
##########
@@ -238,20 +239,21 @@ public <V, C, T extends ConfigurationTree<V, C>> T
getConfiguration(RootKey<T, V
public <T> T represent(List<String> path, ConfigurationVisitor<T> visitor)
throws IllegalArgumentException {
SuperRoot superRoot = changer.superRoot();
- Object node;
+ NodeValue<?> node;
try {
node = ConfigurationUtil.find(path, superRoot, false);
} catch (KeyNotFoundException e) {
throw new IllegalArgumentException(e.getMessage());
}
- if (node instanceof TraversableTreeNode) {
- return ((TraversableTreeNode) node).accept(null, null, visitor);
+ Object value = node.value();
+ if (value instanceof TraversableTreeNode) {
+ return ((TraversableTreeNode) value).accept(null, null, visitor);
Review Comment:
Why don't you pass the field here? I thought we need it
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java:
##########
@@ -204,29 +204,94 @@ public void findSuccessfully() {
)
);
+ NodeValue<Object> root = find(List.of(), parentNode, true);
+ assertNull(root.field());
+ assertSame(parentNode, root.value());
+
+ NodeValue<Object> elements = find(List.of("elements"), parentNode,
true);
+ assertNotNull(elements.field());
+ assertSame(parentChange.elements(), elements.value());
+
+ NodeValue<Object> elementsName = find(List.of("elements", "name"),
parentNode, true);
+ assertNotNull(elementsName.field());
+ assertSame(parentChange.elements().get("name"), elementsName.value());
+
+ NodeValue<Object> elementsNameChild = find(List.of("elements", "name",
"child"), parentNode, true);
+ assertNotNull(elementsNameChild.field());
+ assertSame(parentChange.elements().get("name").child(),
elementsNameChild.value());
+
+ NodeValue<Object> elementsNameChildStr = find(List.of("elements",
"name", "child", "str"), parentNode, true);
+ assertNotNull(elementsNameChildStr.field());
+ assertSame(parentChange.elements().get("name").child().str(),
elementsNameChildStr.value());
+ }
+
+ @Test
+ public void findSuccessfullyPolymorphicConfig() {
+ InnerNode parentNode =
newNodeInstance(PolymorphicRootConfigurationSchema.class);
+ addDefaults(parentNode);
+
+ PolymorphicRootChange parentChange = (PolymorphicRootChange)
parentNode;
+
+ parentChange.changePolymorphicSubCfg(sub -> {
+ sub.convert(FirstPolymorphicInstanceChange.class)
+ .changeStrVal("str")
+ .changeLongVal(0);
+ });
+
+ parentChange.changePolymorphicNamedCfg(named -> {
+ named.createOrUpdate("name0", sub -> {
+ sub.convert(FirstPolymorphicInstanceChange.class)
+ .changeStrVal("substr0")
+ .changeLongVal(-1);
+ });
+
+ named.createOrUpdate("name1", sub -> {
+ sub.convert(SecondPolymorphicInstanceChange.class)
+ .changeIntVal(1)
+ .changeLongVal(1);
+ });
+ });
+
+ NodeValue<Object> subCfgStrVal = find(List.of("polymorphicSubCfg",
"strVal"), parentNode, true);
+ assertSame(String.class, subCfgStrVal.field().getGenericType());
Review Comment:
Is there a practical reason of using "getGenericType()" instead of
"getType()"?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java:
##########
@@ -204,29 +204,94 @@ public void findSuccessfully() {
)
);
+ NodeValue<Object> root = find(List.of(), parentNode, true);
+ assertNull(root.field());
+ assertSame(parentNode, root.value());
+
+ NodeValue<Object> elements = find(List.of("elements"), parentNode,
true);
+ assertNotNull(elements.field());
+ assertSame(parentChange.elements(), elements.value());
+
+ NodeValue<Object> elementsName = find(List.of("elements", "name"),
parentNode, true);
+ assertNotNull(elementsName.field());
+ assertSame(parentChange.elements().get("name"), elementsName.value());
+
+ NodeValue<Object> elementsNameChild = find(List.of("elements", "name",
"child"), parentNode, true);
+ assertNotNull(elementsNameChild.field());
+ assertSame(parentChange.elements().get("name").child(),
elementsNameChild.value());
+
+ NodeValue<Object> elementsNameChildStr = find(List.of("elements",
"name", "child", "str"), parentNode, true);
+ assertNotNull(elementsNameChildStr.field());
+ assertSame(parentChange.elements().get("name").child().str(),
elementsNameChildStr.value());
+ }
+
+ @Test
+ public void findSuccessfullyPolymorphicConfig() {
Review Comment:
We need a test where polymorphic extensions would have a sub-configuration
with the same name, that is not primitive in both cases. I bet it wouldn't work.
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java:
##########
@@ -204,29 +204,94 @@ public void findSuccessfully() {
)
);
+ NodeValue<Object> root = find(List.of(), parentNode, true);
+ assertNull(root.field());
Review Comment:
Can we assert that it's actual field that we expect? We can check its name
and declaring class, for example. NotNull is not enough
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -175,8 +176,8 @@ class InnerNodeAsmGenerator extends AbstractAsmGenerator {
/** {@link ConstructableTreeNode#construct(String, ConfigurationSource,
boolean)} method name. */
private static final String CONSTRUCT_MTD_NAME = "construct";
- /** {@link Field} to {@link FieldDefinition} map. */
- private final Map<Field, FieldDefinition> dieldToDieldDefinitionMap = new
HashMap<>();
+ /** Mapping for each configuration schema node to it's {@link
FieldDefinition}. */
+ private final Map<Field, FieldDefinition> fieldToFieldDefinitionMap = new
HashMap<>();
Review Comment:
Still not clear, but I know that this class is not documented properly, so I
can't ask for too much.
_"it's FieldDefinition"_ doesn't say what this definition means, and where
it is. It's like saying _"maps field to its string"_ instead of _"... to its
name"_.
By the way, **"its"**, not **"it's"**
--
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]
