Skip to content

no precondition data tables #5248

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.

Sign up for GitHub

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

Closed
knutwannheden wants to merge 4 commits into from
Closed

no precondition data tables #5248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4dce52
Fix templating for `replaceArguments()` coordinate
knutwannheden Apr 2, 2025
4fb0280
Precondition recipes must not produce data table rows (#5228)
knutwannheden Mar 28, 2025
0066035
Precondition recipes must not produce data table rows
knutwannheden Apr 3, 2025
2717247
Merge branch 'main' into no-precondition-data-tables
knutwannheden Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion rewrite-core/src/main/java/org/openrewrite/DataTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import org.intellij.lang.annotations.Language;
import org.jspecify.annotations.Nullable;

import java.lang.reflect.ParameterizedType;
import java.util.ArrayList;
Expand All @@ -38,6 +39,7 @@ public class DataTable<Row> {
@Language("markdown")
private final @NlsRewrite.Description String description;

private transient final @Nullable Recipe recipe;
/**
* @param recipe The recipe that this data table is associated with.
* @param type The model type for a single row of this data table.
Expand All @@ -53,6 +55,7 @@ public DataTable(Recipe recipe, Class<Row> type, String name,
@NlsRewrite.Description @Language("markdown") String description) {
this.displayName = displayName;
this.description = description;
this.recipe = recipe;
recipe.addDataTable(this);
}

Expand All @@ -68,6 +71,7 @@ public DataTable(Recipe recipe,
@NlsRewrite.Description @Language("markdown") String description) {
this.displayName = displayName;
this.description = description;
this.recipe = recipe;

// Only null when transferring DataTables over RPC.
//noinspection ConstantValue
Expand All @@ -87,7 +91,7 @@ public String getName() {
}

public void insertRow(ExecutionContext ctx, Row row) {
if (!allowWritingInThisCycle(ctx)) {
if (!allowWritingInThisCycle(ctx) || !ctx.getCycleDetails().acceptRowForDataTable(this)) {
return;
}
ctx.computeMessage(ExecutionContext.DATA_TABLES, row, ConcurrentHashMap::new, (extract, allDataTables) -> {
Expand Down
5 changes: 1 addition & 4 deletions rewrite-core/src/main/java/org/openrewrite/TreeVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,7 @@ public P reduce(Tree tree, P p, Cursor parent) {
return defaultValue(null, p);
}

boolean topLevel = false;
if (visitCount == 0) {
topLevel = true;
}
boolean topLevel = visitCount == 0;

visitCount++;
setCursor(new Cursor(cursor, tree));
Expand Down
59 changes: 41 additions & 18 deletions rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.experimental.FieldDefaults;
import lombok.experimental.NonFinal;
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.internal.ExceptionUtils;
Expand Down Expand Up @@ -50,6 +51,10 @@ public class RecipeRunCycle<LSS extends LargeSourceSet> {
*/
Recipe recipe;

@NonFinal
@Nullable
Recipe current;

/**
* The current cycle in the range [1, maxCycles].
*/
Expand Down Expand Up @@ -77,19 +82,19 @@ public int getRecipePosition() {
public LSS scanSources(LSS sourceSet) {
return sourceSetEditor.apply(sourceSet, sourceFile ->
allRecipeStack.reduce(sourceSet, recipe, ctx, (source, recipeStack) -> {
Recipe recipe = recipeStack.peek();
current = recipeStack.peek();
if (source == null) {
return null;
}

SourceFile after = source;

if (recipe instanceof ScanningRecipe) {
if (current instanceof ScanningRecipe) {
try {
//noinspection unchecked
ScanningRecipe<Object> scanningRecipe = (ScanningRecipe<Object>) recipe;
ScanningRecipe<Object> scanningRecipe = (ScanningRecipe<Object>) current;
Object acc = scanningRecipe.getAccumulator(rootCursor, ctx);
recipeRunStats.recordScan(recipe, () -> {
recipeRunStats.recordScan(current, () -> {
TreeVisitor<?, ExecutionContext> scanner = scanningRecipe.getScanner(acc);
if (scanner.isAcceptable(source, ctx)) {
Tree maybeMutated = scanner.visit(source, ctx, rootCursor);
Expand All @@ -102,39 +107,40 @@ public LSS scanSources(LSS sourceSet) {
return source;
});
} catch (Throwable t) {
after = handleError(recipe, source, after, t);
after = handleError(current, source, after, t);
// We don't normally consider anything the scanning phase does to be a change
// But this simplifies error reporting so that exceptions can all be handled the same
assert after != null;
after = addRecipesThatMadeChanges(recipeStack, after);
}
}
current = null;
return after;
}, sourceFile)
);
}

public LSS generateSources(LSS sourceSet) {
List<SourceFile> generatedInThisCycle = allRecipeStack.reduce(sourceSet, recipe, ctx, (acc, recipeStack) -> {
Recipe recipe = recipeStack.peek();
if (recipe instanceof ScanningRecipe) {
current = recipeStack.peek();
if (current instanceof ScanningRecipe) {
assert acc != null;
//noinspection unchecked
ScanningRecipe<Object> scanningRecipe = (ScanningRecipe<Object>) recipe;
ScanningRecipe<Object> scanningRecipe = (ScanningRecipe<Object>) current;
// If some sources have already been generated by prior recipes, scan them now
// This helps to avoid recipes having inconsistent knowledge of which files exist
if (!acc.isEmpty()) {
for (SourceFile source : acc) {
try {
recipeRunStats.recordScan(recipe, () -> {
recipeRunStats.recordScan(current, () -> {
TreeVisitor<?, ExecutionContext> scanner = scanningRecipe.getScanner(scanningRecipe.getAccumulator(rootCursor, ctx));
if (scanner.isAcceptable(source, ctx)) {
scanner.visit(source, ctx, rootCursor);
}
return source;
});
} catch (Throwable t) {
handleError(recipe, source, source, t);
handleError(current, source, source, t);
}
}
}
Expand All @@ -143,9 +149,10 @@ public LSS generateSources(LSS sourceSet) {
if (!generated.isEmpty()) {
acc.addAll(generated);
generated.forEach(source -> recordSourceFileResult(null, source, recipeStack, ctx));
madeChangesInThisCycle.add(recipe);
madeChangesInThisCycle.add(current);
}
}
current = null;
return acc;
}, new ArrayList<>());

Expand All @@ -161,7 +168,7 @@ public LSS editSources(LSS sourceSet) {
// consider any recipes adding new messages as a changing recipe (which can request another cycle)
return sourceSetEditor.apply(sourceSet, sourceFile ->
allRecipeStack.reduce(sourceSet, recipe, ctx, (source, recipeStack) -> {
Recipe recipe = recipeStack.peek();
current = recipeStack.peek();
if (source == null) {
return null;
}
Expand All @@ -172,7 +179,7 @@ public LSS editSources(LSS sourceSet) {
Duration duration = Duration.ofNanos(System.nanoTime() - cycleStartTime);
if (duration.compareTo(ctx.getMessage(ExecutionContext.RUN_TIMEOUT, Duration.ofMinutes(4))) > 0) {
if (thrownErrorOnTimeout.compareAndSet(false, true)) {
RecipeTimeoutException t = new RecipeTimeoutException(recipe);
RecipeTimeoutException t = new RecipeTimeoutException(current);
ctx.getOnError().accept(t);
ctx.getOnTimeout().accept(t, ctx);
}
Expand All @@ -183,11 +190,11 @@ public LSS editSources(LSS sourceSet) {
return source;
}

TreeVisitor<?, ExecutionContext> visitor = recipe.getVisitor();
TreeVisitor<?, ExecutionContext> visitor = current.getVisitor();
// set root cursor as it is required by the `ScanningRecipe#isAcceptable()`
visitor.setCursor(rootCursor);

after = recipeRunStats.recordEdit(recipe, () -> {
after = recipeRunStats.recordEdit(current, () -> {
if (visitor.isAcceptable(source, ctx)) {
// propagate shared root cursor
//noinspection DataFlowIssue
Expand All @@ -197,7 +204,7 @@ public LSS editSources(LSS sourceSet) {
});

if (after != source) {
madeChangesInThisCycle.add(recipe);
madeChangesInThisCycle.add(current);
recordSourceFileResult(source, after, recipeStack, ctx);
if (source.getMarkers().findFirst(Generated.class).isPresent()) {
// skip edits made to generated source files so that they don't show up in a diff
Expand All @@ -207,15 +214,16 @@ public LSS editSources(LSS sourceSet) {
recipeRunStats.recordSourceFileChanged(source, after);
} else if (ctx.hasNewMessages()) {
// consider any recipes adding new messages as a changing recipe (which can request another cycle)
madeChangesInThisCycle.add(recipe);
madeChangesInThisCycle.add(current);
ctx.resetHasNewMessages();
}
} catch (Throwable t) {
after = handleError(recipe, source, after, t);
after = handleError(current, source, after, t);
}
if (after != null && after != source) {
after = addRecipesThatMadeChanges(recipeStack, after);
}
current = null;
return after;
}, sourceFile)
);
Expand Down Expand Up @@ -298,4 +306,19 @@ private static <S extends SourceFile> S addRecipesThatMadeChanges(List<Recipe> r
})
);
}

public boolean isCurrentlyExecuting(Recipe recipe) {
// The second alternative is for declarative recipes with preconditions
// The third alternative is currently required for RPC which doesn't execute recipes via RecipeRunCycle
return current == recipe ||
current instanceof Recipe.DelegatingRecipe && ((Recipe.DelegatingRecipe) current).getDelegate() == recipe ||
this.recipe == recipe;
}

public boolean acceptRowForDataTable(DataTable<?> dataTable) {
return isCurrentlyExecuting(dataTable.getRecipe()) ||
dataTable == recipeRunStats ||
dataTable == sourcesFileResults ||
dataTable == errorsTable;
}
}
128 changes: 128 additions & 0 deletions rewrite-core/src/test/java/org/openrewrite/DataTableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.fasterxml.jackson.annotation.JsonIgnoreType;
import org.junit.jupiter.api.Test;
import org.openrewrite.table.TextMatches;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.text.PlainText;
import org.openrewrite.text.PlainTextVisitor;
Expand Down Expand Up @@ -55,6 +56,133 @@ public PlainText visitText(PlainText text, ExecutionContext ctx) {
);
}

@Test
void noRowsFromPrecondition() {
rewriteRun(
spec -> spec
.recipe(toRecipe(r -> Preconditions.check(toRecipe(r2 -> new PlainTextVisitor<>() {
final WordTable wordTable = new WordTable(r2);

@Override
public PlainText visitText(PlainText text, ExecutionContext ctx) {
int i = 0;
for (String s : text.getText().split(" ")) {
wordTable.insertRow(ctx, new WordTable.Row(i++, s));
}
return text;
}
}), TreeVisitor.noop())))
.afterRecipe(run -> {
assertThat(run.getDataTables().keySet()).noneMatch(dt -> dt instanceof WordTable);
}),
text("hello world")
);
}

@Test
void noRowsFromDeclarativePrecondition() {
rewriteRun(
spec -> spec.recipeFromYaml(
"""
---
type: specs.openrewrite.org/v1beta/recipe
name: test.recipe
displayName: Recipe producing data table as precondition
description: .
preconditions:
- org.openrewrite.text.Find:
find: hello
recipeList:
- org.openrewrite.Recipe$Noop
""",
"test.recipe"
)
.afterRecipe(run -> {
assertThat(run.getDataTables().keySet()).noneMatch(dt -> dt instanceof TextMatches);
}),
text("hello world")
);
}

@Test
void rowsFromDeclarativeRecipe() {
rewriteRun(
spec -> spec.recipeFromYaml(
"""
---
type: specs.openrewrite.org/v1beta/recipe
name: test.recipe
displayName: Recipe producing data table in recipe list
description: .
recipeList:
- org.openrewrite.text.Find:
find: hello
""",
"test.recipe"
)
.afterRecipe(run -> {
assertThat(run.getDataTables().keySet()).anyMatch(dt -> dt instanceof TextMatches);
}),
text("hello world", "~~>hello world")
);
}

@Test
void rowsFromSecondaryDeclarativeRecipe() {
rewriteRun(
spec -> spec.recipeFromYaml(
"""
---
type: specs.openrewrite.org/v1beta/recipe
name: test.recipe
displayName: Top-level recipe
description: .
recipeList:
- test.recipe2
---
type: specs.openrewrite.org/v1beta/recipe
name: test.recipe2
displayName: Recipe producing data table
description: .
recipeList:
- org.openrewrite.text.Find:
find: hello
""",
"test.recipe"
)
.afterRecipe(run -> {
assertThat(run.getDataTables().keySet()).anyMatch(dt -> dt instanceof TextMatches);
}),
text("hello world", "~~>hello world")
);
}

@Test
void rowsFromDeclarativeRecipeWithPrecondition() {
rewriteRun(
spec -> spec.recipeFromYaml(
"""
---
type: specs.openrewrite.org/v1beta/recipe
name: test.recipe
displayName: Recipe producing data table as precondition and in recipe list
description: .
preconditions:
- org.openrewrite.text.Find:
find: hello
recipeList:
- org.openrewrite.text.Find:
find: hello
""",
"test.recipe"
)
.afterRecipe(run -> {
assertThat(run.getDataTables().keySet()).anyMatch(dt -> dt instanceof TextMatches);
}),
text("hello world", "~~>hello world")
);
}

@Test
void descriptor() {
Recipe recipe = toRecipe();
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/test/java/org/openrewrite/maven/AddDependencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,7 @@ void rawVisitorDoesNotDuplicate() {
toRecipe()
.withDisplayName("Add dependency")
.withName("Uses AddDependencyVisitor directly to validate that it will not add a dependency multiple times")
.withGetVisitor(() -> new AddDependencyVisitor(
.withGetVisitor(r -> new AddDependencyVisitor(
"com.google.guava",
"guava",
"29.0-jre",
Expand Down
Loading