Skip to content

Commit 99bfe64

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 8d40472 commit 99bfe64

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
@@ -502,6 +502,7 @@ private GradleProject updateGradleModel(GradleProject gp, ExecutionContext ctx)
502502
return new TreeVisitor<Tree, ExecutionContext>() {
503503
@Nullable
504504
private JavaSourceSetUpdater updater;
505+
private final Map<String, JavaSourceSet> updatedSourceSets = new HashMap<>();
505506

506507
@Override
507508
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
@@ -543,9 +544,8 @@ public Properties visitEntry(Properties.Entry entry, ExecutionContext ctx) {
543544
}
544545
return tree;
545546
}
546-
// For non-Gradle Java source files, update JavaSourceSet marker
547-
if (hasModulesWithOldDep && tree instanceof JavaSourceFile &&
548-
!(tree instanceof G.CompilationUnit) && !(tree instanceof K.CompilationUnit)) {
547+
// For Java source files, update JavaSourceSet marker
548+
if (hasModulesWithOldDep && tree instanceof JavaSourceFile) {
549549
return updateJavaSourceSet((SourceFile) tree, ctx);
550550
}
551551
return gradleVisitor.visit(tree, ctx);
@@ -564,6 +564,11 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
564564
if (!maybeSourceSet.isPresent()) {
565565
return sf;
566566
}
567+
String cacheKey = maybeJp.get().getId().toString() + ":" + maybeSourceSet.get().getName();
568+
JavaSourceSet cached = updatedSourceSets.get(cacheKey);
569+
if (cached != null) {
570+
return sf.withMarkers(sf.getMarkers().setByType(cached));
571+
}
567572
if (updater == null) {
568573
updater = new JavaSourceSetUpdater(ctx);
569574
}
@@ -577,6 +582,7 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
577582
.withRepository(MavenRepository.MAVEN_CENTRAL);
578583
JavaSourceSet updated = updater.changeDependency(maybeSourceSet.get(), oldDep, newDep);
579584
if (updated != maybeSourceSet.get()) {
585+
updatedSourceSets.put(cacheKey, updated);
580586
return sf.withMarkers(sf.getMarkers().setByType(updated));
581587
}
582588
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
@@ -331,6 +331,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
331331
return new TreeVisitor<Tree, ExecutionContext>() {
332332
@Nullable
333333
private JavaSourceSetUpdater updater;
334+
private final Map<String, JavaSourceSet> updatedSourceSets = new HashMap<>();
334335

335336
@Override
336337
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
@@ -365,6 +366,11 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
365366
if (!maybeSourceSet.isPresent()) {
366367
return sf;
367368
}
369+
String cacheKey = maybeJp.get().getId().toString() + ":" + maybeSourceSet.get().getName();
370+
JavaSourceSet cached = updatedSourceSets.get(cacheKey);
371+
if (cached != null) {
372+
return sf.withMarkers(sf.getMarkers().setByType(cached));
373+
}
368374
if (updater == null) {
369375
updater = new JavaSourceSetUpdater(ctx);
370376
}
@@ -383,6 +389,7 @@ private SourceFile updateJavaSourceSet(SourceFile sf, ExecutionContext ctx) {
383389
.withRepository(findRemoteRepository(maybeJp.get()));
384390
JavaSourceSet updated = updater.changeDependency(maybeSourceSet.get(), oldDep, newDep);
385391
if (updated != maybeSourceSet.get()) {
392+
updatedSourceSets.put(cacheKey, updated);
386393
return sf.withMarkers(sf.getMarkers().setByType(updated));
387394
}
388395
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)
⚡