tkobayas commented on code in PR #6780:
URL:
https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3503448123
##########
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:
@drccrd I'm sorry that I might be misunderstanding the intention behind this
fix. If you think it's fine with `passing no fields when no Position
annotations are present`, it's good with the Copilot's suggestion. It would be
concise and if someone wants to use inherited fields without `@Position`, they
can open a PR then.
If you want to use inherited fields without `@Position`, then:
> using setters rather than a constructor
This is still useful to avoid the all-arg constructor issue.
> using the position annotation explicitly allows this sorting ambiguity to
be clarified
yes, it's great.
> a design decision still need to be made for the cases without annotation
With non-exec-model (the legacy approach), the constructor arg order is
non-deterministic, because ClassFieldInspectorImpl.getterMethods is HashMap.
It's not good, but if you don't need to fix it, you can leave it, because
non-exec-model has been deprecated. (We don't eagerly fix issues)
With exec-model, `instanceFields` comes from Class.getDeclaredFields() which
is non-deterministic. I'm not opinionated, but we may sort them with
alphabetical order here.
> Maybe also allowing to exclude fields with a negative position would
provide more flexibility.
Agreed.
I'll leave the decision on the scope of this PR to you. Thanks!
--
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]
