You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "drccrd (via GitHub)" <gi...@apache.org> on 2026/06/26 22:50:36 UTC
[PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
drccrd opened a new pull request, #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780
Fixes: #6779
Adds java bean fields annotated with @Position to declared subtypes under the executable model
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3503560324
##########
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:
One more point:
`testExtendPojoInheritedFieldsConstructor` needs
```
"declare PositionalParent end\n" +
```
to align with the usage explained in https://github.com/apache/incubator-kie-docs/blob/main/drools-docs/src/modules/ROOT/pages/language-reference-traditional/_drl-declarations-extended-con.adoc
In reality, it works without `declare PositionalParent end` because `ClassHierarchyManager.mergeFields` has a path to find an external class. But it's better to stick with the documented way.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3503560324
##########
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:
One more point:
`testExtendPojoInheritedFieldsConstructor` needs
```
"declare PositionalParent end\n" +
```
to align with the usage explained in https://github.com/apache/incubator-kie-docs/blob/main/drools-docs/src/modules/ROOT/pages/language-reference-traditional/_drl-declarations-extended-con.adoc
In reality, it works without `declare PositionalParent end` because `ClassHierarchyManager.mergeFields` has a path to find an external class. But it's better to stick with the documented way.
I found a subtle caveat case : https://github.com/apache/incubator-kie-drools/issues/6790
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3490664590
##########
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 do you agree with passing no fields when no Position annotations are present? That would align with behavior till now, as I see it, but would still be drift from the legacy compilation model. I assume this is appropriate but I have had much to do with the executable model until now.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3490664590
##########
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 do you agree with passing no fields when no Position annotations are present? That would align with behavior till now, as I see it, but would still be drift from the legacy compilation model. I assume this is appropriate but I have not had much to do with the executable model until now (as we are just starting looking into the performance benefit now).
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3523833306
##########
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! If we can just carry the explicit Position annotated extended-java-constructor it works for my use case, and I personally think it is cleaner. In the legacy model we had actually redeclared some java classes in drools with their fields to ensure deterministic constructor order, which is now not necessary.
So as you said: if somebody wants to support the java-inherited-fields in the same manner as the legacy model they can open a new new PR.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "daniel-clark-mint (via GitHub)" <gi...@apache.org>.
daniel-clark-mint commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3498740557
##########
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas merged PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3490149299
##########
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:
hmm, this suggestion seems to explain the `PropertyReactivityTest.testPRWithCollections` test failure.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3489853777
##########
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:
`inheritedFieldsFromSuperClass` falls back to *all* instance fields when no `@Position` annotations are present (via `positioned.isEmpty() ? instanceFields : positioned`). Those inherited fields are unconditionally turned into constructor parameters and passed to `super(...)` by `GeneratedConstructor`, which will almost always fail to compile unless the Java superclass happens to have an all-args constructor matching every instance field (and in reflection order). This introduces a likely regression for declared types extending regular POJOs without `@Position`.
Given the stated intent (only include superclass fields explicitly opted-in with `@Position`), it’s safer to return an empty list when no positioned fields are found so the generated constructor uses `super()` instead of guessing a signature.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "daniel-clark-mint (via GitHub)" <gi...@apache.org>.
daniel-clark-mint commented on code in PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#discussion_r3498740557
##########
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [6779] Exec model: include Java superclass @Position fields in genera… [incubator-kie-drools]
Posted by "drccrd (via GitHub)" <gi...@apache.org>.
drccrd commented on PR #6780:
URL: https://github.com/apache/incubator-kie-drools/pull/6780#issuecomment-4897375890
@tkobayas I added a section to the documentation, if you would also have a moment to review: https://github.com/apache/incubator-kie-docs/pull/4594
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org