gnodet commented on PR #1907:
URL: https://github.com/apache/maven-resolver/pull/1907#issuecomment-4649120003
Thanks for this contribution, @jiteshkhatri11! This is a well-motivated
addition — `addSslContext()` is the natural complement to the existing
`addHostnameVerifier()` method (added back in Bug 411445 / commit `78261748`),
and it fills a real gap for mTLS use cases.
**What I checked:**
- **Code correctness:** The implementation correctly mirrors
`addHostnameVerifier` — null guard, `ComponentAuthentication` delegation with
`AuthenticationContext.SSL_CONTEXT` as the key, and builder chaining. ✔️
- **Import ordering:** `javax.net.ssl.SSLContext` is correctly placed
alongside `HostnameVerifier` in the `javax` import group, before the blank-line
separator and `java.util` imports. ✔️
- **Javadoc:** The `<strong>Note:</strong>` warning about
`ComponentAuthentication` semantics (digest based on runtime type, not
configuration) is correctly adapted from `addHostnameVerifier`. This is
important — `ComponentAuthentication.digest()` uses
`value.getClass().getName()`, so two `SSLContext` instances of the same
implementation class but configured with different `KeyManager`s will produce
the same digest. The Javadoc properly directs users to
`addCustom(Authentication)` for configuration-sensitive contexts. ✔️
- **Transporter support:** All three transport implementations already read
`AuthenticationContext.SSL_CONTEXT`:
- `JdkTransporter.java` (line 478)
- `JettyTransporter.java` (line 378)
- `ConnMgrConfig.java` (line 67, used by the Apache transport)
So the new builder method will work across all currently supported
transports. ✔️
- **Null-safety:** Consistent with every other method in
`AuthenticationBuilder` — null input is a no-op, returns `this`. ✔️
**One minor Javadoc suggestion** (posted as an inline comment): consider
adding `@see AuthenticationContext#SSL_CONTEXT` to help users navigating the
API find the constant and its documentation. This is optional.
**On tests:** I see the PR checklist has "Unit tests written" unchecked.
There's no `AuthenticationBuilderTest` in the project today — the underlying
`ComponentAuthentication` is already covered by `ComponentAuthenticationTest`
(fill, digest, equals, hashCode). Given that this is a thin convenience method
delegating to well-tested infrastructure, the lack of a dedicated test isn't a
blocker. That said, if you'd like to add one, the existing
`ComponentAuthenticationTest` pattern would work well as a template.
Overall this looks good to me. Clean, minimal, follows the established
pattern.
— gnodet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
