Skip to content

[MRESOLVER-43] Enhancements to the public API to add support for trac…#17

Open
ChristianSchulte wants to merge 1 commit into
apache:masterfrom
ChristianSchulte:MRESOLVER-43
Open

[MRESOLVER-43] Enhancements to the public API to add support for trac…#17
ChristianSchulte wants to merge 1 commit into
apache:masterfrom
ChristianSchulte:MRESOLVER-43

Conversation

@ChristianSchulte

Copy link
Copy Markdown
Contributor

…king various informational declaration source hints.

…king various informational declaration source hints.
public DependencyManagement()
{
// enables default constructor
super();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the super if we don't extend anything?

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-Assisted Review — PR #17 ([MRESOLVER-43] Enhancements to the public API)

This PR proposes enhancements to the public API for dependency management, adding "source hint" tracking to understand where managed dependency attributes (versions, scopes, optionals, local paths, exclusions) originate. The concept of tracking management provenance is valuable for tooling and debugging.

However, this PR faces significant challenges:

Verified findings

1. Massive staleness & merge conflicts (8+ years)

Created March 2, 2018 — over 8 years ago. The merge state is DIRTY/CONFLICTING. The version gap spans 1.1.2-SNAPSHOT → current 2.0.21-SNAPSHOT. Key files have been moved or restructured — for example, DefaultDependencyCollector.java was moved from org.eclipse.aether.internal.impl to org.eclipse.aether.internal.impl.collect in commit 142469e (MRESOLVER-35). The codebase has undergone massive structural changes making this PR unmergeable as-is.

2. Copy-paste bug in getPropertiesManagementSourceHint()

The method getPropertiesManagementSourceHint() checks the wrong bitmask and reads the wrong data key:

// Current (incorrect — copied from getExclusionsManagementSourceHint):
if ((node.getManagedBits() & DependencyNode.MANAGED_EXCLUSIONS) == 0)
    ...node.getData().get(NODE_DATA_EXCLUSIONS_MANAGEMENT_SOURCE_HINT)

// Should be:
if ((node.getManagedBits() & DependencyNode.MANAGED_PROPERTIES) == 0)
    ...node.getData().get(NODE_DATA_PROPERTIES_MANAGEMENT_SOURCE_HINT)

3. Typo in constant value

NODE_DATA_EXCLUSIONS_MANAGEMENT_SOURCE_HINT has the string value "exlusions.management.source.hint" — missing the letter 'c' in "exclusions" (should be "exclusions.management.source.hint").

4. equals()/hashCode() not updated for new fields

Five new source hint map fields were added to ClassicDependencyManager (managedVersionsSourceHints, managedScopesSourceHints, managedOptionalsSourceHints, managedLocalPathsSourceHints, managedExclusionsSourceHints), but equals() only compares the original four maps. Two instances with different source hints would be considered equal, violating the equals/hashCode contract.

Suggestion

The concept of tracking management source provenance is a valuable one. If there's still interest in this capability, it would likely be more productive to open a fresh issue and implement it against the modern 2.x codebase, as the structural changes over 8 years make this PR unmergeable without a complete rewrite.

🤖 This review was generated with AI assistance (reviewer + independent verifier pattern). Findings were independently verified before posting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants