Copilot commented on code in PR #7216:
URL: https://github.com/apache/incubator-kie/pull/7216#discussion_r4323492250
##########
drools-compiler/src/main/java/org/drools/compiler/builder/impl/TypeDeclarationFactory.java:
##########
@@ -66,44 +65,26 @@ public TypeDeclaration processTypeDeclaration(
PackageRegistry pkgRegistry,
type = new TypeDeclaration( typeDescr.getTypeName() );
type.setResource( typeDescr.getResource() );
- // if is not new, search the already existing declaration and
- // compare them o see if they are at least compatibles
- // check whether it is necessary to build the class or not
Class<?> existingClass =
TypeDeclarationUtils.getExistingDeclarationClass( typeDescr, pkgRegistry );
type.setTypeClass( existingClass );
type.setNovel( existingClass == null );
type.setNature( existingClass == null ?
TypeDeclaration.Nature.DEFINITION : TypeDeclaration.Nature.DECLARATION );
-
- // Pre-populate from Java class annotations so that
finalizeConfigurator()
- // (which runs before wireFieldAccessors) sees the correct role,
expiration,
- // reactivity, and dynamic settings. DRL annotations applied next
will override.
- if ( existingClass != null ) {
- TypeDeclaration.processTypeAnnotations( type,
- new Annotated.ClassAdapter( existingClass ),
- context.getBuilderConfiguration().getOption(
PropertySpecificOption.KEY ) );
- }
}
- processTypeAnnotations(typeDescr, type);
- return type;
- }
-
- private void processTypeAnnotations( AbstractClassTypeDeclarationDescr
typeDescr, TypeDeclaration type ) {
try {
- processAnnotations( typeDescr, type );
+ processAnnotations( typeDescr, type,
context.getBuilderConfiguration().getOption( PropertySpecificOption.KEY ) );
} catch (Exception e) {
results.addBuilderResult(new TypeDeclarationError(typeDescr,
e.getMessage() ) );
}
+ return type;
}
- // Processes annotations from the DRL descriptor, overriding values
pre-populated from
- // Java class annotations in processTypeDeclaration(). Runs before
finalizeConfigurator(),
- // which needs @Role and @PropertyChangeSupport already set.
- public static void processAnnotations( AbstractClassTypeDeclarationDescr
typeDescr, TypeDeclaration type ) {
- Role role = getTypedAnnotation(typeDescr, Role.class);
- if (role != null) {
- type.setRole(role.value());
+ public static void processAnnotations( AbstractClassTypeDeclarationDescr
typeDescr, TypeDeclaration type, PropertySpecificOption propertySpecificOption
) {
+ Annotated annotated = toAnnotated(typeDescr);
+ if (type.getTypeClass() != null) {
+ annotated = Annotated.withFallback(annotated, new
Annotated.ClassAdapter(type.getTypeClass()));
}
+ TypeDeclaration.processTypeAnnotations( type, annotated,
propertySpecificOption );
TypeSafe typeSafe = getTypedAnnotation(typeDescr, TypeSafe.class);
if (typeSafe != null) {
Review Comment:
This method builds a merged `Annotated` (DRL with Java fallback) but then
reads `@TypeSafe` from `typeDescr` directly. This undermines the consolidation
goal and can produce inconsistent behavior vs other annotations (e.g., Java
fallback won’t apply for `@TypeSafe`). Consider (mandatory) sourcing `TypeSafe`
via the same merged `annotated` (e.g.,
`annotated.getTypedAnnotation(TypeSafe.class)`) to keep all type annotation
resolution consistent.
##########
drools-compiler/src/main/java/org/drools/compiler/builder/impl/TypeDeclarationFactory.java:
##########
@@ -115,10 +96,10 @@ public static void processAnnotations(
AbstractClassTypeDeclarationDescr typeDes
} else if (typeDescr instanceof TypeDeclarationDescr &&
((TypeDeclarationDescr)typeDescr).isTrait()) {
type.setKind(TypeDeclaration.Kind.TRAIT);
}
+ }
- if (typeDescr.hasAnnotation(PropertyChangeSupport.class)) {
- type.setDynamic(true);
- }
+ public static void processAnnotations( AbstractClassTypeDeclarationDescr
typeDescr, TypeDeclaration type ) {
+ processAnnotations( typeDescr, type, PropertySpecificOption.ALWAYS );
}
Review Comment:
The 2-arg `processAnnotations` overload now hardcodes
`PropertySpecificOption.ALWAYS`, which can unintentionally change behavior for
existing callers (it used to only apply a small subset of annotations).
Consider (mandatory) either: (1) deprecating/removing this overload and
requiring callers to pass `PropertySpecificOption`, or (2) preserving prior
semantics by delegating to a default derived from configuration rather than
forcing `ALWAYS`.
##########
drools-base/src/main/java/org/drools/base/rule/TypeDeclaration.java:
##########
@@ -594,9 +598,22 @@ private static void configurePropertyReactivity(
TypeDeclaration type, Annotated
}
private static void configurePropertyChangeSupport( TypeDeclaration type,
Annotated annotated ) {
- // only ever switch it on: a DRL declaration may already have made the
type dynamic
if ( annotated.hasAnnotation( PropertyChangeSupport.class ) ) {
type.setDynamic( true );
}
}
+
+ private static void configureTimestampAttribute( TypeDeclaration type,
Annotated annotated ) {
+ Timestamp timestamp = annotated.getTypedAnnotation(Timestamp.class);
+ if (timestamp != null) {
+ type.setTimestampAttribute( timestamp.value() );
+ }
+ }
+
+ private static void configureDurationAttribute( TypeDeclaration type,
Annotated annotated ) {
+ Duration duration = annotated.getTypedAnnotation(Duration.class);
+ if (duration != null) {
+ type.setDurationAttribute( duration.value() );
+ }
+ }
Review Comment:
`timestamp.value()` / `duration.value()` are now evaluated during
`processTypeAnnotations()`. Previously, evaluation was guarded (and converted
into a `TypeDeclarationError`) at wiring time. If `getTypedAnnotation(...)`
returns proxies that can throw when reading values for DRL-provided
annotations, this can now surface as uncaught exceptions in call paths that
don’t wrap `processTypeAnnotations()` in builder error handling (e.g.,
bean-based creation paths). Consider (mandatory) catching runtime exceptions
here and rethrowing an exception type that upstream consistently converts into
a `TypeDeclarationError`, or otherwise ensuring all callers are covered by a
builder-result conversion.
##########
drools-compiler/src/main/java/org/drools/compiler/builder/impl/TypeDeclarationFactory.java:
##########
@@ -253,8 +234,9 @@ protected int compareTypeDeclarations(TypeDeclaration
oldDeclaration,
}
}
- //different duration -> Incompatible
- if (!nullSafeEqualityComparison(oldDeclaration.getDurationAttribute(),
newDeclaration.getDurationAttribute())) {
+ //different duration -> Incompatible (only when both declare one)
+ if (oldDeclaration.getDurationAttribute() != null &&
newDeclaration.getDurationAttribute() != null
+ &&
!nullSafeEqualityComparison(oldDeclaration.getDurationAttribute(),
newDeclaration.getDurationAttribute())) {
Review Comment:
This changes redeclaration compatibility so duration differences are only
considered incompatible when *both* sides set a duration. That makes
adding/removing a duration attribute between declarations always compatible,
which is a behavioral change from the prior check. If the intended contract is
'absence means inherit/unspecified', consider (mandatory) making that explicit
by normalizing/merging attributes before comparison; otherwise, consider
treating null vs non-null as incompatible to avoid silent semantic changes on
redeclare.
--
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]
