Make CreateFileVisitor less opaque by returning if it changed the value

[ajturnerora]

What's changed?

Changed CreateFileVisitor to return SearchResult.found(sourceFile) rather than sourceFile if it updates the shouldCreate boolean.

What's your motivation?

To allow the visitor to be more easily used in precondition checks - it is currently used as a type of precondition in scanner recipes but is currently alway alone - e.g., when doing an .and it will return if any visitor returns the tree unchanged which this visitor always does. Currently you don't know if the visitor found the file or not without checking an external variable.

Have you considered any alternatives or workarounds?

Yes, I could wrap the visitor, but I don't see any downside to the visitor exposing if it made a change or not.

Checklist

• [x] I've added unit tests to cover both positive and negative cases
• [x] I've read and applied the recipe conventions and best practices
• [x] I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Hi @ajturnerora ; welcome back! Would you mind describing the use case you're looking to use CreateFileVisitor in more detail?

I'm mostly wondering if new FindSourceFiles(".github/workflows/*.yml").getVisitor() might be a better match to find existing files, as that also supports glob: https://github.com/openrewrite/rewrite/blob/a98d83d698dda9971c93a0767b7f85d5866f306d/rewrite-core/src/main/java/org/openrewrite/FindSourceFiles.java#L30

Typically we try to avoid having two ways to do one thing, so I'm mostly wondering if your use case is sufficiently different.

[ajturnerora]

Hi @timtebeek , I am creating a file if a condition is met (matching maven property) so I was making a recipe similar to CreateTextFile which uses CreateFileVisitor. I'll list some points of confusion I had lead to this PR.

My initial though was something like this:

preconditions:
  - my.org.MavenFind:
      artifactId: *-api
recipeList:
  - org.openrewrite.text.CreateTextFile:
     fileContents: Hi Tim!
     relativeFileName: test.txt
     overwriteExisting: true

1. Documentation say "Only when all preconditions are met will the recipes in the recipe list be run. When applying preconditions to ScanningRecipes they limit both the scanning phase and the edit phase.". I guess that is true, but it omits that the generate phase is always executed regardless of the preconditions which lead me to some confusion. Although, I'm also not sure about the "limit both the scanning phase and the edit phase". If I have a recipe as follows:

preconditions:
  - org.openrewrite.text.Find:
      find: 42
recipeList:
  - org.openrewrite.text.CreateTextFile:
      fileContents: Hi Tim!
      relativeFileName: test.txt
      overwriteExisting: true

The precondition cannot match, but the scanner still runs as BellwetherDecoratedScanningRecipe.getScanner doesn't consider the preconditions.

2. CreateTextFile has shouldCreate default to true, so with the above precondition it would seem like the logic is:

• if the file doesn't exist the precondition fails and the file will contain "Hi Tim!"
• if the file does exist but contains "Hi Andy!" the precondition fails and the file will contain "Hi Andy!"

This behavior seems a little odd as the precondition failed in both cases but the file contents differ.

3. Let's ignore I'm probably using this wrong, I do think the design of CreateFileVisitor is a little odd with passing a shared reference in the constructor. I would have thought there would be a supported pattern (similar to Result.subtreeChanged) where result = tree != visit(tree) and CreateFileVisitor returns a SearchResult.found as searching for a file is what CreateFileVisitor is doing.

4. The way I was using this - which I now realise is incorrect - was having my getScanner return Preconditions.and(mavenArtifactVisitor, createFileVisitor); as I want both of these things to be true for my file to be created with a recipe like:

  - com.oracle.rewrite.CreateFileFromResource:
      relativeSrcFileName: META-INF/rewrite/AddAuthModule.yml
      relativeDestFileName: target/AddAuthModule.yml
      mavenArtifactId: "*-api"

However, the scanner visitors aren't really preconditions so returning a Preconditions.and is somewhat incorrect. Although, I would still like to do the same thing where I can return something similar to return TreeVisitor.visitAll( mavenArtifactVisitor, createFileVisitor ) or chain them like return mavenArtifactVisitor.doAfter(createFileVisitor) (but doAfter is protected). I am not sure if there is already a visitor chaining mechanism other than Preconditions. I could manually call the visitor methods, but as CreateFileVisitor is already the visitor used to decide the generate logic in the provided CreateX recipes I wanted to compose CreateFileVisitor && MyLogicVisitor.

[timtebeek]

Thanks for the detailed write up of your thought process @ajturnerora ! In this case indeed CreateFileVisitor is a shared implementation detail of the various create file recipes, and needs the mutable state passed in to set the Accumulator value for those scanning recipes. I think it might be best not to make CreateFileVisitor dual purpose by having it return a SearchResult as well. For cases where you want to find files at a certain path (and preconditions) we continue to recommend FindSourceFiles. If based on the above you'd like to propose changes to how scanning recipes interact with preconditions then lets handle that separately, now that we both have a better understanding of what you're after.

I propose to close this PR and pick up any other suggestions separately. Thanks again for explaining your process! Do let us know if you have any further questions.