Expose additional transport details to TransportListener

[kwin]

For HTTP Transporters this is:

• HTTP Version
• SSL Protocol (only HTTPS)
• SSL Cipher Suite (only HTTPS)
• Compression Algorithm (if used)
• Transport Size (if it differs from data size)

This closes #1761

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.

[kwin]

@cstamas WDYT about this proposal?

[cstamas]

Looks good

[gnodet]

AI Review — PR #1762: Expose additional transport details to TransportListener

Hi @kwin, thanks for this feature — the TransportPropertyKey design with HTTP-specific enum implementation is well-structured and extensible.

🔴 TransferEvent.transportProperties field visibility (high)

The new transportProperties field is package-private (no access modifier) while every other field in TransferEvent is private final. This breaks the class's immutability contract. Additionally:

• The Builder's copy() constructor doesn't copy transportProperties from the prototype
• The field defaults to null, but the Javadoc says "may be empty" (implying never null)
• Events built without calling setTransportProperties() (INITIATED, PROGRESSED, CORRUPTED, FAILED, SUCCEEDED) will have getTransportProperties() return null

Fix: Make the field private final, initialize to Collections.emptyMap() in the Builder, and copy in copy().

🔴 HttpTransportPropertiesBuilder.build() returns mutable reference (high)

build() returns the raw internal HashMap reference. While TransferEvent.Builder.setTransportProperties() wraps it in Collections.unmodifiableMap(), the builder retains a reference to the mutable map. Continuing to mutate the builder after build() would affect the already-returned map. Fix: Return Collections.unmodifiableMap(properties) from build().

⚠️ Apache transporter PUT doesn't report transport properties (medium)

ApacheTransporter.implPut still uses the deprecated 3-arg utilPut(), so PUT operations never report transport properties. Jetty's implPut does report them. This is an inconsistency across transporters.

⚠️ JdkTransporter.implPeek fires transportStarted() (medium)

This PR adds a transportStarted() call in implPeek() that didn't exist before. PeekTask's Javadoc explicitly says its listener is "always a noop." Apache and Jetty don't fire events during peek. This is currently harmless but creates an inconsistent contract.

💡 Dead SslProtocol enum values (low)

SSL_1_0 and SSL_2_0 are declared but fromStandardName() doesn't handle them — they're unreachable. These protocols were removed from Java years ago. Either add cases (for completeness) or remove the constants.

What works well:

• Clean TransportPropertyKey marker interface extensible for non-HTTP transports
• Thoughtful backwards compat with deprecated overloads in AbstractTransporter and TransportListener
• Solid test coverage in TransportListenerNotifyingInputStreamTest and shared HttpTransporterTest

---

🤖 This review was generated by ForgeBot using a maker/checker pattern (reviewer + independent verifier). All 5 findings were independently confirmed.

[gnodet]

Thanks for the thorough rework, @kwin! The new commit addresses all 5 findings from my earlier review:

1. ✅ transportProperties field is now private final with emptyMap() default — immutability contract restored
2. ✅ HttpTransportPropertiesBuilder.build() returns unmodifiableMap — no more mutable reference leak
3. ✅ Apache transporter now reports transport properties on all task types (Get, Peek, Put) via a dedicated HttpRequestExecutor
4. ✅ The new transportPropertiesAvailable() method is a clean design — it decouples property notification from the transportStarted() transfer lifecycle, solving the PeekTask inconsistency without overloading the event semantics
5. ✅ Dead SSL_1_0 / SSL_2_0 enum values removed

The transportPropertiesAvailable() approach is a nice architectural improvement over the original design — separating concerns between "transfer is starting" and "transport metadata is available" makes the API much clearer.

---

🤖 This follow-up was generated by ForgeBot. Reviewing new commits on a previously reviewed PR.

[gnodet]

Review Summary

Nice work on this, @kwin! The overall API design with TransportPropertyKey as a marker interface and per-transport enum implementations is clean and extensible without breaking backward compatibility. The separation of transportPropertiesAvailable() from the transfer lifecycle events is a good architectural choice.

I found a few issues worth addressing:

Medium Severity

1. properties field visibility in HttpTransportPropertiesBuilder — The properties field has package-private visibility (no access modifier) in a public final SPI class. It should be private final to enforce proper encapsulation.

2. Mutable-backed unmodifiable map from build() — Collections.unmodifiableMap(properties) creates a view backed by the same HashMap. If the builder were reused after build(), the previously returned map would reflect mutations. Using Collections.unmodifiableMap(new HashMap<>(properties)) or Map.copyOf(properties) would produce a truly detached snapshot. Current callers are safe (they don't reuse the builder), but as a public SPI class the API contract should be defensive.

Low Severity

3. Broken Javadoc link in HttpTransportProperty.java — {@link #HttpVersion} should be {@link HttpVersion} (without the #). The # prefix means "member of the current type" which refers to the Key enum, not the HttpVersion enum.

4. Self-referencing Javadoc link in HttpTransportProperty.java — {@link #SSL_PROTOCOL} should be {@link SslProtocol} to reference the value type, not the constant being documented.

5. Broken @see reference in HttpTransportPropertiesBuilder — @see TransportListener#transportStarted(long, long, Map) references a non-existent 3-parameter method. Should be @see TransportListener#transportPropertiesAvailable(Map).

---

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

[gnodet]

This field has package-private visibility (no access modifier). Since this is a public final SPI class, it should be private final to enforce encapsulation and prevent same-package classes from accessing the mutable map directly.

Suggested change

	* @since NEXT
	private final Map<TransferEvent.TransportPropertyKey, Object> properties = new HashMap<>();

[gnodet]

Collections.unmodifiableMap(properties) creates an unmodifiable view backed by the same HashMap. If the builder is reused after calling build(), the previously returned map would reflect mutations. Consider using a defensive copy:

Suggested change

	public HttpTransportPropertiesBuilder withSslProtocol(String name) {
	public Map<TransferEvent.TransportPropertyKey, Object> build() {
	return Collections.unmodifiableMap(new HashMap<>(properties));
	}