Repository navigation
fix: implement Node.getRepository() via local repository manager lookup - #13159
Merged
gnodet merged 3 commits intoSep 17, 2026
Merged
Conversation
* fix: implement Node.getRepository() via local repository manager lookup * Address review: improve Javadoc summary and cache repos local variable
gnodet-bot
reviewed
Sep 16, 2026
gnodet-bot
reviewed
Sep 17, 2026
gnodet-bot
left a comment
There was a problem hiding this comment.
Both findings from the previous review are addressed — null guard via Optional.ofNullable(result) is correct, and the tests now use ArgumentCaptor to validate request construction. Two observations before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
- Add cachedRepository field to DefaultNode for lazy memoization: avoids repeated disk I/O (LRM .repositories file reads) on multi-call scenarios - Switch to Optional.ofNullable(result).map(...) chain — also handles non-standard LRM implementations that return null from find() - Extend Node.getRepository() Javadoc @return to mention that LRMs which do not track artifact origins (e.g. SimpleLocalRepositoryManager) always return empty - Add testGetRepositoryReturnsEmptyWhenLrmReturnsNull and testGetRepositoryIsCached tests
gnodet-bot
approved these changes
Sep 17, 2026
gnodet-bot
left a comment
There was a problem hiding this comment.
All findings from the previous two rounds are addressed.
- ✅
Optional.ofNullable(result)guards against nullfind()return — plus a dedicated test (testGetRepositoryReturnsEmptyWhenLrmReturnsNull) - ✅
ArgumentCaptor<LocalArtifactRequest>in both applicable tests — artifact and repo list are now verified - ✅
Node.java@returnJavadoc extended to document theSimpleLocalRepositoryManagerlimitation - ✅
cachedRepositoryfield added — LRM disk lookup is now deferred and memoized;testGetRepositoryIsCachedconfirms exactly onefind()call across multiple invocations
The implementation is correct: lrm.find() is the right mechanism, all guard conditions are in place, and the six tests cover every branch. Notably the null guard on result makes the backport marginally more defensive than the current master implementation.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet
added a commit
that referenced
this pull request
Sep 17, 2026
…ory() (#13162) Forward-port of hardening applied to the maven-4.0.x backport (PR #13159): - Use Optional.ofNullable(result).map(...) chain instead of result.getRepository() directly, protecting against third-party LRM implementations returning null - Strengthen tests with ArgumentCaptor to verify LocalArtifactRequest content - Add testGetRepositoryReturnsEmptyWhenLrmReturnsNull to cover the null-result path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #13153 to
maven-4.0.x.Cherry-pick of ada2740, applied cleanly with no conflicts.