Add a test validation that execution context is not mutated during re…

…cipe execution (#4879) * Add a test validation that execution context is not mutated during the recipe run. * Allow messages from MavenExecutionContextView
openrewrite · Jan 16, 2025 · 36efb73 · 36efb73
1 parent 462dc74
commit 36efb73
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 5 deletions.
diff --git a/rewrite-core/src/main/java/org/openrewrite/CursorValidatingExecutionContextView.java b/rewrite-core/src/main/java/org/openrewrite/CursorValidatingExecutionContextView.java
@@ -15,8 +15,11 @@
  */
 package org.openrewrite;
 
+import org.jspecify.annotations.Nullable;
+
 public class CursorValidatingExecutionContextView extends DelegatingExecutionContext {
     private static final String VALIDATE_CURSOR_ACYCLIC = "org.openrewrite.CursorValidatingExecutionContextView.ValidateCursorAcyclic";
+    private static final String VALIDATE_CTX_MUTATION = "org.openrewrite.CursorValidatingExecutionContextView.ValidateExecutionContextImmutability";
 
     public CursorValidatingExecutionContextView(ExecutionContext delegate) {
         super(delegate);
@@ -38,4 +41,23 @@ public CursorValidatingExecutionContextView setValidateCursorAcyclic(boolean val
         putMessage(VALIDATE_CURSOR_ACYCLIC, validateCursorAcyclic);
         return this;
     }
+
+    public CursorValidatingExecutionContextView setValidateImmutableExecutionContext(boolean allowExecutionContextMutation) {
+        putMessage(VALIDATE_CTX_MUTATION, allowExecutionContextMutation);
+        return this;
+    }
+
+    @Override
+    public void putMessage(String key, @Nullable Object value) {
+        boolean mutationAllowed = !getMessage(VALIDATE_CTX_MUTATION, false) || key.equals(VALIDATE_CURSOR_ACYCLIC) || key.equals(VALIDATE_CTX_MUTATION)
+                                  || key.equals(ExecutionContext.CURRENT_CYCLE) || key.equals(ExecutionContext.CURRENT_RECIPE) || key.equals(ExecutionContext.DATA_TABLES)
+                                  || key.startsWith("org.openrewrite.maven"); // MavenExecutionContextView stores metrics
+        assert mutationAllowed
+                : "Recipe mutated execution context key \"" + key + "\". " +
+                  "Recipes should not mutate the contents of the ExecutionContext as it allows mutable state to leak between " +
+                  "recipes, opening the door for difficult to debug recipe composition errors. " +
+                  "If you need to store state within the execution of a single recipe use Cursor messaging. " +
+                  "If you want to pass state between recipes, use a ScanningRecipe instead.";
+        super.putMessage(key, value);
+    }
 }
diff --git a/rewrite-core/src/test/java/org/openrewrite/ExecutionContextTest.java b/rewrite-core/src/test/java/org/openrewrite/ExecutionContextTest.java
@@ -17,6 +17,7 @@
 
 import org.junit.jupiter.api.Test;
 import org.openrewrite.test.RewriteTest;
+import org.openrewrite.test.TypeValidation;
 import org.openrewrite.text.PlainText;
 import org.openrewrite.text.PlainTextVisitor;
 
@@ -41,7 +42,8 @@ public PlainText visitText(PlainText text, ExecutionContext ctx) {
                   cycles.incrementAndGet();
                   return text;
               }
-          }).withCausesAnotherCycle(true)),
+          }).withCausesAnotherCycle(true))
+            .typeValidationOptions(TypeValidation.all().immutableExecutionContext(false)),
           text("hello world")
         );
         assertThat(cycles.get()).isEqualTo(2);

diff --git a/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java b/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java
@@ -367,9 +367,9 @@ default void rewriteRun(Consumer<RecipeSpec> spec, SourceSpec<?>... sourceSpecs)
             lss = new LargeSourceSetCheckingExpectedCycles(expectedCyclesThatMakeChanges, runnableSourceFiles);
         }
 
-        CursorValidatingExecutionContextView.view(recipeCtx)
-                .setValidateCursorAcyclic(TypeValidation.before(testMethodSpec, testClassSpec)
-                .cursorAcyclic());
+        recipeCtx = CursorValidatingExecutionContextView.view(recipeCtx)
+                .setValidateCursorAcyclic(TypeValidation.before(testMethodSpec, testClassSpec).cursorAcyclic())
+                .setValidateImmutableExecutionContext(TypeValidation.before(testMethodSpec, testClassSpec).immutableExecutionContext());
         RecipeRun recipeRun = recipe.run(
                 lss,
                 recipeCtx,

diff --git a/rewrite-test/src/main/java/org/openrewrite/test/TypeValidation.java b/rewrite-test/src/main/java/org/openrewrite/test/TypeValidation.java
@@ -103,6 +103,14 @@ public class TypeValidation {
     @Builder.Default
     private boolean erroneous = true;
 
+    /**
+     * Adding messages to execution context is a side effect which makes the recipe run itself stateful.
+     * Potentially allows recipes to interfere with each other in surprising and hard to debug ways.
+     * Problematic for all the same reasons mutable global variables or singletons are.
+     */
+    @Builder.Default
+    private boolean immutableExecutionContext = true;
+
     /**
      * Enable all invariant validation checks.
      */
@@ -114,7 +122,7 @@ public static TypeValidation all() {
      * Skip all invariant validation checks.
      */
     public static TypeValidation none() {
-        return new TypeValidation(false, false, false, false, false, false, false, false, o -> false, false);
+        return new TypeValidation(false, false, false, false, false, false, false, false, o -> false, false, false);
     }
 
     static TypeValidation before(RecipeSpec testMethodSpec, RecipeSpec testClassSpec) {

diff --git a/rewrite-test/src/test/java/org/openrewrite/test/internal/RewriteTestTest.java b/rewrite-test/src/test/java/org/openrewrite/test/internal/RewriteTestTest.java
@@ -112,6 +112,42 @@ void rejectRecipeValidationFailure() {
               """, "org.openrewrite.RefersToNonExistentRecipe")
           ));
     }
+
+    @Test
+    void rejectExecutionContextMutation() {
+        assertThrows(AssertionError.class, () ->
+          rewriteRun(
+            spec -> spec.recipe(new MutateExecutionContext()),
+            text("irrelevant")
+          ));
+    }
+}
+
+@Value
+@EqualsAndHashCode(callSuper = false)
+@NullMarked
+class MutateExecutionContext extends Recipe {
+
+    @Override
+    public String getDisplayName() {
+        return "Mutate execution context";
+    }
+
+    @Override
+    public String getDescription() {
+        return "Mutates the execution context to trigger a validation failure.";
+    }
+
+    @Override
+    public TreeVisitor<?, ExecutionContext> getVisitor() {
+        return new TreeVisitor<>() {
+            @Override
+            public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
+                ctx.putMessage("mutated", true);
+                return tree;
+            }
+        };
+    }
 }
 
 @Value