[MNG-8547] Expose repository events through the Maven API - #13011
[MNG-8547] Expose repository events through the Maven API#13011goutamadwant wants to merge 9 commits into
Conversation
Add Maven API repository event and listener types and bridge all Resolver repository callbacks without exposing Resolver types. Keep registration scoped to the underlying Resolver session and cover event mapping, listener isolation, and derived-session behavior. Signed-off-by: goutamadwant <workwithgoutam@gmail.com>
gnodet
left a comment
There was a problem hiding this comment.
Well-implemented feature exposing Maven Resolver repository events through the public Maven API. The event model, listener lifecycle, and bridge from Resolver to Maven are clean. Comprehensive test coverage (19 event types, isolation, concurrency, lambda ambiguity guard). One low-severity finding noted below.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
|
||
| @Override | ||
| public Collection<RepositoryListener> getRepositoryListeners() { | ||
| return null; |
There was a problem hiding this comment.
Low: getRepositoryListeners() returns null, but the Session interface (added in this same PR) declares it @Nonnull with Javadoc "never null". The real implementation in AbstractSession correctly returns an unmodifiable collection.
Consider returning List.of() or Collections.emptyList() here to honor the contract:
| return null; | |
| return List.of(); |
(The pre-existing getListeners() above has the same null-return anti-pattern, but no need to extend it to new methods.)
Return an empty immutable collection from SessionStub instead of null, matching the non-null Session API contract. Signed-off-by: goutamadwant <workwithgoutam@gmail.com>
gnodet
left a comment
There was a problem hiding this comment.
Solid implementation. The bridge architecture (Resolver → Maven API) is clean, thread safety is correct (CopyOnWriteArrayList in session data, listener isolation via catch-and-log), and test coverage is comprehensive (all 19 event types, failure isolation, concurrency, lambda ambiguity guard, derived-session sharing).
The previous review finding (SessionStub returning null for getRepositoryListeners()) has been addressed.
Two documentation gaps in the public API:
-
RepositoryListener— all 19 callback methods lack Javadoc. This is a@ConsumerAPI that plugin developers will implement. Each method should at minimum describe when it fires (e.g., "Called when an artifact resolution operation has started" vs "Called after an artifact has been resolved from a repository, whether successfully or with a failure"). Without this, consumers must reverse-engineer semantics from the Resolver docs or the enum names. -
RepositoryMetadata— all 7 getter methods lack Javadoc. At minimum,getType()(which returns the metadata filename, not a MIME type) andgetNature()need clarification for users unfamiliar with Resolver internals.
RepositoryEventType enum constants would also benefit from brief descriptions, but the names are reasonably self-documenting so this is lower priority.
These are documentation-only issues — the implementation itself is correct.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| public interface RepositoryListener { | ||
|
|
||
| default void artifactDescriptorInvalid(@Nonnull RepositoryEvent event) {} | ||
|
|
There was a problem hiding this comment.
All 19 callback methods lack Javadoc. For a @Consumer public API, each method should describe when it fires and what the event contains. For example:
/**
* Called when an artifact descriptor could not be parsed.
*
* @param event the event details; {@link RepositoryEvent#getArtifact()} identifies the artifact,
* {@link RepositoryEvent#getException()} describes the parse failure
*/
default void artifactDescriptorInvalid(@Nonnull RepositoryEvent event) {}Without this, consumers must reverse-engineer semantics from Resolver documentation.
There was a problem hiding this comment.
@gnodet Addressed in cac2b6f1f5. Added Javadocs for all 19 callbacks describing when each event fires and the relevant event details.
| @Nonnull | ||
| String getArtifactId(); | ||
|
|
||
| @Nonnull |
There was a problem hiding this comment.
All getter methods lack Javadoc. At minimum, getType() needs clarification — it returns the metadata filename (e.g., "maven-metadata.xml"), not a MIME type or content descriptor. getNature() should explain the RELEASE/SNAPSHOT/RELEASE_OR_SNAPSHOT semantics.
There was a problem hiding this comment.
Added Javadocs for all RepositoryMetadata accessors, including clarification that getType() returns the metadata filename and getNature() describes release/snapshot applicability.
Describe when repository listener callbacks fire and clarify the values exposed by repository metadata.
|
@gnodet addressed all review comments. let me know. thanks! |
API Design Suggestion: Event/Listener HierarchyThe current PR introduces
Proposed hierarchy for 4.1.0: introduce Event hierarchy@Experimental @Immutable
public interface Event {
@Nonnull Session session();
}
@Experimental @Immutable
public interface ExecutionEvent extends Event {
@Nonnull ExecutionEventType type();
@Nonnull Optional<Project> project();
@Nonnull Optional<MojoExecution> mojoExecution();
@Nonnull Optional<Exception> exception();
}
@Experimental @Immutable
public interface RepositoryEvent extends Event {
@Nonnull RepositoryEventType type();
@Nonnull Optional<Artifact> artifact();
@Nonnull Optional<RepositoryMetadata> metadata();
@Nonnull Optional<Path> path();
@Nonnull Optional<Repository> repository();
@Nonnull Optional<Exception> exception();
@Nonnull List<Exception> exceptions();
@Nonnull Optional<RequestTrace> trace();
}Listener hierarchy@Experimental @Consumer
public interface Listener {
/** @deprecated Implement ExecutionListener or RepositoryListener instead. */
@Deprecated
default void onEvent(@Nonnull Event event) {}
}
@Experimental @Consumer
public interface ExecutionListener extends Listener {
default void sessionStarted(@Nonnull ExecutionEvent event) {}
default void sessionEnded(@Nonnull ExecutionEvent event) {}
default void projectDiscoveryStarted(@Nonnull ExecutionEvent event) {}
default void projectStarted(@Nonnull ExecutionEvent event) {}
default void projectSucceeded(@Nonnull ExecutionEvent event) {}
default void projectFailed(@Nonnull ExecutionEvent event) {}
default void projectSkipped(@Nonnull ExecutionEvent event) {}
default void mojoStarted(@Nonnull ExecutionEvent event) {}
default void mojoSucceeded(@Nonnull ExecutionEvent event) {}
default void mojoFailed(@Nonnull ExecutionEvent event) {}
default void mojoSkipped(@Nonnull ExecutionEvent event) {}
default void forkStarted(@Nonnull ExecutionEvent event) {}
default void forkSucceeded(@Nonnull ExecutionEvent event) {}
default void forkFailed(@Nonnull ExecutionEvent event) {}
default void forkedProjectStarted(@Nonnull ExecutionEvent event) {}
default void forkedProjectSucceeded(@Nonnull ExecutionEvent event) {}
default void forkedProjectFailed(@Nonnull ExecutionEvent event) {}
}
@Experimental @Consumer
public interface RepositoryListener extends Listener {
default void artifactDescriptorInvalid(@Nonnull RepositoryEvent event) {}
default void artifactDescriptorMissing(@Nonnull RepositoryEvent event) {}
default void metadataInvalid(@Nonnull RepositoryEvent event) {}
default void artifactResolving(@Nonnull RepositoryEvent event) {}
default void artifactResolved(@Nonnull RepositoryEvent event) {}
// ... etc (19 typed callbacks, as in this PR)
}Session impactSingle registration point — no overloaded methods needed: // Session keeps ONE set of listener methods for both types:
void registerListener(@Nonnull Listener listener);
void unregisterListener(@Nonnull Listener listener);
Collection<Listener> getListeners();The dispatcher routes via Benefits
|
Route execution and repository events through one shared listener registry. Add typed execution callbacks and noun-style event accessors while preserving existing execution Event getters and Listener lambdas. Cover all event callbacks, combined listeners, derived-session registration, concurrent updates, and failure isolation.
@gnodet Updated to one listener registration path with typed execution and repository callbacks, including listeners implementing both interfaces. Added noun-style event accessors and ExecutionEventType. I kept Listener functional and retained the existing Event getters: making onEvent a default would break existing lambdas, and replacing Event with a marker would remove its current methods. SessionEvent provides the shared event base, while TypedListener supplies the common default without conflicting inherited methods. The PR description includes the full integration results and successful redirect-test retry. |
Make Event the base interface for both ExecutionEvent and RepositoryEvent, removing the SessionEvent intermediate. Drop TypedListener by moving the dispatch logic into ExecutionListener.onEvent() and RepositoryListener.onEvent() as default methods. This makes the dispatch self-contained in the API interfaces and simplifies EventSpyImpl to a single listener.onEvent(event) call. Changes: - Event: base interface with session() + deprecated getX() compat defaults - ExecutionEvent: extends Event (unchanged noun-style accessors) - RepositoryEvent: extends Event (was SessionEvent) - ExecutionListener: extends Listener, default onEvent() dispatches to typed callbacks - RepositoryListener: extends Listener, default onEvent() dispatches to typed callbacks - SessionEvent: deleted (redundant, Event is the base) - TypedListener: deleted (each sub-interface provides its own default onEvent()) - EventSpyImpl: simplified to just call listener.onEvent(event) - Listener: unchanged (@FunctionalInterface, abstract onEvent(Event))
gnodet
left a comment
There was a problem hiding this comment.
All three findings from the previous review (2026-09-03) have been addressed:
- SessionStub
getRepositoryListeners()null return → fixed (returnsList.of()). RepositoryListener— 19 callback methods now have Javadoc.RepositoryMetadata—getType()andgetNature()now have clarifying Javadoc.
The new commits (unified listener hierarchy, ExecutionEvent/ExecutionListener/ExecutionEventType) are a clean improvement. Two documentation issues in the new types:
This review was generated by an AI agent, Hermès on behalf of @gnodet.
… redundant session() in RepositoryEvent
gnodet
left a comment
There was a problem hiding this comment.
Both findings from the 2026-09-07 review are addressed:
getType()Javadoc missing@throws→@throws UnsupportedOperationException if this event is not an ExecutionEventadded in740c915e04. ✅- Redundant
session()re-declaration inRepositoryEvent→ removed in740c915e04. ✅ RepositoryMetadatanoun-style accessors → renamed fromgetXxx()toxxx()inb963221926. ✅
Two documentation issues remain in the new code:
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| /** | ||
| * Gets the type of the event. | ||
| * | ||
| * @return the type of the event, never {@code null} |
There was a problem hiding this comment.
@throws UnsupportedOperationException was correctly added in 740c915e04, but line 64 still says never {@code null} — which is misleading: for any Event that is not an ExecutionEvent, the method throws rather than returning. The @return description and @Nonnull both imply the method always produces a value, contradicting the @throws.
Change the @return description to scope the guarantee:
| * @return the type of the event, never {@code null} | |
| * @return the execution event type, never {@code null}; only meaningful when this event is an {@link ExecutionEvent} |
There was a problem hiding this comment.
Fixed in a8fe34a. Changed @return to: "the execution event type, never null; only meaningful when this event is an ExecutionEvent", which is consistent with the @throws and no longer implies the method always produces a value.
| public interface RepositoryEvent extends Event { | ||
|
|
||
| /** | ||
| * Returns the kind of repository operation represented by this event. |
There was a problem hiding this comment.
Low: Javadoc style inconsistency across sibling types. All eight accessors in RepositoryEvent use prose-style /** Returns X */ without a @return tag. Both sibling types — ExecutionEvent and RepositoryMetadata — use the modern /** {@return X} */ single-tag form throughout. Using the same style here keeps the public API consistent and ensures @return appears in generated Javadoc output.
For example, type() should be:
| * Returns the kind of repository operation represented by this event. | |
| /** | |
| * {@return the kind of repository operation represented by this event} | |
| */ |
Apply the same {@return ...} pattern to the remaining seven accessors (artifact(), metadata(), path(), repository(), exception(), exceptions(), trace()).
There was a problem hiding this comment.
Fixed in a8fe34a. All eight accessors in RepositoryEvent now use the /** {\@return ...} */ single-tag form, consistent with ExecutionEvent and RepositoryMetadata.
gnodet
left a comment
There was a problem hiding this comment.
All findings from previous reviews (2026-09-02, 2026-09-03, 2026-09-07, 2026-09-08) have been addressed in the latest merge commit a8fe34a4bb. The PR is in good shape.
Summary of resolved items:
- SessionStub null return →
List.of()✅ - RepositoryListener Javadoc (19 callbacks) ✅
- RepositoryMetadata Javadoc (
type(),nature(), all accessors) ✅ getType()Javadoc@throws+ scoped@return✅- Redundant
session()re-declaration in RepositoryEvent → removed ✅ - RepositoryEvent
{@return}style consistency ✅ - RepositoryMetadata noun-style accessors (
getXxx()→xxx()) ✅
Current state assessment — no blocking issues found.
The implementation is clean and well-structured:
-
Event hierarchy —
Event→ExecutionEvent/RepositoryEventwith sharedsession()base. Backward-compatible deprecated defaults onEventthat delegate properly (getType()throws for non-execution events, others returnOptional.empty()). Clean. -
Listener dispatch —
ExecutionListenerandRepositoryListenereach provide their owndefault onEvent()with switch dispatch to typed callbacks. No need for aTypedListenerintermediary.Listenerstays@FunctionalInterface. Existing lambdas continue to work. This is the right design. -
Registration sharing — Listeners stored in
RepositorySystemSession.getData()keyed byListener.class, so derived sessions (withRemoteRepositories,withContext) share the sameCopyOnWriteArrayList. Thread-safe and consistent. -
Bridge wiring —
MavenRepositoryListener(Resolver → Maven API) correctly chains into the session factory viaChainedRepositoryListener, andApiRunnersets it up for standalone use.EventSpyImplhandles execution events with listener failure isolation. -
Test coverage — comprehensive: all 17 execution and 19 repository event types, combined listeners, concurrent registration, derived-session sharing, failure isolation, lambda ambiguity guard, metadata mapping, immutable collections, and the session factory wiring test.
Two optional observations (not blocking):
-
DefaultEventstill storesEventTypeand converts viaExecutionEventType.valueOf(eventType.name())intype()— this works because the enum constants have matching names, but it's a runtime coupling. Could storeExecutionEventTypedirectly. Minor. -
CombinedListenerpattern requires explicitonEventoverride — a class implementing bothExecutionListenerandRepositoryListenermust writeonEventto call both supers. The tests demonstrate this clearly, but the API Javadoc onListenercould mention it. Nice-to-have.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Explain the onEvent override required when a listener implements both ExecutionListener and RepositoryListener, with an example delegating to both default methods.
gnodet
left a comment
There was a problem hiding this comment.
New commit (f77c6dbeb9) documents the combined listener dispatch pattern on Listener — the onEvent override required when implementing both ExecutionListener and RepositoryListener. This was the last nice-to-have from the previous review round.
Documentation is clear, correct, and includes a useful code example. No issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Fixes #10677
Description
Expose repository operations through Maven-owned API types without leaking Resolver types into the public API. The bridge covers all 19 repository event types and maps artifacts, metadata, repositories, paths, exceptions, and request traces.
New API types (22 files, +1908/−49):
RepositoryEvent/RepositoryEventType/RepositoryListener— typed repository event family (19 event types: artifact/metadata install, deploy, resolve, download, etc.)ExecutionEvent/ExecutionEventType/ExecutionListener— typed execution event family with noun-style accessorsRepositoryMetadata— immutable Maven API wrapper for repository metadata (noun-style accessors)Event hierarchy:
Eventis the shared base interface withsession()accessorEvent(getType(),getSession(),getProject(),getMojoExecution(),getException()) are preserved for backward compatibility;getType()documents that it throwsUnsupportedOperationExceptionfor non-ExecutionEventinstancesListenerremains@FunctionalInterfacefor backward compatibility; existing lambdas continue to receive execution events unchangedListener registration:
Session.registerListener(Listener)/unregisterListener(Listener)/getListeners()pathExecutionListener,RepositoryListener) receive only their event-family callbacks; a single listener can implement bothImplementation:
DefaultRepositoryEventmaps ResolverRepositoryEventto the Maven API, converting artifacts, metadata, repositories, paths, exceptions, and request tracesMavenRepositoryListenerbridges Resolver repository events to registeredRepositoryListenerinstancesDefaultEventandEventSpyImplupdated for the new hierarchyAbstractSessionupdated for unified listener registrationDefaultRepositorySystemSessionFactorywires the repository listener bridgeTests
EventSpyImplTest(337 lines): all 17 execution callbacks, legacy listener lambdas, noun accessors, combined listeners, repository-only listeners, single delivery without duplicate legacy callback, registration/removal through derived sessions, concurrent registration, immutable collection views, null arguments, listener failure isolationMavenRepositoryListenerTest(347 lines): all 19 repository event types, event-field conversion, metadata mapping with noun-style accessors, request traces, immutable metadata propertiesDefaultRepositorySystemSessionFactoryTest(41 lines): repository listener bridge wiringFollowing this checklist to help us incorporate your contribution quickly and easily:
This pull request addresses one issue without unrelated changes.
The description explains what the pull request does, how, and why.
Each commit has a meaningful subject and body.
Unit tests cover the behavioral changes.
Reactor unit tests and basic verification checks pass.
The complete Core IT suite passes.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004