Copilot commented on code in PR #4166:
URL:
https://github.com/apache/incubator-kie-kogito-runtimes/pull/4166#discussion_r2686662083
##########
kogito-codegen-modules/kogito-codegen-manager/src/main/java/org/kie/kogito/codegen/manager/BuilderManager.java:
##########
@@ -76,14 +76,29 @@ public static void build(BuildInfo buildInfo) throws
MalformedURLException {
LOGGER.info("Project build done");
}
+ static void executeConfigurationLog(BuildInfo buildInfo) {
+ LOGGER.info("========================================");
+ LOGGER.info(" Kogito Code Generation Configuration");
+ LOGGER.info("========================================");
+ LOGGER.info(" Java Version : {}", buildInfo.javaVersion());
+ LOGGER.info(" Source Encoding : {}",
buildInfo.javaSourceEncoding());
+ LOGGER.info(" Base Directory : {}",
buildInfo.projectBaseAbsolutePath());
+ LOGGER.info(" Output Directory : {}", buildInfo.outputDirectory());
+ LOGGER.info(" JSON Schema Version : {}",
buildInfo.jsonSchemaVersion() != null ? buildInfo.jsonSchemaVersion() : "nd");
+ LOGGER.info(" Persistence Enabled : {}",
buildInfo.enablePersistence());
+ LOGGER.info(" Keep Sources : {}", buildInfo.keepSources());
+ LOGGER.info(" Framework : {}", buildInfo.framework());
Review Comment:
Inconsistent spacing in the log output. The "Framework" label has extra
spaces compared to other labels. All other labels are padded to 19 characters,
but "Framework" has 20 characters with the trailing spaces. Consider aligning
it consistently with the other labels by using the same padding pattern.
```suggestion
LOGGER.info(" Framework : {}", buildInfo.framework());
```
##########
kogito-codegen-modules/kogito-codegen-manager/src/main/java/org/kie/kogito/codegen/manager/GeneratedFileManager.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.kie.kogito.codegen.manager;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Collection;
+import java.util.Objects;
+import java.util.stream.Stream;
+
+import org.drools.codegen.common.GeneratedFile;
+import org.drools.codegen.common.GeneratedFileWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Utility class responsible for managing and writing generated files to
specific locations.
+ * This class provides methods to handle the dumping of generated files while
+ * leveraging an internally configured {@code GeneratedFileWriter} instance.
+ * The {@code GeneratedFileManager} is designed to be used as a static utility
and
+ * therefore cannot be instantiated.
+ */
+public class GeneratedFileManager {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(GeneratedFileManager.class);
+
+ private static final GeneratedFileWriter.Builder
GENERATED_FILE_WRITER_BUILDER =
+ GeneratedFileWriter.builder("kogito",
"kogito.codegen.resources.directory", "kogito.codegen.sources.directory");
+
+ private GeneratedFileManager() {
+ // Not instantiable - static methods only.
+ }
+
+ /**
+ * Dumps a collection of generated files to the specified base path using
a configured {@code GeneratedFileWriter}.
+ * If the provided collection of generated files is null or empty, no
action is taken.
+ *
+ * @param generatedFiles the collection of {@code GeneratedFile} objects
to be written; may be null or empty.
+ * @param basePath the base directory {@code Path} where the files will be
written; must not be null.
+ * @throws NullPointerException if {@code basePath} is null.
+ */
+ public static void dumpGeneratedFiles(Collection<GeneratedFile>
generatedFiles, Path basePath) {
+ Objects.requireNonNull(basePath, "basePath must not be null");
+
+ if (generatedFiles == null || generatedFiles.isEmpty()) {
+ LOGGER.debug("No generated files to write (0 items).");
+ return;
+ }
+
+ GeneratedFileWriter writer =
GENERATED_FILE_WRITER_BUILDER.build(basePath);
+ generatedFiles.forEach(generatedFile ->
writeGeneratedFile(generatedFile, writer));
+ }
+
+ /**
+ * Writes a single {@code GeneratedFile} to its target destination using
the specified {@code GeneratedFileWriter}.
+ * The method logs the file path if logging at the INFO level is enabled.
+ *
+ * @param generatedFile the {@code GeneratedFile} to be written; must not
be null
+ * @param writer the {@code GeneratedFileWriter} responsible for handling
the file write operation; must not be null
+ */
+ public static void writeGeneratedFile(GeneratedFile generatedFile,
GeneratedFileWriter writer) {
+ if (LOGGER.isInfoEnabled()) {
+ LOGGER.info("Writing file: {}", generatedFile.path());
+ }
+ writer.write(generatedFile);
+ }
+
+ /**
+ * Deletes all files with the specified extension in a given directory
+ *
+ * @param directory the directory to search for files
+ * @param extension the file extension to match (e.g., "drl", "dmn",
"java")
+ */
+ public static void deleteFilesByExtension(Path directory, String
extension) {
+ final String normalizedExtension = extension.startsWith(".") ?
extension : "." + extension;
+
+ try (final Stream<Path> files = Files.find(directory,
+ Integer.MAX_VALUE,
+ (p, f) ->
p.toString().toLowerCase().endsWith(normalizedExtension.toLowerCase()))) {
+ files.forEach(p -> {
+ try {
+ Files.delete(p);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ "Error during " + normalizedExtension + " files deletion",
e);
+ }
+ }
+
+}
Review Comment:
The new `GeneratedFileManager` class lacks test coverage. Given that similar
utility classes in this module (e.g., `CodeGenManagerUtil`) have comprehensive
test coverage, this new class should also have tests. Consider adding tests
for: (1) `dumpGeneratedFiles` with null/empty collections and valid input, (2)
`writeGeneratedFile` behavior, and (3) `deleteFilesByExtension` with various
extension formats and edge cases like non-existent directories or extensions
with/without dots.
##########
kogito-maven-plugin/src/test/java/org/kie/kogito/maven/plugin/GenerateModelMojoTest.java:
##########
@@ -44,14 +42,14 @@ class GenerateModelMojoTest {
@Test
@InjectMojo(goal = "generateModel", pom =
"src/test/resources/unit/generate-model/pom.xml")
void generateModelWithOnDemand(GenerateModelMojo mojo) {
- commonSetup(mojo, true);
+ commonSetup(mojo);
commonGenerateModel(mojo);
}
@Test
@InjectMojo(goal = "generateModel", pom =
"src/test/resources/unit/generate-model/pom.xml")
void generateModelWithoutOnDemand(GenerateModelMojo mojo) {
- commonSetup(mojo, false);
+ commonSetup(mojo);
commonGenerateModel(mojo);
}
Review Comment:
The test method names `generateModelWithOnDemand` and
`generateModelWithoutOnDemand` are misleading because the `onDemand` parameter
has been removed. Since both tests now execute identical logic (calling
`commonSetup` and `commonGenerateModel` without any differences), these names
no longer accurately describe what they test. Consider renaming them to reflect
their actual purpose or consolidating them into a single test if they're truly
identical.
##########
kogito-codegen-modules/kogito-codegen-manager/src/main/java/org/kie/kogito/codegen/manager/CompilerHelper.java:
##########
@@ -141,15 +100,11 @@ static void
compileAndWriteClasses(Collection<GeneratedFile> generatedClasses,
for (PortablePath path : trgMfs.getFilePaths()) {
byte[] data = trgMfs.getBytes(path);
- writeGeneratedFile(new
GeneratedFile(GeneratedFileType.COMPILED_CLASS, path.asString(), data),
fileWriter);
+ GeneratedFileManager.dumpGeneratedFiles(Set.of(new
GeneratedFile(GeneratedFileType.COMPILED_CLASS, path.asString(), data)),
baseDir);
}
Review Comment:
Creating a new Set and calling `dumpGeneratedFiles` for each compiled file
in a loop is inefficient. Consider collecting all GeneratedFile objects first
and then calling `dumpGeneratedFiles` once with the complete collection. This
would reduce overhead from repeatedly creating GeneratedFileWriter instances
(one per file) and improve performance when compiling multiple files.
##########
kogito-codegen-modules/kogito-codegen-manager/src/main/java/org/kie/kogito/codegen/manager/GeneratedFileManager.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.kie.kogito.codegen.manager;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Collection;
+import java.util.Objects;
+import java.util.stream.Stream;
+
+import org.drools.codegen.common.GeneratedFile;
+import org.drools.codegen.common.GeneratedFileWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Utility class responsible for managing and writing generated files to
specific locations.
+ * This class provides methods to handle the dumping of generated files while
+ * leveraging an internally configured {@code GeneratedFileWriter} instance.
+ * The {@code GeneratedFileManager} is designed to be used as a static utility
and
+ * therefore cannot be instantiated.
+ */
+public class GeneratedFileManager {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(GeneratedFileManager.class);
+
+ private static final GeneratedFileWriter.Builder
GENERATED_FILE_WRITER_BUILDER =
+ GeneratedFileWriter.builder("kogito",
"kogito.codegen.resources.directory", "kogito.codegen.sources.directory");
+
+ private GeneratedFileManager() {
+ // Not instantiable - static methods only.
+ }
+
+ /**
+ * Dumps a collection of generated files to the specified base path using
a configured {@code GeneratedFileWriter}.
+ * If the provided collection of generated files is null or empty, no
action is taken.
+ *
+ * @param generatedFiles the collection of {@code GeneratedFile} objects
to be written; may be null or empty.
+ * @param basePath the base directory {@code Path} where the files will be
written; must not be null.
+ * @throws NullPointerException if {@code basePath} is null.
+ */
+ public static void dumpGeneratedFiles(Collection<GeneratedFile>
generatedFiles, Path basePath) {
+ Objects.requireNonNull(basePath, "basePath must not be null");
+
+ if (generatedFiles == null || generatedFiles.isEmpty()) {
+ LOGGER.debug("No generated files to write (0 items).");
+ return;
+ }
+
+ GeneratedFileWriter writer =
GENERATED_FILE_WRITER_BUILDER.build(basePath);
+ generatedFiles.forEach(generatedFile ->
writeGeneratedFile(generatedFile, writer));
+ }
+
+ /**
+ * Writes a single {@code GeneratedFile} to its target destination using
the specified {@code GeneratedFileWriter}.
+ * The method logs the file path if logging at the INFO level is enabled.
+ *
+ * @param generatedFile the {@code GeneratedFile} to be written; must not
be null
+ * @param writer the {@code GeneratedFileWriter} responsible for handling
the file write operation; must not be null
+ */
+ public static void writeGeneratedFile(GeneratedFile generatedFile,
GeneratedFileWriter writer) {
+ if (LOGGER.isInfoEnabled()) {
+ LOGGER.info("Writing file: {}", generatedFile.path());
+ }
+ writer.write(generatedFile);
+ }
+
+ /**
+ * Deletes all files with the specified extension in a given directory
+ *
+ * @param directory the directory to search for files
+ * @param extension the file extension to match (e.g., "drl", "dmn",
"java")
+ */
+ public static void deleteFilesByExtension(Path directory, String
extension) {
+ final String normalizedExtension = extension.startsWith(".") ?
extension : "." + extension;
+
+ try (final Stream<Path> files = Files.find(directory,
+ Integer.MAX_VALUE,
+ (p, f) ->
p.toString().toLowerCase().endsWith(normalizedExtension.toLowerCase()))) {
+ files.forEach(p -> {
+ try {
+ Files.delete(p);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ } catch (IOException e) {
+ throw new UncheckedIOException(
+ "Error during " + normalizedExtension + " files deletion",
e);
+ }
+ }
Review Comment:
The `deleteFilesByExtension` method doesn't check if the directory exists
before attempting to search it. If the directory doesn't exist, `Files.find`
will throw a `NoSuchFileException` which gets wrapped in an
`UncheckedIOException`. Consider adding a check using `Files.exists(directory)`
and returning early if the directory doesn't exist, or document that the
directory must exist when calling this method.
--
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]
