Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[incubator-kie-drools-5938] [new-parser] Tests using wrong duration a… #5969

Merged
merged 2 commits into from
May 27, 2024
Merged

[incubator-kie-drools-5938] [new-parser] Tests using wrong duration a… #5969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed97900
[incubator-kie-drools-5938] [new-parser] Tests using wrong duration a…
tkobayas May 23, 2024
4a63810
- Use explicit test value
tkobayas May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ...drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/MiscDRLParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5216,4 +5216,20 @@ void doubleAmpersandInfixAndInAccumulate() {
assertThat(accumulateFunction.getFunction()).isEqualTo("average");
assertThat(accumulateFunction.getParams()).containsExactly("$a + $b");
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tkobayas
IIUC a method has been removed related to this test.
Is there also a "fails" test or similar, i.e. something that demonstrates a managed excpetion, or error, when the duration input si "wrong" ?

void durationChunk() {
final String text =
"rule R\n" +
" duration (wrong input) \n" +
"when\n" +
"then\n" +
"end";
RuleDescr rule = parseAndGetFirstRuleDescr(text);
assertThat(rule.getAttributes()).containsKey("duration");
assertThat(rule.getAttributes().get("duration").getType()).isEqualTo(AttributeDescr.Type.EXPRESSION);

// At the moment, the parser accepts any input and let the compile phase validate it.
assertThat(rule.getAttributes().get("duration").getValue()).isEqualTo("wrong input");
}
}
2 changes: 1 addition & 1 deletion drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ attribute : name=( 'salience' | 'enabled' ) conditionalAttributeValue #expressio
| name=( 'agenda-group' | 'activation-group' | 'ruleflow-group' | 'date-effective' | 'date-expires' | 'dialect' ) DRL_STRING_LITERAL #stringAttribute
| name='calendars' DRL_STRING_LITERAL ( COMMA DRL_STRING_LITERAL )* #stringListAttribute
| name='timer' ( DECIMAL_LITERAL | LPAREN chunk RPAREN ) #intOrChunkAttribute
| name='duration' ( DECIMAL_LITERAL | TIME_INTERVAL | LPAREN TIME_INTERVAL RPAREN ) #durationAttribute
| name='duration' ( DECIMAL_LITERAL | LPAREN chunk RPAREN ) #intOrChunkAttribute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tkobayas IIUC a method has been removed related to this test. Is there also a "fails" test or similar, i.e. something that demonstrates a managed excpetion, or error, when the duration input si "wrong" ?

@gitgabrio

  1. Using the chunk rule makes the parser extremely benevolent wrt. the duration attribute's value. The input basically cannot be wrong because the chunk rule accepts anything between the two parentheses. So no need for a negative test, IMO.
  2. Wrt. the removed method - the method name corresponds to the rule alternative's label (string after #). In this case, the label has changed from durationAttribute to intOrChunkAttribute making the visitDurationAttribute() method unused. The visitIntOrChunkAttribute() method already exists (it handles for example the timer subrule one line above) so no need to add a new method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yurloc , but now I'm slightly confused. IIUC, the parser does not do any kind of data validation, i.e. anything can be parsed as "DURATION", delegating the verification to the client code; and I have the impression it should not be that way (mind you, I may be wrong, of course, especially in that context)
@mariofusco wdyt ?

Copy link
Contributor Author

@tkobayas tkobayas May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gitgabrio

anything can be parsed as "DURATION", delegating the verification to the client code;

Indeed, the old Antlr3 parser delegates the verification to the compile phase, so the rule build reports an error anyway.

At the moment (= until this new parser passes all existing unit tests), I'd like to follow the same approach as the old Antlr3 parser as possible, so that we can minimize the behaviour difference now.

Then, we will consider any possible improvements. I'll note them in #5972

Does it sound good?

Hi @yurloc . Thank you for the follow-up for the Gabriele's question.

I would maybe choose something that is more obviously wrong, for example, wrong input.

Thanks, I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @tkobayas , thanks for explanation, fine for me!

;

conditionalAttributeValue : ( LPAREN conditionalExpression RPAREN | conditionalExpression ) ;
Expand Down
15 changes: 0 additions & 15 deletions drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,21 +514,6 @@ public AttributeDescr visitIntOrChunkAttribute(DRLParser.IntOrChunkAttributeCont
return attributeDescr;
}

@Override
public AttributeDescr visitDurationAttribute(DRLParser.DurationAttributeContext ctx) {
AttributeDescr attributeDescr = BaseDescrFactory.builder(new AttributeDescr(ctx.name.getText()))
.withParserRuleContext(ctx)
.build();
if (ctx.DECIMAL_LITERAL() != null) {
attributeDescr.setValue(ctx.DECIMAL_LITERAL().getText());
attributeDescr.setType(AttributeDescr.Type.NUMBER);
} else {
attributeDescr.setValue(unescapeJava(safeStripStringDelimiters(ctx.TIME_INTERVAL().getText())));
attributeDescr.setType(AttributeDescr.Type.EXPRESSION);
}
return attributeDescr;
}

/**
* entry point for LHS
*/
Expand Down
Loading