Copilot commented on code in PR #4229:
URL:
https://github.com/apache/incubator-kie-kogito-runtimes/pull/4229#discussion_r3116831281
##########
drools/kogito-dmn/src/main/java/org/kie/kogito/dmn/DMNKogito.java:
##########
@@ -58,17 +67,36 @@ private DMNKogito() {
public static DMNRuntime createGenericDMNRuntime(Set<DMNProfile>
customDMNProfiles, boolean enableRuntimeTypeCheckOption, Reader... readers) {
DMNKogitoCallbacks.beforeCreateGenericDMNRuntime(readers);
List<Resource> resources =
Stream.of(readers).map(ReaderResource::new).collect(Collectors.toList());
- EvalHelper.clearGenericAccessorCache(); // KOGITO-3325 DMN hot reload
manage accessor cache when stronglytyped
- DMNRuntimeBuilder dmnRuntimeBuilder = DMNRuntimeBuilder.fromDefaults();
- customDMNProfiles.forEach(dmnRuntimeBuilder::addProfile);
- DMNRuntime dmnRuntime = dmnRuntimeBuilder
- .buildConfiguration()
- .fromResources(resources)
- .getOrElseThrow(e -> new RuntimeException("Error initializing
DMNRuntime", e));
- RuntimeTypeCheckOption runtimeTypeCheckOption = new
RuntimeTypeCheckOption(enableRuntimeTypeCheckOption);
- ((DMNRuntimeImpl) dmnRuntime).setOption(runtimeTypeCheckOption);
- DMNKogitoCallbacks.afterCreateGenericDMNRuntime(dmnRuntime);
- return dmnRuntime;
+ return createGenericDMNRuntime(customDMNProfiles,
enableRuntimeTypeCheckOption, resources);
+ }
+
+ /**
+ * Internal Utility class.<br/>
+ * Use {@link Application#decisionModels()} of Kogito API to
programmatically access DMN assets and evaluate DMN
+ * decisions.
+ *
+ * @param customDMNProfiles
+ * @param enableRuntimeTypeCheckOption
+ * @param modelPaths A Map of model path to model encoding
+ * @return
+ */
+ public static DMNRuntime createGenericDMNRuntime(Set<DMNProfile>
customDMNProfiles, boolean enableRuntimeTypeCheckOption,
+ Map<String, String> modelPaths) {
+ DMNKogitoCallbacks.beforeCreateGenericDMNRuntime(modelPaths);
+ List<Resource> resources = modelPaths.entrySet()
+ .stream()
+ .map(modelPathEntry -> {
+ Optional<InputStream> modelStream =
getDMNModelStream(modelPathEntry.getKey());
+ if (modelStream.isPresent()) {
+ return readResource(modelStream.get(),
modelPathEntry.getValue());
+ } else {
+ logger.warn("DMN model stream not found for path: {}",
modelPathEntry.getKey());
+ }
+ return null;
+ })
+ .filter(Objects::nonNull)
+ .map(ReaderResource::new).collect(Collectors.toList());
Review Comment:
`createGenericDMNRuntime(..., Map<String,String> modelPaths)` logs a warning
and silently skips entries whose streams can’t be found, potentially
initializing a runtime with missing models and failing later in less obvious
ways. It would be safer to fail fast (throw) when any expected model can’t be
loaded, or at least collect all missing paths and throw a single exception
summarizing them.
##########
kogito-codegen-modules/kogito-codegen-decisions/src/main/java/org/kie/kogito/codegen/decision/DecisionCodegenUtils.java:
##########
@@ -94,6 +98,16 @@ public class DecisionCodegenUtils {
private DecisionCodegenUtils() {
}
+ /**
+ *
+ * @param generatedFiles ACt as accumulator for all generated files
+ * @param classesForManualReflection
+ * @param cResources
+ * @param customDMNProfiles
+ * @param runtimeTypeCheckOption
+ * @param generator
+ * @return
Review Comment:
The added Javadoc block has typos and placeholder-style tags (e.g., "ACt as
accumulator" and empty `@return`). Either remove it or complete it with
accurate parameter/return descriptions to avoid misleading documentation.
```suggestion
* Loads and validates DMN models from the collected resources, then
generates the
* corresponding source files and metadata artifacts.
*
* @param generatedFiles accumulator for all generated files produced
during code generation
* @param classesForManualReflection class names that must be registered
for manual reflection
* @param cResources collected DMN resources to compile and process
* @param customDMNProfiles custom DMN profiles to use during model
loading and validation
* @param runtimeTypeCheckOption runtime type check configuration to
apply while compiling models
* @param generator decision code generator providing build context and
application metadata
* @return the generated resources entry produced while loading and
validating the DMN models
```
##########
kogito-codegen-modules/kogito-codegen-processes-integration-tests/src/test/java/org/kie/kogito/codegen/AbstractCodegenIT.java:
##########
@@ -194,22 +194,34 @@ protected Application generateCode(Map<TYPE,
List<String>> resourcesTypeMap, Kog
srcMfs.write("org/drools/project/model/ProjectRuntime.java",
DUMMY_PROCESS_RUNTIME.getBytes());
}
- if (LOGGER.isInfoEnabled()) {
- Path temp = Files.createTempDirectory("KOGITO_TESTS");
- LOGGER.info("Dumping generated files in " + temp);
- for (GeneratedFile entry : generatedFiles) {
+ Path temp = Files.createTempDirectory("KOGITO_TESTS");
+ LOGGER.info("Dumping generated files in " + temp);
+ URL modelPathsUrl = null;
+ String modelPathsFilePath = null;
+ for (GeneratedFile entry : generatedFiles) {
+ String fileName = entry.relativePath();
+ if (fileName.contains("modelPaths.txt")) {
+ Path fpath = temp.resolve(entry.relativePath());
+ fpath.getParent().toFile().mkdirs();
+ Files.write(fpath, entry.contents());
+ modelPathsUrl = fpath.toUri().toURL();
+ modelPathsFilePath = entry.relativePath();
+ } else if (LOGGER.isInfoEnabled()) {
Review Comment:
`generateCode(...)` now always creates a temp directory and logs "Dumping
generated files" at INFO, even when INFO logging is disabled and even when no
files are actually dumped. This can slow down test runs and leave many temp
directories behind. Restore the previous `LOGGER.isInfoEnabled()` guard (or
delete the temp dir when not needed) while still handling `modelPaths.txt` when
present.
```suggestion
boolean dumpGeneratedFiles = LOGGER.isInfoEnabled();
Path temp = null;
URL modelPathsUrl = null;
String modelPathsFilePath = null;
for (GeneratedFile entry : generatedFiles) {
String fileName = entry.relativePath();
if (fileName.contains("modelPaths.txt")) {
if (temp == null) {
temp = Files.createTempDirectory("KOGITO_TESTS");
}
Path fpath = temp.resolve(entry.relativePath());
fpath.getParent().toFile().mkdirs();
Files.write(fpath, entry.contents());
modelPathsUrl = fpath.toUri().toURL();
modelPathsFilePath = entry.relativePath();
} else if (dumpGeneratedFiles) {
if (temp == null) {
temp = Files.createTempDirectory("KOGITO_TESTS");
LOGGER.info("Dumping generated files in " + temp);
}
```
##########
kogito-codegen-modules/kogito-codegen-decisions/src/main/java/org/kie/kogito/codegen/decision/DecisionContainerGenerator.java:
##########
@@ -103,37 +99,10 @@ public CompilationUnit compilationUnit() {
setupDecisionModelTransformerVariable(initMethod,
context.getAddonsConfig().useMonitoring());
setupCustomDMNProfiles(initMethod, customDMNProfiles);
setupEnableRuntimeTypeCheckOption(initMethod,
enableRuntimeTypeCheckOption);
-
- for (CollectedResource resource : resources) {
- Optional<String> encoding = determineEncoding(resource);
- MethodCallExpr getResAsStream =
getReadResourceMethod(applicationClass, resource);
- MethodCallExpr isr = new
MethodCallExpr("readResource").addArgument(getResAsStream);
- encoding.map(StringLiteralExpr::new).ifPresent(isr::addArgument);
- initMethod.addArgument(isr);
- }
-
+ setupModelPathsFile(initMethod, applicationCanonicalName);
return compilationUnit;
Review Comment:
`DecisionContainerGenerator` no longer uses the `resources` collection or
the `LOG` logger after switching to `setupModelPathsFile(...)`. Leaving these
unused fields/imports makes the generator harder to maintain and may produce
compiler warnings. Consider removing unused members or reintroducing usage
where needed.
##########
quarkus/extensions/kogito-quarkus-decisions-extension/kogito-quarkus-decisions-integration-test/src/test/resources/application.properties:
##########
@@ -17,4 +17,5 @@
# under the License.
#
-quarkus.http.test-port=0
\ No newline at end of file
+quarkus.http.test-port=0
+quarkus.log.category."org.kie.kogito".level=DEBUG
Review Comment:
Setting `org.kie.kogito` logging to DEBUG for this integration test module
can significantly increase CI logs and make failures harder to scan. If this
was only for troubleshooting, please remove it or scope it to a specific test
via test logging configuration rather than committing DEBUG by default.
```suggestion
quarkus.http.test-port=0
```
##########
kogito-codegen-modules/kogito-codegen-decisions/src/test/java/org/kie/kogito/codegen/decision/DecisionCodegenUtilsTest.java:
##########
@@ -45,7 +53,26 @@ void loadModelsAndValidate(KogitoBuildContext.Builder
contextBuilder) {
Paths.get("src/test/resources/decision/models/vacationDays").toAbsolutePath());
Map<Resource, CollectedResource> r2cr =
cResources.stream().collect(Collectors.toMap(CollectedResource::resource,
Function.identity()));
Map.Entry<String, GeneratedResources> retrieved =
DecisionCodegenUtils.loadModelsAndValidate(context, r2cr,
Collections.emptySet(), new RuntimeTypeCheckOption(false));
- System.out.println(retrieved.getKey());
+ assertThat(retrieved).isNotNull();
+ }
+
+ @Test
+ void generateDecisionsFiles() {
+ final Collection<CollectedResource> cResources =
CollectedResourceProducer.fromPaths(
+
Paths.get("src/test/resources/decision/models/vacationDays").toAbsolutePath(),
+
Paths.get("src/test/resources/decision/alltypes").toAbsolutePath());
+ Collection<GeneratedFile> generatedFiles = new ArrayList<>();
+ DecisionCodegenUtils.generateModelPathsFile(generatedFiles,
cResources);
+ assertThat(generatedFiles).hasSize(1);
Review Comment:
Test name `generateDecisionsFiles()` is misleading: it only asserts the
contents of the generated `modelPaths.txt`. Renaming the test to reflect what
it verifies will make failures easier to interpret (e.g.,
`generateModelPathsFile_shouldContainAllModels`).
##########
drools/kogito-dmn/src/main/java/org/kie/kogito/dmn/AbstractDecisionModels.java:
##########
@@ -97,6 +110,21 @@ protected static java.io.InputStreamReader
readResource(java.io.InputStream stre
}
}
+ static Map<String, String> getModelPaths(URL modelPathsUrl) {
+ LOG.trace("getModelPaths {}", modelPathsUrl);
+ if (modelPathsUrl == null) {
+ LOG.error("Failed to find {} with current thread classloader",
DMN_MODEL_PATHS_FILE);
+ throw new IllegalStateException("Failed to find " +
DMN_MODEL_PATHS_FILE);
+ }
+ try (BufferedReader reader = new BufferedReader(new
InputStreamReader(modelPathsUrl.openStream()))) {
+ return reader.lines().collect(Collectors.toMap(
+ line -> line.substring(0, line.indexOf(':')),
+ line -> line.substring(line.indexOf(':') + 1)));
+ } catch (IOException e) {
Review Comment:
`getModelPaths(...)` parses each line using `substring(0,
line.indexOf(':'))` / `substring(line.indexOf(':') + 1)` without validating
that `':'` exists. A malformed/blank line will cause
`StringIndexOutOfBoundsException`, and `Collectors.toMap` will also throw on
duplicate paths. Consider splitting with a limit of 2, skipping empty/comment
lines, and providing an explicit merge function or throwing a descriptive
exception when duplicates are encountered.
##########
kogito-codegen-modules/kogito-codegen-decisions/src/main/java/org/kie/kogito/codegen/decision/DecisionCodegenUtils.java:
##########
@@ -334,6 +360,25 @@ static void
generateAndStoreDecisionModelResourcesProvider(Collection<GeneratedF
storeFile(generatedFiles, GeneratedFileType.SOURCE,
generator.generatedFilePath(), generator.generate());
}
+ private static Optional<String> determineEncoding(CollectedResource
resource) {
+ try {
+ BufferedReader br = new
BufferedReader(resource.resource().getReader());
+ StringBuilder sb = new StringBuilder(br.readLine());
+ sb.append(br.readLine());
+ String head = sb.toString();
+ boolean prologUTF8 = head.startsWith("<?xml version=\"1.0\"
encoding=\"UTF-8\"") || head.startsWith("<?xml version=\"1.0\"
encoding=\"utf-8\"");
+ boolean kogitoDMNEditor =
head.contains("xmlns:kie=\"http://www.drools.org/kie/dmn";);
+ LOGGER.debug("resource {} determineEncoding results; prologUTF8
{}, kogitoDMNEditor {}.", resource.resource(), prologUTF8, kogitoDMNEditor);
+ if (prologUTF8 || kogitoDMNEditor) {
+ return Optional.of("UTF-8");
+ } else {
+ return Optional.empty();
+ }
+ } catch (Exception e) {
+ return Optional.empty();
+ }
Review Comment:
`determineEncoding(...)` doesn’t close the `Reader` obtained from
`resource.resource().getReader()`, which can leak file handles during codegen
(especially with many models). Also, `br.readLine()` can return `null`, so `new
StringBuilder(br.readLine())` can throw NPE for short/empty files. Use
try-with-resources and handle missing lines defensively.
##########
drools/kogito-dmn/src/main/java/org/kie/kogito/dmn/AbstractDecisionModels.java:
##########
@@ -97,6 +110,21 @@ protected static java.io.InputStreamReader
readResource(java.io.InputStream stre
}
}
+ static Map<String, String> getModelPaths(URL modelPathsUrl) {
+ LOG.trace("getModelPaths {}", modelPathsUrl);
+ if (modelPathsUrl == null) {
+ LOG.error("Failed to find {} with current thread classloader",
DMN_MODEL_PATHS_FILE);
+ throw new IllegalStateException("Failed to find " +
DMN_MODEL_PATHS_FILE);
Review Comment:
The log message "Failed to find ... with current thread classloader" is
misleading here: `getModelPaths(...)` receives a `URL` from the caller rather
than performing any classloader lookup. Update the message to reflect the
actual failure (e.g., caller provided null / resource not found) so
troubleshooting points to the right place.
```suggestion
LOG.error("No URL provided for {}", DMN_MODEL_PATHS_FILE);
throw new IllegalStateException("No URL provided for " +
DMN_MODEL_PATHS_FILE);
```
--
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]
