Copilot commented on code in PR #6788:
URL:
https://github.com/apache/incubator-kie-drools/pull/6788#discussion_r3511046225
##########
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/visitor/pattern/ClassPatternDSL.java:
##########
@@ -166,12 +169,36 @@ private Optional<Expression>
buildFromDeclaration(PatternDescr pattern) {
}
private Optional<String> findFirstInnerBinding(List<? extends BaseDescr>
constraintDescrs, Class<?> patternType) {
- return constraintDescrs.stream()
- .map( constraint ->
ConstraintExpression.createConstraintExpression( context, patternType,
constraint, isPositional(constraint) ).getExpression() )
- .map( DrlxParseUtil::parseExpression )
- .filter( drlx -> drlx.getBind() != null )
- .map( drlx -> drlx.getBind().asString() )
- .findFirst();
+ for ( BaseDescr constraint : constraintDescrs ) {
+ ConstraintExpression constraintExpression =
+ ConstraintExpression.createConstraintExpression( context,
patternType, constraint, isPositional( constraint ) );
+ if ( constraintExpression == null ) {
+ continue;
+ }
+ String expression = constraintExpression.getExpression();
+ if ( expression == null || expression.trim().isEmpty() ) {
+ continue;
+ }
+ DrlxExpression drlx;
+ try {
+ drlx = DrlxParseUtil.parseExpression( expression );
+ } catch ( RuntimeException e ) {
+ // This method only parses constraints to derive a name for an
*unnamed* pattern. A
+ // constraint that doesn't parse here cannot declare an inner
binding ($x : field) anyway,
+ // so deriving a name from it is impossible regardless; don't
let it abort the whole
+ // KieBase build. Log which rule and pattern carry the
offending constraint (to aid
+ // diagnosis), then skip. The actual constraint compilation is
handled separately in
+ // findAllConstraint and will report a proper compilation
error there if warranted.
+ LOG.warn( "Skipping a constraint that could not be parsed
while generating an identifier for an " +
+ "unnamed pattern of type {} in rule '{}'. Offending
constraint expression: [{}] ({}: {})",
+ patternType, context.getRuleName(), expression,
e.getClass().getSimpleName(), e.getMessage() );
+ continue;
+ }
Review Comment:
The catch is currently `catch (RuntimeException e)`, but
`DrlxParseUtil.parseExpression` ultimately calls `DrlxParser.simplifiedParse`,
which can throw other RuntimeExceptions (e.g. `IllegalStateException` when a
parse is marked successful but has no result). Swallowing those would hide
internal bugs and silently change generated identifiers; only the expected
parse failure (`ParseProblemException`) should be skipped/logged here.
##########
drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/DeclaredTypesTest.java:
##########
@@ -599,6 +607,42 @@ public void testExtendPojo(RUN_TYPE runType) throws
Exception {
assertThat(ksession.fireAllRules()).isEqualTo(1);
}
+ @ParameterizedTest
+ @MethodSource("parametersPatternOnly")
+ public void
testUnparseableConstraintInUnnamedPatternIsLoggedWithContext(RUN_TYPE runType) {
+ // When the pattern auto-naming pre-pass
(ClassPatternDSL.findFirstInnerBinding) hits a
+ // constraint it cannot parse, it must not fail with a bare
"unexpected token:" that hides which
+ // rule is at fault. It now logs a WARN naming the rule and the
offending expression, then skips
+ // it; the real constraint compilation still rejects it (that abort is
unchanged and correct).
+ // 'class == X.class' is rejected by the executable-model DRLX
grammar, so it triggers the path.
+ // Executable-model only: the classic compiler accepts 'class ==' and
never reaches this pre-pass.
+ Logger logger = (Logger)
LoggerFactory.getLogger(ClassPatternDSL.class);
+ ListAppender<ILoggingEvent> appender = new ListAppender<>();
+ appender.start();
+ logger.addAppender(appender);
+ try {
+ String str =
+ "package org.test;\n" +
+ "import " + Person.class.getCanonicalName() + ";\n" +
+ "import " + Child.class.getCanonicalName() + ";\n" +
+ "rule R when\n" +
+ " Person( class == Child.class )\n" +
+ "then end";
+ try {
+ createKieBuilder(runType, str);
+ } catch (RuntimeException expected) {
+ // The real constraint compilation still rejects 'class ==
...'; that abort is unchanged.
+ }
+ assertThat(appender.list)
+ .as("a WARN naming the rule and the offending constraint
expression must be logged")
+ .anyMatch(e -> e.getLevel() == Level.WARN
+ && e.getFormattedMessage().contains("rule 'R'")
+ &&
e.getFormattedMessage().contains("Child.class"));
+ } finally {
+ logger.detachAppender(appender);
+ }
Review Comment:
The test starts the `ListAppender` but never stops it. Even though it’s
detached from the logger, stopping it in the `finally` block avoids lifecycle
leaks and keeps the pattern consistent with other Logback-based tests.
--
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]
