Fix ApacheTransport auth cache CCEx

[cstamas]

Started as proposed, to make session-wide auth cache string key classloader aware. Then realized, we have "classloader aware" class, that we make into string, with tricks, to make it "classloader aware", and this sounds awkward. Hence, Keys helper class.

This PR also revealed a bug, hidden deep, where (probably a copy pasted) code snippet used wrong key, see RemoteRepositoryFilterSourceSupport.

---

Back story: originally Resolver used String as keys that as we know, are perfect candidates for keys. But, if we stick to Maven, same component may end up loaded twice in Maven, and if key value contains something that is component specific (like the issue mentioned Apache transport value is Apache transport specific class), this will cause inevitably ClassCastEx if one instance puts something into map, and other one from the other classloader discovers it (and tries to cast it). In Maven, the resolver api, spi, util, impl and basic connector modules cannot be replaced (is prevented by Maven; circumvention is relocation or uber jar, changing the GAV), but other resolver modules can be loaded up multiple times, see https://github.com/apache/maven/blob/4106910f94e92a0967c61c62b4dbbcd6294c97a0/maven-core/src/main/resources/META-INF/maven/extension.xml#L185-L189

Hence the Keys, where user still has to decide how "scoped" the key should be, if using Strings as key element, it will be global, and that is fine for keys mapped to values that are usable across class loaders as well (ie. value is consisted of java.base classes only). If we want class loader awareness (like in the issue, for Apache transport we do), then all user should do, is to use class loader specific class as one key element, and it will distinguish it from duplicate class loaded by another class loader. Finally, as update check manager does, "instance bound" keys are still possible too.

This change, while may look "radical" or even breaking, is in fact limited and does not cause API breakage in public surface of modules: api, spi and util.

Fixes #1919

[gnodet]

Minor defensive-programming note: the varargs array is stored directly without copying:

Suggested change

	this.keys = keys;
	private Key(Object[] keys) {
	this.keys = keys.clone();
	this.hashCode = Arrays.hashCode(this.keys);
	}

All current callers use varargs syntax (which creates a fresh array per call), so this is safe in practice today. But since Keys is a public API class in maven-resolver-api, a future caller could do:

Object[] arr = { "a", "b" };
Object key = Keys.of(arr);
arr[0] = "changed"; // silently corrupts the key

A clone() in the constructor would close that off. The cost is negligible since key arrays are tiny. Up to you whether you want to be defensive here — it's the kind of thing that burns someone exactly once, years from now.

[gnodet]

Very minor Javadoc nits:

1. <p/> (lines 31, 33, 36, 39) — self-closing <p/> is a no-op in HTML. Standard Javadoc convention is <p> (block-level paragraph separator). This appears to be specific to this new file — I didn't find <p/> elsewhere in the maven-resolver-api sources.

2. Line 33: "keys our of object instances" → "keys out of object instances"

3. Line 39: "use od {@link String}" → "use of {@link String}"

[gnodet]

The subkey expansion only happens when a Key appears as the first element. If a Key appears at position > 0, it's treated as a regular element (compared by its own equals/hashCode). This is probably intentional for the "base key + suffix" use case, but worth mentioning in the Javadoc since it could surprise someone doing:

Object base = Keys.of("prefix");
Object k1 = Keys.of("other", base, "suffix");  // base is NOT expanded
Object k2 = Keys.of(base, "other", "suffix");  // base IS expanded → ["prefix", "other", "suffix"]

The current Javadoc says "If the first instance of key elements is an object instance..." — I'd suggest making it slightly more explicit that expansion is first-position-only.

[gnodet]

Good catch. Confirmed via git blame: this was introduced in commit 79b6d5f2 (Repository Key Function SPI, #1679) where the repositoryKey() method was added. The FileTrustedChecksumsSourceSupport.class reference was clearly copy-pasted from that class's own repositoryKey() method and never updated to RemoteRepositoryFilterSourceSupport.class.

The practical effect was that RemoteRepositoryFilterSourceSupport subclasses and FileTrustedChecksumsSourceSupport shared the same repository key function cache entry in DefaultRepositoryKeyFunctionFactory. In practice this likely didn't cause visible bugs since both use the same default function, but it's semantically wrong and could mask issues if configuration diverged.

[gnodet]

Pre-existing issue (not introduced by this PR), but worth noting: using `session.hashCode()` as a key element means that two different derived sessions with a hashCode collision would share the same filter instance. `Integer.hashCode()` is just the int value itself, so this relies on `RepositorySystemSession.hashCode()` being sufficiently unique. This was the same behavior before the PR (`INSTANCE_KEY + "." + session.hashCode()`), so no regression here.

[gnodet]

Note: this changes two `public static final` fields from type `String` to type `Object`. While `OptionalDependencySelector` is in the `internal.impl` package (so the PR description's claim of "no API breakage in api, spi, util" is accurate), these fields are `public` and the class is `public final`.

If any external code (e.g., a custom `DependencySelector` implementation or a Maven plugin) references `OptionalDependencySelector.IGNORED_KEYS` and assigns it to a `String` variable, that code will fail to compile. Worth considering whether to keep the `String` type here (since these are only used as `SessionData` keys, and `SessionData.get()` accepts `Object`).

That said, the fields were only added in commit `0447a67c` (very recently), so the blast radius is likely near zero.

[cstamas]

Yes, these two keys are new, and are not yet present in any released code.

[gnodet]

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 Repository Key Function SPI #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 (DependencyCollectionChecker: new session member #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