Skip to content

Commit af0520b

Browse files
steve-aom-elliottsteve-aom-elliott
committed
Address review feedback: warning markers, remove G/K guard, cache source set updates
- Attach Markup.warn to build files when version resolution for JavaSourceSet updates fails, so users know the classpath won't be updated - Remove counter-productive G.CompilationUnit/K.CompilationUnit exclusion in Gradle AddDependency and ChangeDependency — Groovy/Kotlin source files can have JavaSourceSet markers; Gradle build files won't have one and are already filtered by the maybeSourceSet.isPresent() check - Cache computed JavaSourceSet per source set in ChangeDependency (Gradle) and ChangeDependencyGroupIdAndArtifactId (Maven) to avoid redundant per-file recomputation when all files in a source set share identical markers
1 parent b7a4818 commit af0520b

5 files changed

Lines changed: 54 additions & 13 deletions

File tree

‎rewrite-gradle/src/main/java/org/openrewrite/gradle/AddDependency.java‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import org.openrewrite.gradle.marker.GradleProject;
2323
import org.openrewrite.gradle.search.FindJVMTestSuites;
2424
import org.openrewrite.gradle.trait.JvmTestSuite;
25-
import org.openrewrite.groovy.tree.G;
2625
import org.openrewrite.internal.StringUtils;
2726
import org.openrewrite.java.JavaIsoVisitor;
2827
import org.openrewrite.java.marker.JavaProject;
@@ -31,7 +30,7 @@
3130
import org.openrewrite.java.search.UsesType;
3231
import org.openrewrite.java.tree.J;
3332
import org.openrewrite.java.tree.JavaSourceFile;
34-
import org.openrewrite.kotlin.tree.K;
33+
import org.openrewrite.marker.Markup;
3534
import org.openrewrite.maven.MavenDownloadingException;
3635
import org.openrewrite.maven.MavenExecutionContextView;
3736
import org.openrewrite.maven.internal.MavenPomDownloader;
@@ -148,6 +147,8 @@ public static class Scanned {
148147
@Nullable
149148
String resolvedVersion;
150149
List<MavenRepository> repositories = new ArrayList<>();
150+
@Nullable
151+
Exception versionResolutionFailure;
151152
}
152153

153154
@Override
@@ -186,7 +187,9 @@ private boolean usesType(SourceFile sourceFile, ExecutionContext ctx) {
186187
sourceFile == hasTestSourceSet.visit(sourceFile, ctx)) {
187188
return tree;
188189
}
189-
sourceFile.getMarkers().findFirst(JavaProject.class).ifPresent(javaProject -> {
190+
Optional<JavaProject> maybeJavaProject = sourceFile.getMarkers().findFirst(JavaProject.class);
191+
if (maybeJavaProject.isPresent()) {
192+
JavaProject javaProject = maybeJavaProject.get();
190193
boolean uses = usesType(sourceFile, ctx);
191194
acc.usingType.compute(javaProject, (jp, usingType) -> Boolean.TRUE.equals(usingType) || uses);
192195

@@ -209,11 +212,11 @@ private boolean usesType(SourceFile sourceFile, ExecutionContext ctx) {
209212
acc.repositories = maybeGp.get().getMavenRepositories();
210213
}
211214
} catch (MavenDownloadingException e) {
212-
// Version resolution failed
215+
acc.versionResolutionFailure = e;
213216
}
214217
}
215218
}
216-
});
219+
}
217220
return tree;
218221
}
219222
};
@@ -310,6 +313,24 @@ private boolean isTopLevel(Cursor cursor) {
310313
);
311314

312315
if (acc.configurationsByProject.isEmpty() || acc.resolvedVersion == null) {
316+
if (acc.versionResolutionFailure != null) {
317+
Exception failure = acc.versionResolutionFailure;
318+
return new TreeVisitor<Tree, ExecutionContext>() {
319+
@Override
320+
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
321+
return gradleVisitor.isAcceptable(sourceFile, ctx);
322+
}
323+
324+
@Override
325+
public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
326+
Tree result = gradleVisitor.visit(tree, ctx);
327+
if (result != tree) {
328+
result = Markup.warn(result, failure);
329+
}
330+
return result;
331+
}
332+
};
333+
}
313334
return gradleVisitor;
314335
}
315336

@@ -331,8 +352,7 @@ public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
331352
if (gradleVisitor.isAcceptable(sf, ctx)) {
332353
return gradleVisitor.visit(tree, ctx);
333354
}
334-
if (sf instanceof JavaSourceFile
335-
&& !(sf instanceof G.CompilationUnit) && !(sf instanceof K.CompilationUnit)) {
355+
if (sf instanceof JavaSourceFile) {
336356
return updateJavaSourceSet(sf, ctx);
337357
}
338358
return tree;

‎rewrite-gradle/src/main/java/org/openrewrite/gradle/ChangeDependency.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,7 @@ private GradleProject updateGradleModel(GradleProject gp, ExecutionContext ctx)
485485
return new TreeVisitor<Tree, ExecutionContext>() {
486486
@Nullable
487487
private JavaSourceSetUpdater updater;
488+
private final Map<String, JavaSourceSet> updatedSourceSets = new HashMap<>();
488489

489490
@Override
490491
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
@@ -526,9 +527,8 @@ public Properties visitEntry(Properties.Entry entry, ExecutionContext ctx) {
526527
}
527528
return tree;
528529
}
529-
// For non-Gradle Java source files, update JavaSourceSet marker
530-
if (hasModulesWithOldDep && tree instanceof JavaSourceFile &&
531-
!(tree instanceof G.CompilationUnit) && !(tree instanceof K.CompilationUnit)) {
530+
// For Java source files, update JavaSourceSet marker
531+
if (hasModulesWithOldDep && tree instanceof JavaSourceFile) {
532532
return updateJavaSourceSet((SourceFile) tree, ctx);
533533
}
534534
return gradleVisitor.visit(tree, ctx);
@@ -547,6 +547,11 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
547547
if (!maybeSourceSet.isPresent()) {
548548
return sf;
549549
}
550+
String cacheKey = maybeJp.get().getId().toString() + ":" + maybeSourceSet.get().getName();
551+
JavaSourceSet cached = updatedSourceSets.get(cacheKey);
552+
if (cached != null) {
553+
return sf.withMarkers(sf.getMarkers().setByType(cached));
554+
}
550555
if (updater == null) {
551556
updater = new JavaSourceSetUpdater(ctx);
552557
}
@@ -560,6 +565,7 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
560565
.withRepository(MavenRepository.MAVEN_CENTRAL);
561566
JavaSourceSet updated = updater.changeDependency(maybeSourceSet.get(), oldDep, newDep);
562567
if (updated != maybeSourceSet.get()) {
568+
updatedSourceSets.put(cacheKey, updated);
563569
return sf.withMarkers(sf.getMarkers().setByType(updated));
564570
}
565571
return sf;

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import lombok.With;
2121
import org.jspecify.annotations.Nullable;
2222
import org.openrewrite.*;
23+
import org.openrewrite.marker.Markup;
2324
import org.openrewrite.java.marker.JavaProject;
2425
import org.openrewrite.java.marker.JavaSourceSet;
2526
import org.openrewrite.java.search.HasSourceSet;
@@ -163,6 +164,8 @@ public static class Scanned {
163164
@Nullable
164165
String resolvedVersion;
165166
List<MavenRepository> repositories = new ArrayList<>();
167+
@Nullable
168+
Exception versionResolutionFailure;
166169
}
167170

168171
@Override
@@ -218,7 +221,7 @@ public Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
218221
acc.repositories = repos;
219222
}
220223
} catch (Exception e) {
221-
// Version resolution failed; JavaSourceSet won't be updated
224+
acc.versionResolutionFailure = e;
222225
}
223226
}
224227
}
@@ -278,9 +281,13 @@ public Xml visitDocument(Xml.Document document, ExecutionContext ctx) {
278281
return maven;
279282
}
280283

281-
return new AddDependencyVisitor(
284+
Xml result = new AddDependencyVisitor(
282285
groupId, artifactId, version, versionPattern, resolvedScope, releasesOnly,
283286
type, classifier, optional, familyPatternCompiled, metadataFailures).visitNonNull(document, ctx);
287+
if (result != document && acc.versionResolutionFailure != null) {
288+
result = Markup.warn(result, acc.versionResolutionFailure);
289+
}
290+
return result;
284291
}
285292

286293
private boolean isSubprojectOfParentInRepository(Scanned acc) {

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
326326
return new TreeVisitor<Tree, ExecutionContext>() {
327327
@Nullable
328328
private JavaSourceSetUpdater updater;
329+
private final Map<String, JavaSourceSet> updatedSourceSets = new HashMap<>();
329330

330331
@Override
331332
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
@@ -360,6 +361,11 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
360361
if (!maybeSourceSet.isPresent()) {
361362
return sf;
362363
}
364+
String cacheKey = maybeJp.get().getId().toString() + ":" + maybeSourceSet.get().getName();
365+
JavaSourceSet cached = updatedSourceSets.get(cacheKey);
366+
if (cached != null) {
367+
return sf.withMarkers(sf.getMarkers().setByType(cached));
368+
}
363369
if (updater == null) {
364370
updater = new JavaSourceSetUpdater(ctx);
365371
}
@@ -378,6 +384,7 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
378384
.withRepository(findRemoteRepository(maybeJp.get()));
379385
JavaSourceSet updated = updater.changeDependency(maybeSourceSet.get(), oldDep, newDep);
380386
if (updated != maybeSourceSet.get()) {
387+
updatedSourceSets.put(cacheKey, updated);
381388
return sf.withMarkers(sf.getMarkers().setByType(updated));
382389
}
383390
return sf;

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,8 @@ void dontAddDuplicateIfUpdateModelOnPriorRecipeCycleFailed() {
9696
</project>
9797
""",
9898
"""
99-
<project>
99+
<!--~~(doesnotexist:doesnotexist failed. Unable to download metadata. Tried repositories:
100+
https://repo.maven.apache.org/maven2: HTTP 404)~~>--><project>
100101
<groupId>com.mycompany.app</groupId>
101102
<artifactId>my-app</artifactId>
102103
<version>1</version>

0 commit comments

Comments
 (0)
⚡