Skip to content

Commit 61b818e

Browse files
bmuschkobmuschko
authored
Ensure proper because ordering for multiple Maven dependencies (#6352)
1 parent b30b162 commit 61b818e

File tree

3 files changed

+263
-42
lines changed

3 files changed

+263
-42
lines changed

‎rewrite-maven/src/main/java/org/openrewrite/maven/AddManagedDependencyVisitor.java‎

Lines changed: 87 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import lombok.RequiredArgsConstructor;
1919
import org.jspecify.annotations.Nullable;
20+
import org.openrewrite.Cursor;
2021
import org.openrewrite.ExecutionContext;
2122
import org.openrewrite.Tree;
2223
import org.openrewrite.marker.Markers;
@@ -27,9 +28,9 @@
2728
import org.openrewrite.xml.tree.Xml;
2829

2930
import java.util.ArrayList;
31+
import java.util.Comparator;
3032
import java.util.List;
3133
import java.util.concurrent.atomic.AtomicBoolean;
32-
import java.util.stream.IntStream;
3334

3435
import static java.util.Collections.emptyList;
3536

@@ -138,69 +139,113 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
138139
"</dependency>"
139140
);
140141

141-
doAfterVisit(new AddToTagVisitor<>(tag, dependencyTag,
142+
// Add the dependency with optional comment in a single operation to ensure
143+
// the comment stays with its associated dependency regardless of sorting order
144+
doAfterVisit(new AddDependencyWithCommentVisitor(tag, dependencyTag, because,
142145
new InsertDependencyComparator(tag.getContent() == null ? emptyList() : tag.getContent(), dependencyTag)));
143-
144-
// If because is provided, add a visitor to prepend the comment before the dependency
145-
if (because != null) {
146-
doAfterVisit(new AddCommentBeforeDependency(groupId, artifactId, because));
147-
}
148146
}
149-
return super.
150-
151-
visitTag(tag, ctx);
147+
return super.visitTag(tag, ctx);
152148
}
153149
}
154150

155151
@RequiredArgsConstructor
156-
private static class AddCommentBeforeDependency extends MavenIsoVisitor<ExecutionContext> {
157-
private final String groupId;
158-
private final String artifactId;
152+
private static class AddDependencyWithCommentVisitor extends MavenIsoVisitor<ExecutionContext> {
153+
private final Xml.Tag scope;
154+
private final Xml.Tag dependencyTag;
155+
@Nullable
159156
private final String because;
157+
private final Comparator<Content> tagComparator;
160158

161159
@Override
162-
public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
163-
if (MANAGED_DEPENDENCIES_MATCHER.matches(getCursor()) && tag.getContent() != null) {
164-
List<Content> contents = new ArrayList<>(tag.getContent());
165-
166-
return IntStream.range(0, contents.size())
167-
.filter(i -> contents.get(i) instanceof Xml.Tag)
168-
.mapToObj(i -> new IndexedTag(i, (Xml.Tag) contents.get(i)))
169-
.filter(pair -> matchesDependency(pair.tag, groupId, artifactId))
170-
.findFirst()
171-
.map(pair -> addComment(tag, contents, pair))
172-
.orElse(tag);
160+
public Xml.Tag visitTag(Xml.Tag t, ExecutionContext ctx) {
161+
if (scope.isScope(t)) {
162+
t = ensureClosingTag(t, ctx);
163+
Xml.Tag formattedDependencyTag = formatDependencyTag(ctx);
164+
165+
List<Content> content = t.getContent() == null ? new ArrayList<>() : new ArrayList<>(t.getContent());
166+
int insertIndex = findInsertIndex(content);
167+
168+
if (because != null) {
169+
addComment(content, insertIndex, because, formattedDependencyTag.getPrefix());
170+
addDependency(content, insertIndex + 1, formattedDependencyTag);
171+
} else {
172+
addDependency(content, insertIndex, formattedDependencyTag);
173+
}
174+
175+
t = t.withContent(content);
173176
}
174-
return super.visitTag(tag, ctx);
177+
178+
return super.visitTag(t, ctx);
179+
}
180+
181+
/**
182+
* Ensures the tag has a proper closing tag (not self-closing) with a newline prefix.
183+
*/
184+
private Xml.Tag ensureClosingTag(Xml.Tag t, ExecutionContext ctx) {
185+
if (t.getClosing() == null) {
186+
t = t.withClosing(autoFormat(new Xml.Tag.Closing(Tree.randomId(), "\n",
187+
Markers.EMPTY, t.getName(), ""), null, ctx, getCursor()))
188+
.withBeforeTagDelimiterPrefix("");
189+
}
190+
if (!t.getClosing().getPrefix().contains("\n")) {
191+
t = t.withClosing(t.getClosing().withPrefix("\n"));
192+
}
193+
return t;
175194
}
176195

177-
private Xml.Tag addComment(Xml.Tag tag, List<Content> contents, IndexedTag pair) {
178-
// Extract the prefix (should be like "\n ")
179-
String prefix = pair.tag.getPrefix();
196+
/**
197+
* Formats the dependency tag with proper newline prefix and indentation.
198+
*/
199+
private Xml.Tag formatDependencyTag(ExecutionContext ctx) {
200+
Xml.Tag formatted = dependencyTag;
201+
if (!formatted.getPrefix().contains("\n")) {
202+
formatted = formatted.withPrefix("\n");
203+
}
204+
return autoFormat(formatted, null, ctx, getCursor());
205+
}
180206

181-
// Create comment with the same prefix as the dependency tag
182-
// Add spaces around the comment text for proper XML formatting
207+
/**
208+
* Find the insertion position by comparing only with dependency tags.
209+
* This ensures we don't insert between a comment and its associated dependency.
210+
*/
211+
private int findInsertIndex(List<Content> content) {
212+
for (int i = 0; i < content.size(); i++) {
213+
Content item = content.get(i);
214+
// Only compare with dependency tags, skip comments
215+
if (item instanceof Xml.Tag && tagComparator.compare(item, dependencyTag) > 0) {
216+
// Found a dependency that should come after ours.
217+
// We want to insert before this dependency AND any preceding comments.
218+
return findInsertPositionBeforePrecedingComments(content, i);
219+
}
220+
}
221+
return content.size();
222+
}
223+
224+
private void addComment(List<Content> content, int insertIndex, String because, String prefix) {
183225
Xml.Comment comment = new Xml.Comment(
184226
Tree.randomId(),
185227
prefix,
186228
Markers.EMPTY,
187229
" " + because + " "
188230
);
231+
content.add(insertIndex, comment);
232+
}
189233

190-
// Insert comment before the dependency
191-
contents.add(pair.index, comment);
192-
193-
// Update the dependency to have the same prefix (newline + indentation)
194-
// so it appears on the next line after the comment
195-
contents.set(pair.index + 1, pair.tag.withPrefix(prefix));
196-
197-
return tag.withContent(contents);
234+
private void addDependency(List<Content> content, int insertIndex, Xml.Tag dependencyTag) {
235+
content.add(insertIndex, dependencyTag);
198236
}
199237

200-
@RequiredArgsConstructor
201-
private static class IndexedTag {
202-
final int index;
203-
final Xml.Tag tag;
238+
/**
239+
* Given an index of a dependency tag, find the insertion position that's before
240+
* any preceding comments that belong to that dependency.
241+
*/
242+
private int findInsertPositionBeforePrecedingComments(List<Content> content, int tagIndex) {
243+
int insertPos = tagIndex;
244+
// Walk backwards from the tag, skipping any immediately preceding comments
245+
while (insertPos > 0 && content.get(insertPos - 1) instanceof Xml.Comment) {
246+
insertPos--;
247+
}
248+
return insertPos;
204249
}
205250
}
206251
}

‎rewrite-maven/src/test/java/org/openrewrite/maven/AddManagedDependencyVisitorTest.java‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,10 @@
1717

1818
import org.junit.jupiter.api.Test;
1919
import org.openrewrite.DocumentExample;
20+
import org.openrewrite.ExecutionContext;
2021
import org.openrewrite.test.RecipeSpec;
2122
import org.openrewrite.test.RewriteTest;
23+
import org.openrewrite.xml.tree.Xml;
2224

2325
import static org.openrewrite.maven.Assertions.pomXml;
2426
import static org.openrewrite.test.RewriteTest.toRecipe;
@@ -122,4 +124,116 @@ void dependencyManagementTagExists() {
122124
)
123125
);
124126
}
127+
128+
@Test
129+
void multipleDependenciesWithBecauseComments() {
130+
rewriteRun(
131+
spec -> spec.recipe(toRecipe(() -> new MavenIsoVisitor<>() {
132+
@Override
133+
public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
134+
doAfterVisit(new AddManagedDependencyVisitor("org.apache.commons", "commons-compress", "1.26.0",
135+
null, null, null, "CVE-2023-42503"));
136+
doAfterVisit(new AddManagedDependencyVisitor("org.jetbrains.kotlin", "kotlin-stdlib", "1.6.21",
137+
null, null, null, "CVE-2022-24329"));
138+
doAfterVisit(new AddManagedDependencyVisitor("org.xerial.snappy", "snappy-java", "1.1.10.4",
139+
null, null, null, "CVE-2023-34455"));
140+
return document;
141+
}
142+
})),
143+
pomXml(
144+
"""
145+
<project>
146+
<groupId>com.mycompany.app</groupId>
147+
<artifactId>my-app</artifactId>
148+
<version>1</version>
149+
</project>
150+
""",
151+
"""
152+
<project>
153+
<groupId>com.mycompany.app</groupId>
154+
<artifactId>my-app</artifactId>
155+
<version>1</version>
156+
<dependencyManagement>
157+
<dependencies>
158+
<!-- CVE-2023-42503 -->
159+
<dependency>
160+
<groupId>org.apache.commons</groupId>
161+
<artifactId>commons-compress</artifactId>
162+
<version>1.26.0</version>
163+
</dependency>
164+
<!-- CVE-2022-24329 -->
165+
<dependency>
166+
<groupId>org.jetbrains.kotlin</groupId>
167+
<artifactId>kotlin-stdlib</artifactId>
168+
<version>1.6.21</version>
169+
</dependency>
170+
<!-- CVE-2023-34455 -->
171+
<dependency>
172+
<groupId>org.xerial.snappy</groupId>
173+
<artifactId>snappy-java</artifactId>
174+
<version>1.1.10.4</version>
175+
</dependency>
176+
</dependencies>
177+
</dependencyManagement>
178+
</project>
179+
"""
180+
)
181+
);
182+
}
183+
184+
@Test
185+
void multipleDependenciesWithBecauseCommentsSortedAlphabetically() {
186+
rewriteRun(
187+
spec -> spec.recipe(toRecipe(() -> new MavenIsoVisitor<>() {
188+
@Override
189+
public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
190+
doAfterVisit(new AddManagedDependencyVisitor("org.xerial.snappy", "snappy-java", "1.1.10.4",
191+
null, null, null, "CVE-2023-34455"));
192+
doAfterVisit(new AddManagedDependencyVisitor("org.apache.commons", "commons-compress", "1.26.0",
193+
null, null, null, "CVE-2023-42503"));
194+
doAfterVisit(new AddManagedDependencyVisitor("org.jetbrains.kotlin", "kotlin-stdlib", "1.6.21",
195+
null, null, null, "CVE-2022-24329"));
196+
return document;
197+
}
198+
})),
199+
pomXml(
200+
"""
201+
<project>
202+
<groupId>com.mycompany.app</groupId>
203+
<artifactId>my-app</artifactId>
204+
<version>1</version>
205+
</project>
206+
""",
207+
"""
208+
<project>
209+
<groupId>com.mycompany.app</groupId>
210+
<artifactId>my-app</artifactId>
211+
<version>1</version>
212+
<dependencyManagement>
213+
<dependencies>
214+
<!-- CVE-2023-42503 -->
215+
<dependency>
216+
<groupId>org.apache.commons</groupId>
217+
<artifactId>commons-compress</artifactId>
218+
<version>1.26.0</version>
219+
</dependency>
220+
<!-- CVE-2022-24329 -->
221+
<dependency>
222+
<groupId>org.jetbrains.kotlin</groupId>
223+
<artifactId>kotlin-stdlib</artifactId>
224+
<version>1.6.21</version>
225+
</dependency>
226+
<!-- CVE-2023-34455 -->
227+
<dependency>
228+
<groupId>org.xerial.snappy</groupId>
229+
<artifactId>snappy-java</artifactId>
230+
<version>1.1.10.4</version>
231+
</dependency>
232+
</dependencies>
233+
</dependencyManagement>
234+
</project>
235+
"""
236+
)
237+
);
238+
}
125239
}

‎rewrite-maven/src/test/java/org/openrewrite/maven/UpgradeTransitiveDependencyVersionTest.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,4 +248,66 @@ void addManagedDependencyWithBecause() {
248248
)
249249
);
250250
}
251+
252+
@Test
253+
void addMultipleManagedDependenciesWithBecause() {
254+
rewriteRun(spec ->
255+
spec.recipe(new UpgradeTransitiveDependencyVersion(
256+
"org.apache.tomcat.embed", "*", "10.1.42", null, null, null, null, null, null, null, "CVE-2024-TOMCAT")),
257+
pomXml(
258+
"""
259+
<project>
260+
<modelVersion>4.0.0</modelVersion>
261+
<groupId>org.openrewrite</groupId>
262+
<artifactId>core</artifactId>
263+
<version>0.1.0-SNAPSHOT</version>
264+
<dependencies>
265+
<dependency>
266+
<groupId>org.springframework.boot</groupId>
267+
<artifactId>spring-boot-starter-tomcat</artifactId>
268+
<version>3.3.12</version>
269+
</dependency>
270+
</dependencies>
271+
</project>
272+
""",
273+
"""
274+
<project>
275+
<modelVersion>4.0.0</modelVersion>
276+
<groupId>org.openrewrite</groupId>
277+
<artifactId>core</artifactId>
278+
<version>0.1.0-SNAPSHOT</version>
279+
<dependencyManagement>
280+
<dependencies>
281+
<!-- CVE-2024-TOMCAT -->
282+
<dependency>
283+
<groupId>org.apache.tomcat.embed</groupId>
284+
<artifactId>tomcat-embed-core</artifactId>
285+
<version>10.1.42</version>
286+
</dependency>
287+
<!-- CVE-2024-TOMCAT -->
288+
<dependency>
289+
<groupId>org.apache.tomcat.embed</groupId>
290+
<artifactId>tomcat-embed-el</artifactId>
291+
<version>10.1.42</version>
292+
</dependency>
293+
<!-- CVE-2024-TOMCAT -->
294+
<dependency>
295+
<groupId>org.apache.tomcat.embed</groupId>
296+
<artifactId>tomcat-embed-websocket</artifactId>
297+
<version>10.1.42</version>
298+
</dependency>
299+
</dependencies>
300+
</dependencyManagement>
301+
<dependencies>
302+
<dependency>
303+
<groupId>org.springframework.boot</groupId>
304+
<artifactId>spring-boot-starter-tomcat</artifactId>
305+
<version>3.3.12</version>
306+
</dependency>
307+
</dependencies>
308+
</project>
309+
"""
310+
)
311+
);
312+
}
251313
}

0 commit comments

Comments
 (0)