drccrd commented on code in PR #6780:
URL:
https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3500820145
##########
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/declaredtype/DescrTypeDefinition.java:
##########
@@ -161,7 +165,45 @@ private static Optional<TypeDeclarationDescr>
getSuperType(TypeDeclarationDescr
@Override
public List<FieldDefinition> findInheritedDeclaredFields() {
- return findInheritedDeclaredFields(new ArrayList<>(),
getSuperType(typeDeclarationDescr, packageDescr));
+ List<FieldDefinition> fields = findInheritedDeclaredFields(new
ArrayList<>(), getSuperType(typeDeclarationDescr, packageDescr));
+ if (fields.isEmpty()) {
+ abstractClass.ifPresent(superClass ->
fields.addAll(inheritedFieldsFromSuperClass(superClass)));
+ }
+ return fields;
+ }
+
+ /**
+ * Collects the positional fields of a resolved Java superclass, walking
the class hierarchy.
+ * A field participates only when it carries {@link Position} (explicit
opt-in), ordered by its
+ * position value; this deterministically excludes non-positional members
(and keeps the
+ * generated {@code super(...)} call aligned with a positional constructor
on the superclass).
+ * When the superclass declares no {@link Position} at all, fall back to
all non-static instance
+ * fields in declaration order (top-most ancestor first).
+ */
+ private List<FieldDefinition> inheritedFieldsFromSuperClass(Class<?>
superClass) {
+ List<Class<?>> hierarchy = new ArrayList<>();
+ for (Class<?> c = superClass; c != null && c != Object.class; c =
c.getSuperclass()) {
+ hierarchy.add(0, c);
+ }
+
+ List<Field> instanceFields = new ArrayList<>();
+ for (Class<?> c : hierarchy) {
+ for (Field f : c.getDeclaredFields()) {
+ if (!Modifier.isStatic(f.getModifiers())) {
+ instanceFields.add(f);
+ }
+ }
+ }
+
+ List<Field> positioned = instanceFields.stream()
+ .filter(f -> f.getAnnotation(Position.class) != null)
+ .sorted(Comparator.comparingInt(f ->
f.getAnnotation(Position.class).value()))
+ .collect(Collectors.toList());
+
+ List<Field> chosen = positioned.isEmpty() ? instanceFields :
positioned;
+ return chosen.stream()
+ .map(f -> (FieldDefinition) new
DescrFieldDefinition(f.getName(), f.getType().getCanonicalName(), null))
+ .collect(Collectors.toList());
Review Comment:
@tkobayas thank you for the clarification - I went back into the code to
look at all the details, including how the legacy model works in this aspect.
And I think an important question is if we want the executable model behavior
in this point to align with the legacy model - which I think it doesn't
currently anyway.
The legacy model uses the empty constructor for the super class and then
setters for the fields, so it doesn't rely on the java constructor their. But
the generated constructor that is available on the drl side is less
straightforward.
In populateDefinitionFromClass the super fields are collected based on if
they have both a getter and a setter, and are then sorted (by a number of
things). This means that in the generated constructor for the drl files the
java-inherited field order doesn't (necessarily) match any constructor defined
in the java class itself - which is not that big a deal but I have definitely
spent some hours over the years trying to figure out the behavior, and had to
debug the code to find out what constructor I had available.
So I agree with your point and will make the change (i.e. not only using
annotated fields, and using setters rather than a constructor here), and using
the position annotation explicitly allows this sorting ambiguity to be
clarified, but I think a design decision still need to be made for the cases
without annotation - and I guess since nobody has encountered this before it is
not a common use case.
Maybe also allowing to exclude fields with a negative position would provide
more flexibility.
So: shall we match the legacy model for the generated constructor
definition, or take some other approach?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
