pvillard31 commented on code in PR #11095:
URL: https://github.com/apache/nifi/pull/11095#discussion_r3028699582
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/main/python/framework/ProcessorInspection.py:
##########
@@ -268,6 +277,14 @@ def get_imported_property_descriptors_from_ast(root_node:
ast.AST, module_file:
if imported_name in source_descriptors:
imported_descriptors[imported_name] =
source_descriptors[imported_name]
logger.debug(f"Resolved imported PropertyDescriptor
'{imported_name}' from {source_file}")
+ else:
+ # The imported name is not itself a PropertyDescriptor (e.g. it is
a class).
+ # Expose all PropertyDescriptors found in that source module so
that
+ # attribute-style references like ParentClass.MY_PROP can be
resolved.
+ for desc_name, desc in source_descriptors.items():
+ if desc_name not in imported_descriptors:
+ imported_descriptors[desc_name] = desc
+ logger.debug(f"Resolved PropertyDescriptor '{desc_name}'
from class '{imported_name}' in {source_file}")
Review Comment:
I think this else if overly broad. Maybe something along the lines of
```python
else:
# Check if the imported name is a class in the source module
source_classes = [n.name for n in ast.walk(source_ast) if isinstance(n,
ast.ClassDef)]
if imported_name in source_classes:
for desc_name, desc in source_descriptors.items():
if desc_name not in imported_descriptors:
imported_descriptors[desc_name] = desc
```
This would prevent `from utils import helper_function` from pulling in
unrelated `PropertyDescriptors`, while still supporting the `from ParentClass
import ParentClass` pattern. However, this would require caching `source_ast`
alongside the existing `parsed_modules` cache (currently only
`source_descriptors` is cached). Thoughts?
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/main/python/framework/ProcessorInspection.py:
##########
@@ -459,8 +476,13 @@ def get_property_descriptions(class_node,
module_string_constants, module_file,
visitor = CollectPropertyDescriptorVisitors(module_string_constants,
class_node.name, imported_property_descriptors)
visitor.visit(class_node)
- return visitor.discovered_property_descriptors.values()
+ # Merge parent/imported descriptors that are not already defined in the
child class.
+ merged = dict(visitor.discovered_property_descriptors)
+ for name, desc in imported_property_descriptors.items():
+ if name not in merged:
+ merged[name] = desc
+ return merged.values()
Review Comment:
I think this may be too aggressive. Merging all imported descriptors into
the final property descriptions means that any module imported for a
non-descriptor purpose could contribute phantom properties to the manifest. A
more conservative approach would be to only merge descriptors that are
explicitly referenced in the class's `property_descriptors` list or in
dependency declarations. Alternatively, the merge could be limited to
descriptors that were actually used during dependency resolution (only those
that were looked up and found in `resolve_dependencies`).
--
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]
