ibessonov commented on code in PR #2016:
URL: https://github.com/apache/ignite-3/pull/2016#discussion_r1184607313
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java:
##########
@@ -245,12 +246,12 @@ public <T> T represent(List<String> path,
ConfigurationVisitor<T> visitor) throw
}
if (node instanceof TraversableTreeNode) {
- return ((TraversableTreeNode) node).accept(null, visitor);
+ return ((TraversableTreeNode) node).accept(null, null, visitor);
Review Comment:
I remember that we were discussing this place. "represent" must always pass
a field, otherwise user may provide a path to password configuration and you
won't obscure it.
`find` must return not only the object, but also a meta-information about
it. This is the only way to do it. You also should add tests for such scenario.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -306,6 +317,14 @@ private ClassDefinition createNodeClass() {
}
}
+ MethodDefinition classInitializer =
innerNodeClassDef.getClassInitializer();
+ dieldToDieldDefinitionMap.forEach((k, v) -> {
+ // get declared field
+ BytecodeExpression invoke = constantClass(k.getDeclaringClass())
Review Comment:
I would rename this "invoke" to "getDeclaredFieldExp" or something similar
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -163,24 +166,30 @@ class InnerNodeAsmGenerator extends AbstractAsmGenerator {
/** {@link InnerNode#assertMutability()}. */
private static final Method ASSERT_MUTABILITY_MTD;
+ /** {@link Class#getDeclaredField(String)}. */
+ private static final Method GET_DECLARED_FIELD_MTD;
+
/** {@code Node#convert} method name. */
private static final String CONVERT_MTD_NAME = "convert";
/** {@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<>();
Review Comment:
"dield"? Please fix typos in the name.
Also, the fact that there is a mapping from field to field definition is
self-evident from the signature.
Please expand the comment and explain the meaning behind these definitions.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -514,6 +533,11 @@ private FieldDefinition addInnerNodeField(Field
schemaField) {
throw new IllegalArgumentException("Unsupported field: " +
schemaField);
}
+ dieldToDieldDefinitionMap.put(
+ schemaField,
+ innerNodeClassDef.declareField(EnumSet.of(PUBLIC, STATIC,
FINAL), fieldName + "FieldDefinition", Field.class)
Review Comment:
`fieldName + "FieldDefinition"` - poor naming. What do you mean by field
definition here? It looks more list "schema field". I would also make it upper
case, because it's a `static final` field.
Doesn't it break "internal extensions"? They may have repeating properties
names, don't we have such tests? Please check
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -306,6 +317,14 @@ private ClassDefinition createNodeClass() {
}
}
+ MethodDefinition classInitializer =
innerNodeClassDef.getClassInitializer();
+ dieldToDieldDefinitionMap.forEach((k, v) -> {
+ // get declared field
Review Comment:
```suggestion
// Get declared field.
```
--
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]
