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.

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

KOGITO-6131 Enable codegen when RESTEasy reactive is used #1757

Merged

Conversation

tkobayas
Copy link
Contributor

@tkobayas tkobayas commented Nov 29, 2021

https://issues.redhat.com/browse/KOGITO-6131

This is a WIP branch to continue https://github.com/kiegroup/kogito-runtimes/pull/1669

Applied AdditionalStaticResourceBuildItem following quarkusio/quarkus#20846 (comment)

Use @QuarkusIntegrationTest instead of @QuarkusTest to handle static resources generated by codegen
https://github.com/kiegroup/kogito-examples/pull/1004

  • You have read the contributors guide
  • Your code is properly formatted according to this configuration
  • Pull Request title is properly formatted: KOGITO-XYZ Subject
  • Pull Request title contains the target branch if not targeting main: [0.9.x] KOGITO-XYZ Subject
  • Pull Request contains link to the JIRA issue
  • Pull Request contains link to any dependent or related Pull Request
  • Pull Request contains description of the issue
  • Pull Request does not include fixes for issues other than the main ticket
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:
  • Run all builds
    Please add comment: Jenkins retest this

  • Run (or rerun) specific test(s)
    Please add comment: Jenkins (re)run [kogito-runtimes|optaplanner|kogito-apps|kogito-examples|optaplanner-quickstarts|optaweb-employee-rostering|optaweb-vehicle-routing] tests

  • Quarkus LTS checks
    Please add comment: Jenkins run LTS

  • Run (or rerun) LTS specific test(s)
    Please add comment: Jenkins (re)run [kogito-runtimes|optaplanner|kogito-apps|kogito-examples|optaplanner-quickstarts|optaweb-employee-rostering|optaweb-vehicle-routing] LTS

  • Native checks
    Please add comment: Jenkins run native

  • Run (or rerun) native specific test(s)
    Please add comment: Jenkins (re)run [kogito-runtimes|optaplanner|kogito-apps|kogito-examples|optaplanner-quickstarts|optaweb-employee-rostering|optaweb-vehicle-routing] native

  • Full Kogito testing (with cloud images and operator BDD testing)
    Please add comment: Jenkins run BDD
    This check should be used only if a big change is done as it takes time to run, need resources and one full BDD tests check can be done at a time ...

@kie-ci
Copy link
Contributor

kie-ci commented Nov 29, 2021

The (build) kogito-apps check has failed. Please check the logs.

- fix test cases
- temporarily disabled MissingServletCapabilityException check
@tkobayas tkobayas force-pushed the KOGITO-6131-resteasy-reactive-tkobayas02 branch from b86c4ea to 844571f Compare November 29, 2021 08:07
Comment on lines 130 to 132
// if (capabilities.isPresent(Capability.RESTEASY) && capabilities.isMissing(Capability.SERVLET) && kogitoGenerateRestEnabled) {
// throw new MissingServletCapabilityException();
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporarily disabled MissingServletCapabilityException check in order to confirm if all tests run fine without servlet (undertow) dependency.

@tkobayas tkobayas requested a review from evacchi November 29, 2021 08:11
@kie-ci
Copy link
Contributor

kie-ci commented Nov 29, 2021

The (build) kogito-runtimes check has failed. Please check the logs.

@kie-ci
Copy link
Contributor

kie-ci commented Nov 29, 2021

The (build) kogito-examples check has failed. Please check the logs.

@kie-ci
Copy link
Contributor

kie-ci commented Nov 29, 2021

The (build) kogito-runtimes check has failed. Please check the logs.

@kie-ci
Copy link
Contributor

kie-ci commented Nov 29, 2021

The (build) kogito-examples check has failed. Please check the logs.

@kie-ci
Copy link
Contributor

kie-ci commented Nov 29, 2021

The (build) kogito-examples check has failed. Please check the logs.

@tkobayas
Copy link
Contributor Author

Note : In order to access static resources generated by codegen, we need to use @QuarkusIntegrationTest instead of @QuarkusTest. It seems to be the cause of failing kogito-examples tests. I will check.

import io.restassured.RestAssured;
import io.restassured.http.ContentType;

import static io.restassured.RestAssured.given;
import static org.hamcrest.Matchers.is;

@QuarkusTest
@QuarkusIntegrationTest
Copy link
Contributor

@evacchi evacchi Nov 30, 2021

Choose a reason for hiding this comment

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

honest question: why are we making this change as part of this PR?

Copy link
Contributor Author

@tkobayas tkobayas Nov 30, 2021

Choose a reason for hiding this comment

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

@evacchi Sorry about not having commented on that.

For example, if I leave BasicAddIT with @QuarkusTest (and others are @QuarkusIntegrationTest. At least, OASIT requires @QuarkusIntegrationTest), I hit the BindException because quarkus keeps running across the @QuarkusIntegrationTest tests.

[ERROR] Tests run: 4, Failures: 0, Errors: 1, Skipped: 3, Time elapsed: 6.403 s <<< FAILURE! - in org.kie.kogito.integrationtests.quarkus.BasicAddIT
[ERROR] org.kie.kogito.integrationtests.quarkus.BasicAddIT.testDs1  Time elapsed: 0.001 s  <<< ERROR!
java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
Caused by: java.lang.RuntimeException: Failed to start quarkus
Caused by: java.lang.RuntimeException: Unable to start HTTP server
Caused by: java.util.concurrent.ExecutionException: java.net.BindException: Address already in use
Caused by: java.net.BindException: Address already in use

So I think we should not mix @QuarkusTest and @QuarkusIntegrationTest in the same integration-test phase.

Copy link

Choose a reason for hiding this comment

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

Yes, you should not mix them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the confirmation!

Copy link
Contributor

Choose a reason for hiding this comment

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

ok !

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing me out to this thread @tkobayas , I would like to explore if it's feasible to produce a more meaningful help/warn message, reported in quarkusio/quarkus#28526

Copy link
Contributor

@evacchi evacchi left a comment

Choose a reason for hiding this comment

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

looks overall good. I would investigate if it's possible to do a "cleaner" check for static resource paths (i.e. use the type/add a new category). if it's too much work we can at least file a new JIRA for that and do it on the next round.

as for AdditionalStaticResourceBuildItem I am not sure whether it is really picked up by resteasy-classic, so I am summoning @geoand's powers for confirmation. If that is the case, that would mean that we don't need to add undertow as a forced dependency as @tkobayas was suggesting

However, please keep in mind that it looks like some metrics-related examples are breaking because a static file is missing. So either we are forgetting to register some static resources or undertow is indeed necessary.

BuildProducer<NativeImageResourceBuildItem> resource,
BuildProducer<GeneratedResourceBuildItem> genResBI) {
for (GeneratedFile f : generatedFiles) {
if (f.category() == GeneratedFileType.Category.RESOURCE) {
if (f.relativePath().startsWith(STATIC_RESOURCE_DIR)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if we only checked the Category. In theory there is even a specific subtype:

https://github.com/kiegroup/kogito-runtimes/blob/49c622f08f3803d27d0585adfdeabe93200f642e/kogito-codegen-modules/kogito-codegen-api/src/main/java/org/kie/kogito/codegen/api/GeneratedFileType.java#L76

but I don't know if we were consistent with it. I wouldn't check the path, though, it may be brittle

@geoand
Copy link

geoand commented Nov 30, 2021

as for AdditionalStaticResourceBuildItem I am not sure whether it is really picked up by resteasy-classic, so I am summoning @geoand's powers for confirmation. If that is the case, that would mean that we don't need to add undertow as a forced dependency as @tkobayas was suggesting

UsingAdditionalStaticResourceBuildItem should work with resteasy-classic as well

@tkobayas
Copy link
Contributor Author

Summary of this PR:

  • Users can use RESTEasy reactive (See integration-tests-quarkus-resteasy-reactive) with codegen capability
  • No need of quarkus-undertow for both RESTEasy classic and RESTEasy reactive even if codegen generates static http resources (thanks to AdditionalStaticResourceBuildItem). So quarkus-undertow is no longer in the extension's dependecy. If your app need it, add quarkus-undertow dependency to the app explicitly.
  • Now GeneratedFileType.Category.RESOURCE is split into INTERNAL_RESOURCE and STATIC_HTTP_RESOURCE. Kogito codegen developers need to choose from them. If the resource is retrieved by internal code, use INTERNAL_RESOURCE. If the resource is pulished as a static http resource, use STATIC_HTTP_RESOURCE. STATIC_HTTP_RESOURCE automatically adds META-INF/resources/ to its path so codegen developer should specify its path without META-INF/resources/ (e.g. monitoring/dashboards/).
  • If tests require to access such static http resources, the tests need to be annotated with @QuarkusIntegrationTest in integration-test phase rather than @QuarkusTest
  • You cannot mix @QuarkusIntegrationTest and @QuarkusTest in the same integration-test phase.

@danielezonca
Copy link
Contributor

  • If tests require to access such static http resources, the tests need to be annotated with @QuarkusIntegrationTest in integration-test phase rather than @QuarkusTest

@tkobayas
Can you please better clarify this? As far as I remember we mainly use @QuarkusTest so we might add some doc to explain when to use what 🤔

@tkobayas
Copy link
Contributor Author

tkobayas commented Dec 21, 2021

hi @danielezonca ,

AdditionalStaticResourceBuildItem makes generated static resources available to outside (even without quarkus-undertow). However, it requires @QuarkusIntegrationTest (quarkusio/quarkus#20846 (comment)) because of its build cycle. Sorry, I'm not knowedgable how @QuarkusTest intervenes in the build cycle but at the point of @QuarkusTest, those static resources are not yet available. In other words, @QuarkusIntegrationTest seems to be the true "integration test".

I think this javadoc explains well.
https://javadoc.io/doc/io.quarkus/quarkus-junit5/2.5.1.Final/io/quarkus/test/junit/QuarkusIntegrationTest.html

Probably our tests in Kogito repo should align like this?:

  1. @QuarkusTest as unit tests for surefire-plugin
  2. @QuarkusIntegrationTest as integration tests (*IT.java) for failsafe-plugin

If there is a case where @QuarkusTest is required (e.g. requires injection) and also it needs to access static resources, we can work around by @QuarkusTest + quarkus-undertow dependency (test scope).

Do you think it's acceptable to kogito developers? And where is the good place to document this?

@evacchi
Copy link
Contributor

evacchi commented Dec 22, 2021

restarting the GHA build for examples. I think this can be merged if no further changes are required + the build is green

@evacchi
Copy link
Contributor

evacchi commented Dec 22, 2021

/cc @ricardozanini

BTW today GHA is on vacation 🕺

@ricardozanini
Copy link
Member

/cc @ricardozanini

BTW today GHA is on vacation 🕺

Not anymore. :)

@tkobayas
Copy link
Contributor Author

Thank you for re-triggering and I see a compilation error. I need to merge main again and fix.

- Added quarkus-undertow-deployment dependency to knative eventing addon because it's no longer dependency of kogito-quarkus-common-deployment
@tkobayas
Copy link
Contributor Author

Regarding the SonarCloud coverage report, the uncovered codes are not directly related to this PR changes. Maybe we can add more tests to increase the coverage though....

Comment on lines 55 to 58
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-undertow-deployment</artifactId>
</dependency>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ricardozanini With this PR, kogito-quarkus-common-deployment no longer have quarkus-undertow-deployment dependency so adding here.

Copy link
Member

Choose a reason for hiding this comment

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

@tkobayas the Knative add-on doesn't require explicitly undertow. I believe that it comes from the HTTP Messaging: https://github.com/quarkiverse/quarkus-reactive-messaging-http

Since you've made a lot of changes, can you please confirm if this add-on still requires undertow? Otherwise, we can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ricardozanini Thank you for the reply. quarkus-undertow dependency was in quarkus/addons/knative/eventing/runtime/pom.xml. I removed it so now knative addon works without quarkus-undertow. Tests passed.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I had to add because Quarkus Maven Plugin was complaining about it. Glad that worked now that you've removed the transients.

Comment on lines 140 to 146
assertReturnExpressionContains(clazz.getMethods().get(0), "customers", EventKind.CONSUMED);
assertReturnExpressionContains(clazz.getMethods().get(1), "process.messagestartevent.processedcustomers", EventKind.PRODUCED);

MethodDeclaration customersMethod = clazz.getMethods().stream().filter(m -> m.getNameAsString().endsWith("CONSUMED_customers")).findFirst()
.orElseGet(() -> fail("Cannot find CONSUMED_customers method"));
MethodDeclaration processedcustomersMethod = clazz.getMethods().stream().filter(m -> m.getNameAsString().endsWith("PRODUCED_processedcustomers")).findFirst()
.orElseGet(() -> fail("Cannot find PRODUCED_processedcustomers method"));
assertReturnExpressionContains(customersMethod, "customers", EventKind.CONSUMED);
assertReturnExpressionContains(processedcustomersMethod, "process.messagestartevent.processedcustomers", EventKind.PRODUCED);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kostola This test failed randomly (depending on the method order) so changed. If you want to implement it differently, please change it after this PR.

@tkobayas
Copy link
Contributor Author

GHA : Kogito Examples Build

2021-12-23T06:35:28.4106052Z [INFO] Kogito Example :: Onboarding Example :: HR with Drools FAILURE [ 47.107 s]
...
2021-12-23T06:35:28.4321764Z [ERROR] Failed to execute goal io.quarkus:quarkus-maven-plugin:2.6.0.Final:build (default) on project hr: Failed to build quarkus application: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
2021-12-23T06:35:28.4325176Z [ERROR]    [error]: Build step org.kie.kogito.quarkus.common.deployment.KogitoAssetsProcessor#generateModel threw an exception: org.kie.kogito.codegen.rules.RuleCodegenError: Errors were generated during the code-generation process:
2021-12-23T06:35:28.4327788Z [ERROR] java.lang.IllegalStateException: No data of this type found. Use containsData to check for this first.
2021-12-23T06:35:28.4328568Z [ERROR]
2021-12-23T06:35:28.4330305Z [ERROR]    at org.kie.kogito.codegen.rules.IncrementalRuleCodegen.internalGenerate(IncrementalRuleCodegen.java:163)
2021-12-23T06:35:28.4332778Z [ERROR]    at org.kie.kogito.codegen.core.AbstractGenerator.generate(AbstractGenerator.java:69)
2021-12-23T06:35:28.4335005Z [ERROR]    at org.kie.kogito.codegen.core.ApplicationGenerator.lambda$generateComponents$0(ApplicationGenerator.java:97)
2021-12-23T06:35:28.4336947Z [ERROR]    at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:271)
2021-12-23T06:35:28.4338616Z [ERROR]    at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
2021-12-23T06:35:28.4340259Z [ERROR]    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
2021-12-23T06:35:28.4342051Z [ERROR]    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
2021-12-23T06:35:28.4343763Z [ERROR]    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
2021-12-23T06:35:28.4345313Z [ERROR]    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
2021-12-23T06:35:28.4346993Z [ERROR]    at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
2021-12-23T06:35:28.4349240Z [ERROR]    at org.kie.kogito.codegen.core.ApplicationGenerator.generateComponents(ApplicationGenerator.java:99)
2021-12-23T06:35:28.4351761Z [ERROR]    at org.kie.kogito.codegen.core.ApplicationGenerator.generate(ApplicationGenerator.java:74)
2021-12-23T06:35:28.4354574Z [ERROR]    at org.kie.kogito.quarkus.common.deployment.KogitoAssetsProcessor.generateFiles(KogitoAssetsProcessor.java:159)
2021-12-23T06:35:28.4358012Z [ERROR]    at org.kie.kogito.quarkus.common.deployment.KogitoAssetsProcessor.generateModel(KogitoAssetsProcessor.java:106)
...
2021-12-23T06:35:28.4429735Z [ERROR] Caused by: java.lang.IllegalStateException: No data of this type found. Use containsData to check for this first.
2021-12-23T06:35:28.4431023Z [ERROR]    at com.github.javaparser.ast.Node.getData(Node.java:505)
2021-12-23T06:35:28.4432570Z [ERROR]    at com.github.javaparser.ast.CompilationUnit.getPrinter(CompilationUnit.java:154)
2021-12-23T06:35:28.4434175Z [ERROR]    at com.github.javaparser.ast.Node.lambda$getPrinter$1(Node.java:199)
2021-12-23T06:35:28.4435167Z [ERROR]    at java.base/java.util.Optional.map(Optional.java:265)
2021-12-23T06:35:28.4436340Z [ERROR]    at com.github.javaparser.ast.Node.getPrinter(Node.java:199)
2021-12-23T06:35:28.4437622Z [ERROR]    at com.github.javaparser.ast.Node.toString(Node.java:322)
2021-12-23T06:35:28.4438978Z [ERROR]    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)

RuleCodegenError is caused by drools (drools-model-compiler) executable-model code gen processing and the message No data of this type found. Use containsData to check for this first. comes from javaparser so not likely related to this PR. I saw this error in kogito-runtimes org.kie.kogito.codegen.tests.BusinessRuleUnitIT.testSettingOtherVariableFromAutoGeneratedRuleUnit and kogito-examples org.kie.kogito.examples:hr so far. This seems to be a rare random issue. Needs some investigation in the future.

@tkobayas
Copy link
Contributor Author

restarting the GHA build for examples. I think this can be merged if no further changes are required + the build is green

Hi @evacchi , I merged main branch again and made some fixes. Currently GHA "Kogito Examples Build" failed but it's not related to this PR (https://github.com/kiegroup/kogito-runtimes/pull/1757#issuecomment-1000135842) and SonarCloud failed but I think it is not caused by this PR (https://github.com/kiegroup/kogito-runtimes/pull/1757#issuecomment-1000073247).

If you think it's okay to merge (with squash), please do. Note that https://github.com/kiegroup/kogito-examples/pull/1004 has be merged as well (I re-run the tests in the PR but it's redundant).

This PR has some ramifications to other kogito developers as I wrote in https://github.com/kiegroup/kogito-runtimes/pull/1757#issuecomment-996390447 and https://github.com/kiegroup/kogito-runtimes/pull/1757#issuecomment-998560583 (and I'll be off next week) so... we may delay merging until new year :)

@kie-ci
Copy link
Contributor

kie-ci commented Dec 24, 2021

The (build) kogito-examples check has failed. Please check the logs.

@tkobayas
Copy link
Contributor Author

(build) kogito-examples : Probably a flaky test.

[2021-12-24T03:28:14.112Z] [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 434.907 s <<< FAILURE! - in org.kie.kogito.examples.OutboxIT
[2021-12-24T03:28:14.112Z] [ERROR] org.kie.kogito.examples.OutboxIT.testSendProcessEvents  Time elapsed: 304.92 s  <<< ERROR!
[2021-12-24T03:28:14.112Z] org.awaitility.core.ConditionTimeoutException:

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Thank you for this effort!

@sonarcloud
Copy link

sonarcloud bot commented Jan 5, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

78.5% 78.5% Coverage
0.0% 0.0% Duplication

@tkobayas
Copy link
Contributor Author

tkobayas commented Jan 5, 2022

Again, regarding the SonarCloud coverage report, the uncovered codes are not directly related to this PR changes. Maybe we can add more tests to increase the coverage though....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants