gnodet-bot commented on code in PR #13471:
URL: https://github.com/apache/maven/pull/13471#discussion_r4259980026
##########
impl/maven-di/src/main/java/org/apache/maven/di/Injector.java:
##########
@@ -157,7 +166,6 @@ 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
*/
@Nonnull
<T> T getInstance(@Nonnull Class<T> key);
Review Comment:
⚠️ **Missing `@throws` after removing `@throws IllegalStateException`:**
Both `getInstance` overloads now have no checked-exception documentation at
all, but this PR introduces a new failure mode — `DIException` (unchecked) when
bindings are ambiguous. Callers reading only the Javadoc have no indication the
method can throw. Add `@throws DIException if no binding is found or bindings
are ambiguous` to both overloads.
```suggestion
* @param key the class representing the type to retrieve
* @return an instance of the requested type
* @throws NullPointerException if key is null
* @throws DIException if no binding is found for the key, or if
bindings are ambiguous
*/
@Nonnull
<T> T getInstance(@Nonnull Class<T> key);
```
##########
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) {
+ if (!Objects.equals(first.getOriginalKey(), second.getOriginalKey())
+ || first.getPriority() != second.getPriority()
+ || !Objects.equals(first.getScope(), second.getScope())) {
+ return false;
+ }
+ Object firstIdentity = first.getRegistrationIdentity();
+ Object secondIdentity = second.getRegistrationIdentity();
+ if (firstIdentity != first && secondIdentity != second) {
+ return firstIdentity.equals(secondIdentity);
Review Comment:
💡 A brief comment explaining the asymmetric guard would help future readers.
The two-sided `!= first && != second` condition is false when only one binding
has a custom identity (e.g. one is a raw constructor, the other is an
initialized wrapper): those fall through to the `instanceof`/reference checks,
which is intentional. Without a comment, this looks like it could be a bug (why
not `|| (!= first || != second)`).
```suggestion
Object firstIdentity = first.getRegistrationIdentity();
Object secondIdentity = second.getRegistrationIdentity();
// Both sides have explicit identities: use equals-based comparison
(e.g. two initialized
// bindings wrapping the same source class). If only one side has an
explicit identity,
// fall through to instanceof/reference checks — they represent
distinct providers.
if (firstIdentity != first && secondIdentity != second) {
return firstIdentity.equals(secondIdentity);
```
--
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]
