[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2016: IGNITE-19165: Extend ConfigurationVisitor's methods with Field

["ibessonov (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org