-
Notifications
You must be signed in to change notification settings - Fork 466
Allow ChangeDependencyGroupIdAndArtifactId to update in a chainable way keeping the ScanningRecipe in place #5902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ay keeping the ScanningRecipe in place
changedDependency = changedDependency.withVersion(resolvedNewVersion); | ||
} catch (MavenDownloadingException e) { | ||
return e.warn(tag); | ||
throw new RuntimeException("Failed to resolve version for " + changedDependency, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change -> I adapted this as the e.warn modifies the lst and then I had tests fail with a warning indicating that scanners cannot modify the lst
Optional.ofNullable(newArtifactId).orElse(oldArtifactId), | ||
newVersion, versionPattern).getVisitor()); | ||
} | ||
ctx.pollMessage(CHANGED_DEPENDENCIES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is polling the message to clear the context necessary? Is there a better way to clear this message than to run it in every visitor? We can safely remove it after the all-recipe-scanning phase.
Xml.Tag t = (Xml.Tag) super.visitTag(tag, ctx); | ||
boolean isOldDependencyTag = isDependencyTag(oldGroupId, oldArtifactId); | ||
|
||
Map<GroupArtifactVersion, GroupArtifactVersion> changedDependencies = ctx.getMessage(CHANGED_DEPENDENCIES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the context not shared between visiting different POM files? What I was meaning was that if we have a record of a dependency having been changed, we may need distinguish that between different POM files in the same repo so that we don't mistakenly skip changing a dependency in a sibling module's PO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to see how that is different from before.
the accumulator is also shared between different POM files.
I assumed that if this recipe runs, it will bump on all impacted modules making the same changes there. Or am I misunderstanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jenson3210 I think what I was thinking was that getInitialValue(..)
was called once when the recipe was invoked, whereas the visitor was instantiated per file being analyzed, and so if you were checking whether you needed to make a change based on the presence of the message for a particular dependency then it might skip changing when it actually needed to do so still in the current file, but I realize now I had misread the checks, and that they should still be safe, other than what you mentioned in the comment below.
Map<GroupArtifactVersion, GroupArtifactVersion> changedDependencies = ctx.getMessage(CHANGED_DEPENDENCIES); | ||
|
||
GroupArtifactVersion oldDependency = new GroupArtifactVersion(oldGroupId, oldArtifactId, null); | ||
for (Map.Entry<GroupArtifactVersion, GroupArtifactVersion> entry : changedDependencies.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steve-aom-elliott Your comment makes me realize that there might be multiple bumps earlier on so that I will have to change the loop logic still a bit. (luckily the revert gives us this time)
Fix cross-recipe state tracking for sequential dependency transformations
Summary
This PR enhances the
ChangeDependencyGroupIdAndArtifactId
recipe to correctly handle sequential transformations of the same dependency within a single recipe run. The fix uses theExecutionContext
to pass state between different recipe scanning phases, enabling later recipe executions to detect when a dependency will already have been modified by an earlier recipe in the chain by the time this visitor receives it.Problem
When multiple
ChangeDependencyGroupIdAndArtifactId
recipes are chained together in a single recipe run (e.g., transforming A→B then B→C), the second recipe would fail to find and transform the dependency because it was still looking for the original coordinates rather than the updated ones from the first transformation.Solution
The implementation now leverages the
ExecutionContext
to store transformation state between recipe scanning phases:ExecutionContext
Changes
ChangeDependencyGroupIdAndArtifactId.java
ExecutionContext
state management to track transformations across recipe executionsChangeDependencyGroupIdAndArtifactIdTest.java
canChangeSameDependencyMultipleTimesInASingleRecipeRun()
that validates a sequential transformation of bouncycastle dependencies:bcprov-jdk15on
→bcprov-jdk15to18
→bcprov-jdk18on
Technical Details
The
ExecutionContext
serves as a communication channel between recipe phases, storing metadata about planned transformations. This allows the recipe framework to maintain awareness of in-flight changes, ensuring that subsequent recipes in a chain operate on the most current state rather than the original document state. Note that this is not the change that I originally intended / am very happy with, but other attempts to use cursor messages etc have all proved not to be working.Testing
An example
In the example where we run A -> B then B -> C our scanners will run sequentially.
A -> B : the xml code does contain A in the code and calculates new dependency
B-> C: the xml code does not contain B in the code and therefor does not calculate the new dependency.
By introducing the context/Map, the code will now:
A -> B: the xml code does contain A in the code and calculates new dependency + puts A -> B mapping in the context
B -> C: the xml code does not contain B, but the context knows that at the time this recipe will run, B will be present so we calculate the new situation to migrate to based on the expected outcome of the previous run that bumps to B.
This logic is "safe" to bump multiple levels, but might give unexpected results when we have a recipe:
where A and B are present in the same POM as the new dependency will already be present and this accumulator will also not detect this.
As this change is already quite big, I believe a separate PR for this behavior fix is needed (also the logic of
checkIfNewDependencyPresents
needs a fix in that one as it won't work for latest.patch etc...)