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

Improvement of CLI logging and error messages related to reflection. #1602

Closed
wants to merge 10 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,18 @@ public static void main(String... args) {
LOG.debug("Current working directory: {}", System.getProperty("user.dir"));
CommandLine commandLine = new CommandLine(new CobiGenCommand());
commandLine.registerConverter(Path.class, s -> Paths.get(s));
CommandLine.IExecutionExceptionHandler exceptionHandler = new CommandLine.IExecutionExceptionHandler() {
@Override
public int handleExecutionException(Exception e, CommandLine commandLine, CommandLine.ParseResult parseResult) throws Exception {
LOG.error("An error occurred while executing CobiGen: {}", e.getMessage());
if (LOG.isDebugEnabled()) e.printStackTrace();
return 1;
}
};
commandLine.setExecutionExceptionHandler(exceptionHandler);
int exitCode = commandLine.execute(args);
if (exitCode == 0) {
LOG.info("Success");
} else {
LOG.info("Failed");
if (exitCode != 0) {
LOG.error("Cobigen terminated in an unexpected manner, fore more info execute in verbose mode (-v).");
jan-vcapgemini marked this conversation as resolved.
Show resolved Hide resolved
}
System.exit(exitCode);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ public Integer call() throws Exception {

if (this.verbose) {
CLILogger.setLevel(Level.DEBUG);
} else {
CLILogger.setLevel(Level.INFO);
jan-vcapgemini marked this conversation as resolved.
Show resolved Hide resolved
}

return doAction();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ public Integer doAction() throws Exception {
CobiGen cg = CobiGenUtils.initializeCobiGen(this.templatesProject);

resolveTemplateDependencies();

if (this.increments == null && this.templates != null) {
Tuple<List<Object>, List<TemplateTo>> inputsAndArtifacts = preprocess(cg, TemplateTo.class);
for (int i = 0; i < inputsAndArtifacts.getA().size(); i++) {
Expand Down Expand Up @@ -331,16 +330,20 @@ public <T extends GenerableArtifact> void generate(Path inputFile, Object input,
inputFile);
report = cg.generate(input, generableArtifacts, this.outputRootPath.toAbsolutePath(), false, (task, progress) -> {
});
ValidationUtils.checkGenerationReport(report);
Set<Path> generatedJavaFiles = report.getGeneratedFiles().stream().filter(e -> e.getFileName().endsWith(".java"))
.collect(Collectors.toSet());
if (!generatedJavaFiles.isEmpty()) {
try {
ParsingUtils.formatJavaSources(generatedJavaFiles);
} catch (FormatterException e) {
LOG.warn(
"Generation was successful but we were not able to format your code. Maybe you will see strange formatting.");
boolean generationSuccess = ValidationUtils.checkGenerationReport(report);
if (generationSuccess) {
Set<Path> generatedJavaFiles = report.getGeneratedFiles().stream().filter(e -> e.getFileName().endsWith(".java"))
.collect(Collectors.toSet());
if (!generatedJavaFiles.isEmpty()) {
try {
ParsingUtils.formatJavaSources(generatedJavaFiles);
} catch (FormatterException e) {
LOG.warn(
"Generation was successful but we were not able to format your code. Maybe you will see strange formatting.");
jan-vcapgemini marked this conversation as resolved.
Show resolved Hide resolved
}
}
} else {
LOG.error("Generation not successful! For more info execute in verbose mode (-v).");
jan-vcapgemini marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,28 +76,27 @@ public static boolean isOutputRootPathValid(Path outputRootPath) {
* Checks the generation report in order to find possible errors and warnings
*
* @param report the generation report returned by the CobiGen.generate method
* @return true if generation succeeded, false otherwise
*/
public static void checkGenerationReport(GenerationReportTo report) {
public static boolean checkGenerationReport(GenerationReportTo report) {

for (String warning : report.getWarnings()) {
LOG.debug("Warning: {}", warning);
}

// Successful generation
if (report.getErrors() == null || report.getErrors().isEmpty()) {
LOG.info("Successful generation.");
} else {
if (LOG.isDebugEnabled() && report.getErrors().size() > 1) {
for (int i = 1; i < report.getErrors().size(); i++) {
LOG.error("Further reported error:", report.getErrors().get(i));
}
}
if (report.getErrors().get(0) instanceof CobiGenRuntimeException) {
throw report.getErrors().get(0);
} else {
throw new CobiGenRuntimeException("Generation failed. Enable debug mode to see the exceptions occurred.",
report.getErrors().get(0));
}
return true;
}
// Unsuccesful generation, log error messages to user
// Log exceptions for devs in DEBUG
for (int i = 0; i < report.getErrors().size(); i++) {
RuntimeException e = report.getErrors().get(i);
LOG.error("Error: {}", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Don't print messages of exceptions. They will be in the stacktrace anyhow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error message from the exception might contain useful hints for the user on how to fix it himself. My idea was to move the stacktrace of the exception into DEBUG but keep the exception message.

if (LOG.isDebugEnabled()) e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Don't print stacktraces on your own, pass e on debug as last parameter on log function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in next commit.

}
return false;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,14 @@ public void process(TextTemplate template, Map<String, Object> model, Writer out
env.setLogTemplateExceptions(false); // no duplicate logging
env.process();
} catch (TemplateException e) {
String detailedCause = "";
if (e.getCause().getClass().getCanonicalName().contains("java.lang")) {
Copy link
Member

Choose a reason for hiding this comment

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

Just getting the first cause is maybe not what you want. Most probably you want to look at the root cause wouldn't you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! Next commit extracts the rootcause before building error message.

detailedCause = ". A problem with Reflection has likely occurred: "
+ e.getCause().toString()
Copy link
Member

Choose a reason for hiding this comment

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

Never log an exception to string. That's most often resulting in garbage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed like a quick way to get both the canonical type of the exception and the message into one String. I rewrote the extraction manually.

+ ", please consider rebuilding your project as a possible fix.";
}
throw new CobiGenRuntimeException("An error occurred while generating the template: "
+ template.getAbsoluteTemplatePath() + " (FreeMarker v" + FreemarkerMetadata.VERSION + ")", e);
+ template.getAbsoluteTemplatePath() + " (FreeMarker v" + FreemarkerMetadata.VERSION + ")" + detailedCause, e);
jan-vcapgemini marked this conversation as resolved.
Show resolved Hide resolved
} catch (Throwable e) {
throw new CobiGenRuntimeException("An unkonwn error occurred while generating the template: "
+ template.getAbsoluteTemplatePath() + " (FreeMarker v" + FreemarkerMetadata.VERSION + ")", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,9 @@ private Object readInput(Path path, Charset inputCharset, Map<TriggerInterpreter
"Was not able to read input {} with inputreader '{}' although it was reported to be most likely readable. Trying next input reader...",
path, triggerInterpreter, e);
} catch (Throwable e) {
LOG.debug("While reading the input {} with the inputreader {}, an exception occured. Trying next input reader...",
path, triggerInterpreter, e);
LOG.warn("While reading the input {} with the inputreader {}, an exception occured: {} Trying next input reader...",
path, triggerInterpreter, e.getMessage());
if (LOG.isDebugEnabled()) e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Don't print stacktraces on your own, pass the e on case of debug as last log parameter

}
return null;
}
Expand Down