gnodet commented on PR #1923:
URL: https://github.com/apache/maven-resolver/pull/1923#issuecomment-4702778112
## In-Depth Review: PR #1923 — Fix ApacheTransport auth cache CCEx
### Design Assessment
The `Keys` helper class is a clean, well-considered solution to the
ClassLoader-scoping problem. Rather than chasing individual key sites with
ad-hoc classloader-aware string concatenation (as the initial fix in `ec6bd858`
did with `identityHashCode(classLoader)`), this PR correctly identifies that
the **same mechanism** — using `Class` objects as key elements — naturally
provides ClassLoader scoping, since `Class.equals()` is identity-based and
Class instances are ClassLoader-scoped by definition.
Placing `Keys` in `maven-resolver-api` is the right call: both `SessionData`
and `RepositoryCache` live there, and the key contract is defined by those
interfaces. The unified factory approach (`Keys.of(...)`) replaces a scattered
mix of string concatenation, custom inner classes (`LoggedMirror`), and bare
`new Object()` sentinels with a single, consistent pattern.
The three key "flavors" (global/string-only,
ClassLoader-aware/Class-element, private/Object-identity) emerge naturally from
the element types without needing separate factory methods — this is elegant.
### Key Findings
1. **Defensive copy (Keys.java:91)** — The varargs array is stored without
cloning. Safe for all current callers but leaves a footgun in a public API
class. See inline comment.
2. **Javadoc nits (Keys.java:31–39)** — `<p/>` (should be `<p>`), "our" →
"out", "od" → "of". See inline comment.
3. **Subkey expansion asymmetry (Keys.java:75)** — Only first-position `Key`
elements are expanded. The Javadoc could be more explicit about this. See
inline comment.
4. **Copy-paste bug fix confirmed
(RemoteRepositoryFilterSourceSupport.java:121)** — `git blame` confirms
`FileTrustedChecksumsSourceSupport.class` was copy-pasted from #1679 and never
corrected. Good catch. See inline comment.
5. **Public field type change (OptionalDependencySelector.java:41-42)** —
`IGNORED_KEYS` and `UNSELECTED_KEYS` change from `String` to `Object`. Not in
the public API surface (api/spi/util), but these are `public` fields. See
inline comment.
6. **`DefaultRemoteRepositoryManager`: `LoggedMirror` removal** — The inner
class is replaced with `Keys.of(mirror.getId(), mirror.getUrl(),
original.getId(), original.getUrl())`. Semantically equivalent (both use
`Arrays.equals`/`hashCode` on the same 4 string elements). Clean simplification.
7. **`DefaultUpdateCheckManager`: private key pattern preserved** —
`Keys.of(new Object() { toString() })` correctly maintains the identity-based
"private key" semantics of the original bare `new Object()`. The `toString()`
override is preserved for debuggability. The parameter type tightening (`Object
updateKey` → `String updateKey`) is a correct refinement.
### Correctness Analysis
**The core fix is correct.** The root cause of the ClassCastException is:
1. `ApacheTransporter` stored an `AuthCache` (Apache HttpClient class) in
`RepositoryCache` under a string key containing `getClass().getSimpleName()`
(just `"ApacheTransporter"`)
2. When the same transport is loaded by two ClassRealms, both instances
share the same string key
3. Instance A stores an `AuthCache` from ClassLoader 1
4. Instance B retrieves it and casts to `AuthCache` from ClassLoader 2 →
`ClassCastException`
The fix uses `getClass()` (a `Class` object, not its name) as a key element,
so instances from different ClassLoaders produce different keys via
identity-based `Class.equals()`. This is the correct approach.
**All 15 migration sites use the right key scope:**
| Component | Old pattern | New pattern | Scope correct? |
|-----------|------------|-------------|----------------|
| `ApacheTransporter` | `getSimpleName() + repo` | `Keys.of(getClass(),
repo)` | Yes — ClassLoader-aware (transport module) |
| `GlobalState` | `class.getName()` | `Keys.of(class)` | Yes —
ClassLoader-aware (transport module) |
| `DataPool` (4 keys) | `class.getName() + "$X"` | `Keys.of(class, "x")` |
Yes — ClassLoader-aware (impl, safe for single-load) |
| `PrioritizedComponents` | `class.getName() + disc + hash` |
`Keys.of(class, disc, hash)` | Yes — ClassLoader-aware with discriminator |
| `DefaultRepositoryKeyFunctionFactory` | `owner.getName() + ".X"` |
`Keys.of(owner, "X")` | Yes — `owner` is already a Class param |
| `DefaultRemoteRepositoryFilterManager` | `class.getName() + ".instance." +
sessionHash` | `Keys.of(class, "instance", sessionHash)` | Yes |
| `GroupIdRemoteRepositoryFilterSource` (4 keys) | `getClass().getName() +
".X"` | `Keys.of(class, "X")` | Yes |
| `PrefixesRemoteRepositoryFilterSource` (2 keys) | `getClass().getName() +
".X"` | `Keys.of(class, "X")` | Yes |
| `TrustedChecksumsArtifactResolverPostProcessor` | `class.getName() + ".X"`
| `Keys.of(class, "X")` | Yes |
| `OptionalDependencySelector` (2 keys) | `class.getName() + ".X"` |
`Keys.of(class, "X")` | Yes |
| `NamedLockFactoryAdapterFactoryImpl` | `class.getName() + ".adapter"` |
`Keys.of(class, "adapter")` | Yes |
| `SmartExecutorUtils` | `class.getSimpleName() + "-" + prefix` |
`Keys.of(class, prefix)` | Yes |
| `DefaultRemoteRepositoryManager` | `LoggedMirror` inner class | `Keys.of(4
strings)` | Yes — global (no CL scoping needed) |
| `DefaultUpdateCheckManager` | `new Object()` sentinel | `Keys.of(new
Object())` | Yes — private key |
### API & Compatibility
The PR description's claim of "no API breakage in public surface of api,
spi, util modules" is **verified as correct**:
- `Keys` is a **new** class in `maven-resolver-api` — purely additive
- No method signatures in api/spi/util change
- No public classes removed
In `maven-resolver-impl` (internal, not public API):
- `OptionalDependencySelector.IGNORED_KEYS` / `UNSELECTED_KEYS` change from
`String` to `Object` — source-incompatible for any code referencing these as
`String`, but these fields were added very recently (#1899) and are in
`internal.impl`
- `DefaultUpdateCheckManager.isAlreadyUpdated` / `setUpdated` parameter type
tightens from `Object` to `String` — package-private methods, no external impact
- `LoggedMirror` inner class removed — was `private`, no external impact
### Test Coverage
`KeysTest.java` covers the three key flavors well:
- `globalKeys()` — string-only keys, equality and inequality
- `classLoaderAwareKeys()` — two ClassLoaders loading the same class,
verifies distinct keys
- `privateKeys()` — Object-identity keys
- `subKey()` — key expansion/concatenation
- Error cases: `noElement()`, `nullVararg()`
**Gaps:**
- No test for `null` as a key element (e.g., `Keys.of("a", null, "b")`) —
currently this silently creates a key with a null element. Worth either
rejecting or documenting.
- No test for mixed-type key elements (e.g., `Keys.of("string", 42,
SomeClass.class)`) — would increase confidence in the
`Arrays.hashCode`/`Arrays.equals` delegation
- No test for `toString()` output format (useful for debugging scenarios the
Javadoc mentions)
- The `classLoaderAwareKeys` test uses `Paths.get("target/classes")` — this
works with Maven Surefire's working directory convention but could be fragile
in IDE-based test runners
- No integration test that would catch a ClassCastException regression in
the actual transport layer. Understandable given the complexity of setting up
multi-ClassLoader Maven-like scenarios, but the gap is worth noting.
### Minor Notes
- The `DefaultUpdateCheckManager.SESSION_CHECKS` comment `// instance bound
private key` is helpful — consider adding similar comments at other sites where
the key scope choice is non-obvious
- `GlobalState.java` still has its own `CompoundKey` inner class (used for
`userTokens`/`expectContinues`) — this is correct, as those keys serve a
different purpose than the session cache key
### Verdict
The fix is correct and well-designed. The `Keys` abstraction is a clean
improvement over the scattered key patterns, and the ClassLoader-scoping
mechanism is sound. The migration is comprehensive and each site uses the
appropriate key scope.
The findings above are all minor — the most actionable ones are the Javadoc
typos and the optional defensive copy. The copy-paste bug fix in
`RemoteRepositoryFilterSourceSupport` is a nice bonus find.
Overall: this looks good to merge, pending the minor fixes above.
---
_In-depth review by Claude Code on behalf of @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]
