gnodet-bot commented on code in PR #13471:
URL: https://github.com/apache/maven/pull/13471#discussion_r4258147636
##########
impl/maven-di/src/main/java/org/apache/maven/di/Injector.java:
##########
@@ -157,7 +167,8 @@ static Injector create() {
* @param key the class representing the type to retrieve
* @return an instance of the requested type
* @throws NullPointerException if key is null
- * @throws IllegalStateException if the type cannot be provided
+ * @throws DIException if dependency resolution fails; the default
injector also rejects
+ * equally preferred bindings
Review Comment:
💡 **API layer concern:** `@throws DIException` references a type from
`org.apache.maven.di.impl`, which is only exported to 5 named modules in
`module-info.java`. JPMS consumers outside that list cannot reference or catch
`DIException` directly. The Javadoc `{@link DIException}` will resolve for
internal modules but not for external ones.
Options (in order of preference):
1. Move `DIException` to `org.apache.maven.di` (the publicly-exported
package), or
2. Use `{@code DIException}` (no link) with a note that it is an
implementation type, or
3. Keep as-is with a note: _"thrown as
`org.apache.maven.di.impl.DIException`, which is accessible to consumers listed
in `module-info.java`"_.
Note: `InjectorImpl` was already imported in this file before this PR, so
this follows established precedent — but expanding it to `@throws` Javadoc
makes it more visible as a contract issue. The same concern applies to the
`getInstance(Key)` overload below.
##########
impl/maven-di/src/test/java/org/apache/maven/di/impl/InjectorImplTest.java:
##########
@@ -158,10 +158,18 @@ static class MyMojo {
@Test
public void bindInterfacesTest() {
- Injector injector =
Injector.create().bindImplicit(BindInterfaces.class);
+ Injector injector =
Injector.create().bindImplicit(BindInterfaces.ClassImpl.class);
Review Comment:
⚠️ **Undocumented behavioral change:** This test was silently updated from
`bindImplicit(BindInterfaces.class)` to
`bindImplicit(BindInterfaces.ClassImpl.class)`. Before the fix, binding the
outer class would register both `@Named("")` inner implementations (`ClassImpl`
and `TypedClassImpl`) as unqualified for `TestInterface<String>` — the injector
non-deterministically picked one. After the fix, this correctly throws
`DIException` due to ambiguity (two default bindings for the same type).
This is the right behavior, but it's a breaking change for callers of
`bindImplicit(OuterClass.class)` when the outer class contains multiple
`@Named` inner implementations of the same interface. Please document this in
the PR description so release notes and reviewers capture it.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/standalone/ApiRunnerSettingsTest.java:
##########
@@ -37,9 +45,56 @@
*/
class ApiRunnerSettingsTest {
+ private static final String OTHER_SESSION_NAME = "other";
+
@TempDir
Path tempDir;
+ @Test
+ void testNamedSessionProviderDoesNotReplaceStandaloneSession() throws
IOException {
+ Path installation =
Files.createDirectories(tempDir.resolve("installation"));
+ Path localRepository = tempDir.resolve("requested-repository");
+ String oldHome = System.getProperty("user.home");
+ String oldMavenHome = System.getProperty("maven.home");
+ try {
+ System.setProperty("user.home", tempDir.toString());
+ System.setProperty("maven.home", installation.toString());
+ Session session = ApiRunner.createSession(
+ injector -> {
+ injector.bindImplicit(OtherSessionProvider.class);
+ InjectorImpl impl = (InjectorImpl) injector;
+ Set<Binding<?>> ordered =
+ new
LinkedHashSet<>(impl.getBindings().get(Key.of(Session.class,
OTHER_SESSION_NAME)));
+
ordered.addAll(impl.getBindings().get(Key.of(Session.class)));
Review Comment:
💡 A brief comment explaining why the binding reordering is necessary would
help maintainers. The `LinkedHashSet` dance forces the named provider before
the default to reproduce the non-deterministic HashSet iteration order that
triggered the original bug — without it the test could pass even without the
fix if the default provider happened to sort first. Consider:
```suggestion
InjectorImpl impl = (InjectorImpl) injector;
// Force named binding before default to reproduce
worst-case HashSet
// iteration order where the named Session wins over
the default one
// (the bug: ApiRunner would then lose the requested
local repository).
Set<Binding<?>> ordered =
new
LinkedHashSet<>(impl.getBindings().get(Key.of(Session.class,
OTHER_SESSION_NAME)));
ordered.addAll(impl.getBindings().get(Key.of(Session.class)));
impl.getBindings().put(Key.of(Session.class),
ordered);
```
##########
impl/maven-di/src/main/java/org/apache/maven/di/impl/InjectorImpl.java:
##########
@@ -188,10 +189,32 @@ private Injector doBind(Key<?> key, Binding<?> binding) {
protected <U> Injector bind(Key<U> key, Binding<U> b) {
Set<Binding<?>> bindingSet = bindings.computeIfAbsent(key, $ -> new
HashSet<>());
- bindingSet.add(b);
+ if (bindingSet.stream().noneMatch(existing -> isSameBinding(existing,
b))) {
+ bindingSet.add(b);
+ }
return this;
}
+ private static boolean isSameBinding(Binding<?> first, Binding<?> second) {
Review Comment:
💡 Minor: a brief comment on the asymmetric case would improve readability.
When only one of the two bindings has a custom `registrationIdentity`, this
condition is false and the logic falls through to the `instanceof`/reference
checks — which is correct (raw binding and its initialized version are distinct
providers, validated by `aCustomInitializerIsDistinctFromItsRawBinding`). But
without a comment, the intent of the two-sided `!= first && != second` guard is
not obvious to future readers.
--
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]
