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

[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C… #4352

Merged
merged 1 commit into from
May 10, 2022
Merged

[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C… #4352

merged 1 commit into from
May 10, 2022

Conversation

tkobayas
Copy link
Contributor

@tkobayas tkobayas commented May 2, 2022 •
edited
Loading

…lassCastException in executable model build

Ports
This PR is for 7.x.
for main -> https://github.com/kiegroup/drools/pull/4380

JIRA:
https://issues.redhat.com/browse/DROOLS-6936

How to replicate CI configuration locally?

Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.

build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.

How to retest this PR or trigger a specific build:

  • a pull request please add comment: Jenkins retest this

  • for a full downstream build

    • for jenkins job: please add comment: Jenkins run fdb
    • for github actions job: add the label run_fdb

  • a compile downstream build please add comment: Jenkins run cdb

  • a full production downstream build please add comment: Jenkins execute product fdb

  • an upstream build please add comment: Jenkins run upstream

@tkobayas tkobayas marked this pull request as draft May 2, 2022 13:10
@tkobayas
Copy link
Contributor Author

tkobayas commented May 2, 2022

probably will need to check conflict after merging https://github.com/kiegroup/drools/pull/4351

@tkobayas tkobayas marked this pull request as ready for review May 6, 2022 03:34
tkobayas
tkobayas commented May 6, 2022
View reviewed changes
...ler/src/main/java/org/drools/modelcompiler/builder/generator/drlxparse/ConstraintParser.java
Comment on lines -273 to +277
if (drlxExpr instanceof LiteralExpr ) {
if (drlxExpr instanceof LiteralExpr) {
Class<?> literalExpressionType = getLiteralExpressionType(((LiteralExpr) drlxExpr));
if (drlxExpr instanceof BigIntegerLiteralExpr) {
drlxExpr = ((BigIntegerLiteralExpr) drlxExpr).convertToObjectCreationExpr();
} else if (drlxExpr instanceof BigDecimalLiteralExpr) {
drlxExpr = ((BigDecimalLiteralExpr) drlxExpr).convertToObjectCreationExpr();
}
Copy link
Contributor Author

@tkobayas tkobayas May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For BinaryExpr, we have already implemented the conversion from BigDecimalLiteralExpr to ObjectCreationExpr. For simple literal binding, I added the conversion here.

@tkobayas
Copy link
Contributor Author

tkobayas commented May 9, 2022

Jenkins run cdb

1 similar comment
@tkobayas
Copy link
Contributor Author

tkobayas commented May 9, 2022

Jenkins run cdb

@tkobayas
Copy link
Contributor Author

tkobayas commented May 9, 2022

CDB : Not related to this PR.

2022-05-09T11:21:47.984Z] [ERROR] Error executing CLI script null
[2022-05-09T11:21:47.984Z] [ERROR] [disconnected /] embed-server --jboss-home=/home/jenkins/workspace/KIE/7.x/compile/drools-7.x.compile/bc/kiegroup_kie_wb_distributions/business-central-parent/business-central-webapp/target/bootable-jar-build-artifacts/wildfly --std-out=discard
[2022-05-09T11:21:47.984Z]
[2022-05-09T11:21:47.984Z] [INFO] FAILURE build of project org.kie:business-central-webapp

@mariofusco mariofusco merged commit 316f83d into apache:7.x May 10, 2022
pibizza
pibizza reviewed May 10, 2022
View reviewed changes
Copy link
Contributor

@pibizza pibizza left a comment

Choose a reason for hiding this comment

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

Two comments, but they can both be addressed later, since this is already merged.

...ols-model-compiler/src/test/java/org/drools/modelcompiler/bigdecimaltest/BigDecimalTest.java
ksession.fireAllRules();

Assertions.assertThat(result).containsExactly(new BigDecimal("0"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible use the static import for assertThat - it is the most common way in the code, so better to keep it standardized. We can fix this all in a single pass later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pibizza . Filed https://issues.redhat.com/browse/DROOLS-6951

...src/test/java/org/drools/modelcompiler/builder/generator/drlxparse/ConstraintParserTest.java

assertEquals("new java.math.BigDecimal(\"10.3\")", result.getExpr().toString());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are migrating away from junit assert to assertj assert, I suggest to start writing new tests using the assertj framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pibizza . Filed https://issues.redhat.com/browse/DROOLS-6951

tkobayas added a commit to tkobayas/drools that referenced this pull request May 12, 2022
@tkobayas
[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C…
6e00bad 
…lassCastException in executable model build (apache#4352)
@tkobayas tkobayas mentioned this pull request May 12, 2022
Merged
mariofusco pushed a commit that referenced this pull request May 12, 2022
@tkobayas
[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C…
9c8331c 
…lassCastException in executable model build (#4352) (#4380)
tkobayas added a commit to tkobayas/drools that referenced this pull request Jul 21, 2022
@tkobayas
[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C…
5bd5b75 
…lassCastException in executable model build (apache#4352)
@tkobayas tkobayas mentioned this pull request Jul 21, 2022
Closed
tkobayas added a commit to tkobayas/drools that referenced this pull request Jul 21, 2022
@tkobayas
[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C…
7c650e5 
…lassCastException in executable model build (apache#4352)
@tkobayas tkobayas mentioned this pull request Jul 21, 2022
Merged
mariofusco pushed a commit that referenced this pull request Jul 25, 2022
@tkobayas @mariofusco
[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C…
4d561c0 
…lassCastException in executable model build (#4352)
tkobayas added a commit to tkobayas/drools that referenced this pull request Jul 25, 2022
@tkobayas
[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C…
2cf7c32 
…lassCastException in executable model build (apache#4352)
mariofusco pushed a commit that referenced this pull request Jul 25, 2022
@tkobayas @mariofusco
[DROOLS-6936] BigDecimalLiteral with binding in mvel dialect causes C…
366764b 
…lassCastException in executable model build (#4352)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pibizza pibizza pibizza left review comments

@hellowdan hellowdan hellowdan approved these changes

@mariofusco mariofusco mariofusco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tkobayas @mariofusco @pibizza @hellowdan