Java SE 11 Developer: Suppressed Exceptions and Misused Assertions in a Nightly Import

Suppressed Exceptions and Misused Assertions in a Nightly Import

A nightly importer reads a CSV through a custom resource:

try (CsvSource src = new CsvSource(path)) {
    for (Row r : src) { validator.check(r); store(r); }
} catch (Exception e) {
    throw new ImportException("import failed", e);
} catch (IOException e) {
    log.error("io failure", e);
} finally {
    metrics.flush();
}

CsvSource.close() declares IOException; store throws IOException when the disk fills; metrics.flush() can throw an unchecked MetricsException. The build fails on this class. In production the JVM runs without -ea, and rows with a null id reach store and fail with a NullPointerException deep in the persistence layer instead of the intended validation message. validator.check begins with assert row.getId() != null : "missing id";, and ImportException is unchecked.

1. The class does not compile as written. Identify the error, and explain what the caller sees once the ordering is fixed.

Read the worked answer

IOException is a subtype of Exception, so the later catch (IOException e) block is unreachable and javac rejects the file with exception IOException has already been caught. The rule is mechanical: a more specific catch type must precede a broader one; Exception must come last. (The same error appears if you catch an unchecked type after Exception.)

After reordering, the caller gets a wrapped exception, but note which exceptions are wrapped: catch (Exception e) also captures the IOException that close() may throw from the resource, and any RuntimeException from store, all labelled import failed. That is a diagnostic problem, not just a style issue: the caller cannot distinguish a retryable disk-full condition from a programming bug, and the log says the import failed even when every row was written and only the close() flush failed.

A more useful shape is specific catches, ordered narrow to broad, with distinct outcomes:

catch (IOException e)        { throw new ImportException("io", e); }        // retryable
catch (RuntimeException e)   { throw new ImportException("bug", e); }       // not retryable

Multi-catch (catch (IOException | SQLException e)) is appropriate when the handling is genuinely identical. One more subtlety: a try-with-resources statement can surface IOException from close() even when the body never declares it, so the catch set must account for the resource contract, not only the loop body.

2. Both the loop body and `close()` throw `IOException`, and `metrics.flush()` then throws. What reaches the caller, and how do you make every failure visible in the log?

Read the worked answer

Body plus close. Try-with-resources propagates the body's exception as the primary exception and attaches the close() exception via addSuppressed. Calling getCause() returns null for it; the close failure is only visible through getSuppressed(). So the caller sees the original disk-full IOException (wrapped by the catch), with the close IOException suppressed underneath.

Body completes normally, close throws. Then close's IOException becomes the primary exception and reaches the catch block directly.

finally runs. metrics.flush() throwing MetricsException here discards whatever exception is in flight - the original is neither propagated nor logged, which is the worst outcome. A return in finally has the same effect. Fixes: wrap the flush in its own try/catch and log it, or make metrics a second resource in the try-with-resources header so its close failure is suppressed under the primary rather than replacing it:

try (CsvSource src = new CsvSource(path); MetricsScope m = metrics.openScope()) { ... }

Resources close in reverse declaration order, so m closes before src.

Logging. Log the throwable, not a string: log.error("import failed", e) renders suppressed exceptions with printStackTrace semantics, whereas log.error("import failed: " + e.getMessage()) silently drops the suppressed close failure and the stack trace. Never rebuild a wrapped exception from one of its suppressed children - you lose the ordering guarantee the JVM already recorded.

3. `validator.check` relies on `assert row.getId() != null`. Give three distinct problems with that and the correct validation approach.

Read the worked answer

It does not run in production. Assertions are disabled by default; unless the JVM is started with -ea (or -ea:com.acme...), the statement is compiled away entirely. That is exactly what happened: the null check never executed, and the failure surfaced later as an opaque NullPointerException inside the persistence layer, at a point where the offending row is no longer identifiable.
It is the wrong contract for validating input. Assertions express internal invariants that should hold if the code is correct - a broken invariant means a bug and AssertionError is the right signal. A null id in a CSV is bad input, so the caller should get IllegalArgumentException or NullPointerException with a message naming the row. Using assert for public-method argument checks or security checks is a defect, because the check can be switched off per package and the behaviour then differs between environments.
Side effects and environment dependence. Anything inside an assert expression vanishes at run time, so never put a check with side effects (or a method that must execute) there. Because -ea can be enabled selectively, code paths that depend on assertions become non-reproducible between staging and production.

Correct approach: validate at the boundary, before the loop or at the top of check, explicitly:

Objects.requireNonNull(row.getId(), () -> "missing id at line " + row.getLine());
if (row.getAmount() < 0) throw new IllegalArgumentException("negative amount at line " + row.getLine());

Keep assert for post-conditions you want during testing, such as assert rowsWritten == rowsRead : "row count mismatch";, and treat enabling -ea in CI as a requirement rather than a runtime assumption.