Improve error handling across codebase

Author: jha9262

.github/workflows/ci.yml

@@ -32,11 +32,14 @@ jobs:
 
       - name: Build with Maven
         run: ./mvnw -e --no-transfer-progress clean install
+        continue-on-error: false
 
       - name: Check for changes
         shell: bash
         run: |
-          git diff --exit-code || (
+          if ! git diff --exit-code; then
             echo "::error::Code changes detected - run './mvnw clean verify' locally"
+            echo "::error::Modified files:"
+            git diff --name-only
             exit 1
-          )
+          fi

ERROR_HANDLING_IMPROVEMENTS.md

@@ -0,0 +1,62 @@
+# Error Handling Improvements
+
+## What's This About?
+
+Fixed a bunch of error handling issues that were flagged in the code review. Basically, the code wasn't checking for bad inputs properly, which could lead to cryptic NPEs and confusing error messages.
+
+## What Changed?
+
+### CI Workflow (.github/workflows/ci.yml)
+
+The Maven build and git diff steps were failing silently or with unclear errors. Now:
+- Added explicit `continue-on-error: false` so we know immediately when builds fail
+- Rewrote the git diff check to use proper if-statements instead of that weird `||` syntax
+- When files change, it actually shows you which files changed (super helpful for debugging)
+
+### Config File (rewrite.yml)
+
+Added some comments at the top explaining what this file does and how to use it. Also added inline comments for the CheckstyleAutoFix recipe so people know what each path is for and what format it expects.
+
+### Java Code
+
+#### CheckstyleAutoFix.java
+Added a simple `validateInputs()` method that checks if paths are null or empty before we try to use them. Throws clear exceptions like "Violation report path cannot be null or empty" instead of letting it blow up later with an NPE.
+
+#### XmlReportParser.java
+Now checks:
+- Is the path null?
+- Does the file actually exist?
+- Does the XML have a filename attribute?
+
+No more mysterious failures when parsing broken XML files.
+
+#### SarifReportParser.java
+Added validation for pretty much everything:
+- Path exists?
+- File is valid SARIF?
+- Has runs and results?
+- All required fields present (level, message, location, etc.)?
+
+Basically, fail fast with a helpful message instead of crashing halfway through.
+
+#### ConfigurationLoader.java
+Just added some basic null/empty checks for the config path and properties file. Nothing fancy, just defensive programming.
+
+## Why This Matters
+
+- No more NPEs that make you dig through stack traces
+- Error messages actually tell you what's wrong
+- Fails early instead of halfway through processing
+- Easier to debug when something goes wrong
+- CI failures are way more obvious
+
+## Testing
+
+Should probably test:
+- Missing files
+- Malformed XML/SARIF
+- Empty configs
+- Null parameters
+- CI behavior on failures
+
+But honestly, the main thing is that errors are now obvious instead of cryptic.

rewrite.yml

@@ -1,4 +1,6 @@
-# mvn rewrite:run -Drewrite.activeRecipes=org.checkstyle.recipes.OpenRewriteRecipeBestPractices
+# OpenRewrite Configuration for Checkstyle Auto-Fix
+# Usage: mvn rewrite:run -Drewrite.activeRecipes=org.checkstyle.recipes.OpenRewriteRecipeBestPractices
+# This file defines recipes for automatically fixing Checkstyle violations and applying best practices.
 ---
 type: specs.openrewrite.org/v1beta/recipe
 name: org.checkstyle.recipes.OpenRewriteRecipeBestPractices
@@ -101,6 +103,11 @@ tags:
   - code-quality
 recipeList:
   - org.checkstyle.autofix.CheckstyleAutoFix:
+      # Path to Checkstyle violation report (XML or SARIF format)
+      # Ensure this file exists before running the recipe
       violationReportPath: "target/checkstyle/checkstyle-report.xml"
+      # Path or URL to Checkstyle configuration file
+      # Must be accessible and valid XML format
       configurationPath: "https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-${checkstyle.version}/config/checkstyle-checks.xml"
+      # Optional: Path to properties file for Checkstyle configuration
       propertiesPath: "config/checkstyle.properties"

src/main/java/org/checkstyle/autofix/CheckstyleAutoFix.java

@@ -85,6 +85,7 @@ public String getPropertiesPath() {
 
     @Override
     public List<Recipe> getRecipeList() {
+        validateInputs();
         final ReportParser reportParser = createReportParser(getViolationReportPath());
         final List<CheckstyleViolation> violations = reportParser
                 .parse(Path.of(getViolationReportPath()));
@@ -94,6 +95,17 @@ public List<Recipe> getRecipeList() {
         return CheckstyleRecipeRegistry.getRecipes(violations, configuration);
     }
 
+    private void validateInputs() {
+        if (violationReportPath == null || violationReportPath.trim().isEmpty()) {
+            throw new IllegalArgumentException(
+                    "Violation report path cannot be null or empty");
+        }
+        if (configurationPath == null || configurationPath.trim().isEmpty()) {
+            throw new IllegalArgumentException(
+                    "Configuration path cannot be null or empty");
+        }
+    }
+
     private ReportParser createReportParser(String path) {
         final ReportParser result;
         if (path.endsWith(".xml")) {

src/main/java/org/checkstyle/autofix/parser/ConfigurationLoader.java

@@ -71,16 +71,25 @@ private static Map<String, String> getProperties(Configuration config) {
 
     public static Map<CheckstyleCheck, CheckConfiguration> loadConfiguration(
             String checkstyleConfigurationPath, String propFile) {
+        if (checkstyleConfigurationPath == null || checkstyleConfigurationPath.trim().isEmpty()) {
+            throw new IllegalArgumentException(
+                    "Checkstyle configuration path cannot be null or empty");
+        }
+
         Properties props = new Properties();
         if (propFile == null) {
             props = System.getProperties();
         }
         else {
+            if (propFile.trim().isEmpty()) {
+                throw new IllegalArgumentException("Properties file path cannot be empty");
+            }
             try (FileInputStream input = new FileInputStream(propFile)) {
                 props.load(input);
             }
             catch (IOException exception) {
-                throw new IllegalStateException("Failed to read: " + propFile, exception);
+                throw new IllegalStateException("Failed to read properties file: "
+                        + propFile, exception);
             }
         }
 

src/main/java/org/checkstyle/autofix/parser/SarifReportParser.java

@@ -46,13 +46,23 @@ public SarifReportParser() {
 
     @Override
     public List<CheckstyleViolation> parse(Path reportPath) {
+        if (reportPath == null) {
+            throw new IllegalArgumentException("Report path cannot be null");
+        }
+        if (!reportPath.toFile().exists()) {
+            throw new IllegalArgumentException("Report file does not exist: " + reportPath);
+        }
+
         final SarifSchema210 report;
         try {
             report = parser.fromFile(reportPath);
         }
         catch (IOException exception) {
             throw new IllegalArgumentException("Failed to parse report: " + reportPath, exception);
         }
+        if (report == null || report.getRuns() == null || report.getRuns().isEmpty()) {
+            throw new IllegalArgumentException("SARIF report is empty or invalid: " + reportPath);
+        }
         final List<CheckstyleViolation> result = new ArrayList<>();
         for (final Run run: report.getRuns()) {
             if (run.getResults() != null) {
@@ -68,11 +78,24 @@ public List<CheckstyleViolation> parse(Path reportPath) {
     }
 
     private CheckstyleViolation createViolation(CheckstyleCheck check, Result result) {
+        if (result.getLevel() == null) {
+            throw new IllegalStateException("Result level is missing");
+        }
+        if (result.getMessage() == null || result.getMessage().getText() == null) {
+            throw new IllegalStateException("Result message is missing");
+        }
+        if (result.getLocations() == null || result.getLocations().isEmpty()) {
+            throw new IllegalStateException("Result locations are missing");
+        }
+
         final String severity = result.getLevel().name();
         final String message = result.getMessage().getText();
         final PhysicalLocation location = result.getLocations().get(0).getPhysicalLocation();
         final Path filePath = getFilePath(location);
         final Region region = location.getRegion();
+        if (region == null || region.getStartLine() == null) {
+            throw new IllegalStateException("Region or start line is missing");
+        }
         final int line = region.getStartLine();
         final Optional<Integer> columnMaybe = Optional.ofNullable(region.getStartColumn());
         return columnMaybe.map(column -> {

src/main/java/org/checkstyle/autofix/parser/XmlReportParser.java

@@ -56,6 +56,12 @@ public class XmlReportParser implements ReportParser {
 
     @Override
     public List<CheckstyleViolation> parse(Path xmlPath) {
+        if (xmlPath == null) {
+            throw new IllegalArgumentException("XML path cannot be null");
+        }
+        if (!xmlPath.toFile().exists()) {
+            throw new IllegalArgumentException("XML file does not exist: " + xmlPath);
+        }
 
         final List<CheckstyleViolation> result = new ArrayList<>();
 
@@ -108,6 +114,9 @@ private String parseFileTag(StartElement startElement) {
                 break;
             }
         }
+        if (fileName == null || fileName.trim().isEmpty()) {
+            throw new IllegalStateException("File name attribute is missing or empty");
+        }
         return fileName;
     }