Expose transporter details per protocol

[kwin]

Instead of purely relying on exception handling when a transporter cannot deal with a certain kind of repository an in advance check on the repository protocol is introduced

This closes #1770

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]

Maybe to ease listing all transporters it would be better to expose a list of supported protocols (potentially including a wildcard). WDYT @cstamas?

[cstamas]

Isn't requesting an instance for RemoteRepository (and getting NoTransportEx) same?

[cstamas]

Especially as sometimes you cannot tell ahead of time. for HTTP yes, but for file?

[kwin]

It is different in the following aspect: This just requires a protocol not a full blown remote repository. It doesn't rely on exceptions but just returns a boolean.

[cstamas]

I am for exposing (what is related to that in this PR), but I am -1 on canHandle(String) method as I find it flawed. Remember, that RemoteRepository, while it has getProtocol() is waay lax interpreted, and is not strict. Many times it cannot be told even what proto is...

See https://github.com/apache/maven-resolver/blob/master/maven-resolver-api/src/test/java/org/eclipse/aether/repository/RemoteRepositoryTest.java
but also
https://github.com/apache/maven-resolver/blob/master/maven-resolver-api/src/main/java/org/eclipse/aether/repository/RepositoryUriUtils.java

So "" (empty string) proto is completely valid....

[cstamas]

We have cases like dav+http or symlink+file etc. And as said above, we have cases where it is just empty string, but RemoteRepository would "still work", etc

[kwin]

I looked at all transporters we have, and while it is true that some evaluate more than just protocol, they all evaluate the protocol first. Therefore having this aspect separated is IMHO useful for this feature (but also for determining the correct transporter) as in the best case you don't have the overhead of throwing an exception as you bail out for a non-matching protocol first.

[cstamas]

https://github.com/taxilang/taxilang/blob/7d83962001b73a37b5c18f8992ef9c6e87d17e97/package-manager/src/main/java/org/taxilang/packagemanager/transports/TaxiFileSystemTransport.kt#L25
and so on... I don't think you can expect this from any transport.

[cstamas]

Also, some transport may support or not support something based on config... and again, protocol is just not enough to decide all this. Either use repo url or whole repo instance. Hence, my 5 cents would be:

• make the method canHandle(RepositorySystemSession, RemoteRepository) instead. But, the erasure of this method now became same as that of newInstance....

[kwin]

I am open for other suggestions :-). I am not saying that this covers all transporters, but this is just an additional layer. If there is a decision based on protocol impossible the transporter just returns true for canHandle(...) implying that it may be able to fulfil this request.

[gnodet]

AI Review — PR #1771: Expose transporter details per protocol

Hi @kwin, thanks for working on improving protocol handling in the transporter layer!

After review and independent verification, several significant issues were confirmed:

🔴 FileTransporterFactory.canHandle breaks compound protocols (high)

canHandle returns true only for "bundle" and "file", but newInstance also handles symlink+file, hardlink+file, and custom FS providers (JIMFS). The code comments in FileTransporterFactory explicitly state: "to support custom FS providers (like JIMFS), we cannot cover all possible protocols." With the PR's DefaultTransporterProvider changes, canHandle("symlink+file") returns false, causing the factory to be skipped entirely — breaking existing functionality for symlink, hardlink, and JIMFS-based file transport.

🔴 Protocol validation removed from newInstance (high)

On master, all transport factory newInstance() methods throw NoTransporterException for unsupported protocols. The PR removes these checks with a comment "repository protocol already checked in canHandle." This breaks the SPI contract for direct callers who invoke newInstance() without going through DefaultTransporterProvider. The Javadoc still declares @throws NoTransporterException but the implementations no longer honor it.

🔴 getProperties() default throws UnsupportedOperationException (high)

Throwing from a default method in an SPI interface defeats the purpose of default methods (backward-compatible no-op). Any caller that invokes it gets a runtime exception. A default returning Collections.emptyMap() would be the standard Java idiom.

⚠️ Error accumulation degraded (medium)

On master, every factory that rejects a protocol contributes to the final diagnostic error message. With canHandle, factories returning false are silently skipped with only a DEBUG log — making the "Cannot access ... using the registered transporter factories" error incomplete and harder to diagnose.

⚠️ Test coverage regression (medium)

testInit_BadProtocol was changed to only test canHandle instead of newInstance, removing coverage for newInstance behavior with unsupported protocols.

Note: The concern about canHandle(String) being too narrow for compound protocols was evaluated as a design discussion (not a bug), since the default return true implementation allows factories to fall through to newInstance() as before. This aligns with @cstamas's feedback in the PR discussion.

CI is also failing across all platforms.

---

🤖 This review was generated by ForgeBot using a maker/checker pattern (reviewer + independent verifier). 1 of 7 original findings was removed as a false positive after verification.