Expose body for non RFC 9457 error responses

[kwin]

Log only on DEBUG level
This closes #1844

Following this checklist to help us incorporate your
contribution quickly and easily:

• Your pull request should address just one issue, without pulling in other changes.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body.
  Note that commits might be squashed by a maintainer on merge.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
  This may not always be possible but is a best-practice.
• Run mvn verify to make sure basic checks pass.
  A more thorough check will be performed on your pull request automatically.
• You have run the integration tests successfully (mvn -Prun-its verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[gnodet]

Review Summary

Thanks for working on this, @kwin! Making non-RFC 9457 error response bodies visible for debugging is a useful improvement. I found one confirmed bug and a few design concerns worth discussing.

Bug: UncheckedIOException crashes the error-handling path

File: RFC9457Reporter.java (line ~105 in the else branch)

The catch (IOException e) block throws new UncheckedIOException(...). If getBody(response) encounters any I/O error (entirely plausible on network streams), this UncheckedIOException propagates up and prevents baseException.accept(statusCode, reasonPhrase) from ever executing. A failure in a diagnostic/debug code path should never abort the main error-handling flow. The IOException should be caught and logged or swallowed:

} catch (IOException e) {
    LOGGER.debug("Failed to read response body for status code {}", statusCode, e);
}

Concern: Eager stream consumption even when DEBUG is disabled

getBody(response) is passed as a direct argument to LOGGER.debug(), which means the response body stream is always consumed — even when DEBUG logging is disabled. For JDK and Jetty transports, this reads the entire InputStream on every non-RFC-9457 error response, wasting I/O unnecessarily. Consider guarding with LOGGER.isDebugEnabled() or using a lazy supplier.

Concern: Unbounded body size

The response body is read fully into a String with no size limit. A multi-megabyte HTML error page would be fully materialized in memory and logged. Consider truncating to a reasonable limit (e.g., 4096 characters).

Concern: slf4j-api added to SPI module

Adding slf4j-api as a compile-scope dependency to the SPI module increases the transitive dependency footprint for all SPI consumers. The SPI module currently has minimal dependencies (maven-resolver-api and gson). Consider whether this logging belongs in the transport implementations instead.

Note: DEBUG-only vs. issue #1844 intent

Issue #1844 asks for the body to be "exposed (at least partially) in the exception." The current implementation only logs at DEBUG level, which may not be sufficient for regular users diagnosing errors without debug logging enabled. This may be a deliberate design choice, but worth confirming it satisfies the issue's requirements.

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[cstamas]

Few remarks here:

• I dislike SPI pulling in logger, that is wrong IMHO
• logging on DEBUG makes not much sense, as these kind of errors "happen out of the blue" (not in debug), and then if user tries to catch them (and enabled debug) these bugs are usually not happening 😄

I'd rather not introduce logging here, but some sort of Consumer that could maybe just collect bodies, and have them possibly shown at build end, for example in case of build failure. Overall the idea is very good.

[elharo]

Per the Javadoc comment above this is incorrect. baseException should be thrown. I'm not sure if this should be fixed or if the doc needs to be changed, but they should be consistent

[elharo]

probably not worth throwing this at all if it just means we couldn't log a debug message

[kwin]

Thanks for the reviews.

Few remarks here:
...
I'd rather not introduce logging here, but some sort of Consumer that could maybe just collect bodies, and have them possibly shown at build end, for example in case of build failure. Overall the idea is very good.

Any pointers how to not make it too verbose? Collecting all failed response bodies (even extracts) may be too heavyweight or do you suggest only collecting the first/last one?

Who should be responsible for logging?

1. RepositorySystemSession.CloseableSession with its close() method or
2. The consumer setting up the session (Maven in most cases)

[cstamas]

I would go with simplest solution:

• collecting first/last (by next one make previous one lost) is completely okay
• just provide some config, for example to set a consumer in session
• and yes, I would leave this to "integrator" (ie Maven) as Resolver does not format error messages and such, so it would be Maven that would need to somehow show this information in case of build failure.

@gnodet maybe some ideas?

[gnodet]

I agree with @cstamas that the SPI module should not pull in a logger dependency, and that a Consumer-based approach would be a cleaner design.

Here's a possible direction:

1. Define a callback in the session config — something like a BiConsumer<Integer, String> (status code + body excerpt) that transports call when they encounter a non-RFC 9457 error response with a body. This keeps the SPI clean and lets the integrator decide what to do with it.

2. Keep it simple for collection — cstamas's suggestion of "last one wins" (overwriting previous) is the simplest and avoids unbounded memory growth. Alternatively, a small bounded list (e.g., last 3) could give enough context without being overwhelming. The consumer itself can decide the retention policy.

3. Truncate at the source — regardless of the collection strategy, the transport should truncate the body before passing it to the consumer (e.g., first 4 KiB). This avoids materializing multi-megabyte HTML error pages in memory.

4. Display at build end on failure — Maven (as integrator) could register a consumer that collects these, then displays them in a "diagnostic hints" section when the build fails. This addresses the original issue Expose response body digest for unexpected status codes #1844 intent better than DEBUG logging, since users would see the response body without needing to enable debug.

For the session config key, something like aether.transport.http.errorBodyConsumer would follow the existing naming conventions.

This way the SPI stays dependency-free, the body is available where it matters (build failure output), and the transport implementations just need to call the consumer when they have a response body to report.

[gnodet]

The review feedback from maintainers (gnodet, cstamas, elharo — July 13) has not been addressed. The PR still contains only the original commit from July 11.

Key unresolved issues:

1. Bug — UncheckedIOException crashes error-handling path: The catch (IOException e) block on line 107 throws UncheckedIOException, which propagates out and prevents baseException.accept(statusCode, reasonPhrase) from executing. A failure in a diagnostic/debug code path must never abort the main error-handling flow.

2. Architecture — slf4j-api in SPI module: Adding slf4j-api as a compile dependency to the SPI module has been explicitly rejected by maintainers. cstamas stated "I dislike SPI pulling in logger, that is wrong IMHO" and gnodet agreed. The maintainers converged on a Consumer-based callback approach instead.

3. Eager stream consumption: getBody(response) is evaluated as a method argument to LOGGER.debug() regardless of whether DEBUG is enabled — the full response body is always read.

4. No body size limit: The response body is read fully into a String with no truncation. A multi-megabyte HTML error page would be fully materialized in memory.

5. Doesn't satisfy issue #1844: The issue asks to "expose (at least partially) [the response body] in the exception." DEBUG-only logging doesn't meet this requirement for production builds.

6. No test coverage for the new else branch in generateException().

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet