AppendToTextFile doesn't support newline-separated values?

maxime-michel commented 7 months ago

What version of OpenRewrite are you using?

I am using

OpenRewrite v8.20.0
Maven plugin v5.25.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module dummy project:

<project>
  <modelVersion>4.0.0</modelVersion>
  <groupId>info.magnolia</groupId>
  <artifactId>magnolia-rewrite</artifactId>
  <version>1.0-SNAPSHOT</version>
</project>

TEST_LIST:

TEST1
TEST2

rewrite.yml:

---
type: specs.openrewrite.org/v1beta/recipe
name: info.magnolia.rewrite.AddToList
recipeList:
  - org.openrewrite.text.AppendToTextFile:
      relativeFileName: TEST_LIST
      content: TEST3
  - org.openrewrite.text.AppendToTextFile:
      relativeFileName: TEST_LIST_NEW
      content: TEST3

What is the smallest, simplest way to reproduce the problem?

Simply run mvn org.openrewrite.maven:rewrite-maven-plugin:LATEST:run -Drewrite.activeRecipes=info.magnolia.rewrite.AddToList

What did you expect to see?

I would have expected to see TEST3 appended to the existing plain text file. Instead, the logs output:

[DEBUG] java.lang.ClassCastException: org.openrewrite.quark.Quark cannot be cast to org.openrewrite.text.PlainText
    at org.openrewrite.text.AppendToTextFile$2.visit (AppendToTextFile.java:138)
    at org.openrewrite.text.AppendToTextFile$2.visit (AppendToTextFile.java:129)
    at org.openrewrite.TreeVisitor.visit (TreeVisitor.java:184)
    at org.openrewrite.ScanningRecipe$1.visit (ScanningRecipe.java:112)
    at org.openrewrite.ScanningRecipe$1.visit (ScanningRecipe.java:94)
    at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$5 (RecipeRunCycle.java:164)
    at io.micrometer.core.instrument.AbstractTimer.recordCallable (AbstractTimer.java:175)
    at org.openrewrite.table.RecipeRunStats.recordEdit (RecipeRunStats.java:68)
    at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$6 (RecipeRunCycle.java:161)
    at org.openrewrite.scheduling.RecipeStack.reduce (RecipeStack.java:57)
    at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$7 (RecipeRunCycle.java:134)
    at org.openrewrite.internal.InMemoryLargeSourceSet.lambda$edit$0 (InMemoryLargeSourceSet.java:66)
    at org.openrewrite.internal.ListUtils.map (ListUtils.java:176)
    at org.openrewrite.internal.InMemoryLargeSourceSet.edit (InMemoryLargeSourceSet.java:65)
    at org.openrewrite.scheduling.RecipeRunCycle.editSources (RecipeRunCycle.java:133)
    at org.openrewrite.RecipeScheduler.runRecipeCycles (RecipeScheduler.java:86)
    at org.openrewrite.RecipeScheduler.scheduleRun (RecipeScheduler.java:41)
    at org.openrewrite.Recipe.run (Recipe.java:344)
    at org.openrewrite.Recipe.run (Recipe.java:340)
    at org.openrewrite.Recipe.run (Recipe.java:336)
    at org.openrewrite.maven.AbstractRewriteMojo.runRecipe (AbstractRewriteMojo.java:265)
    at org.openrewrite.maven.AbstractRewriteMojo.listResults (AbstractRewriteMojo.java:242)
    at org.openrewrite.maven.AbstractRewriteRunMojo.execute (AbstractRewriteRunMojo.java:62)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:126)
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2 (MojoExecutor.java:328)
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute (MojoExecutor.java:316)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:212)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:174)
    at org.apache.maven.lifecycle.internal.MojoExecutor.access$000 (MojoExecutor.java:75)
    at org.apache.maven.lifecycle.internal.MojoExecutor$1.run (MojoExecutor.java:162)
    at org.apache.maven.plugin.DefaultMojosExecutionStrategy.execute (DefaultMojosExecutionStrategy.java:39)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:159)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:105)
    at io.takari.maven.builder.smart.SmartBuilderImpl.buildProject (SmartBuilderImpl.java:209)
    at io.takari.maven.builder.smart.SmartBuilderImpl$ProjectBuildTask.run (SmartBuilderImpl.java:81)
    at java.util.concurrent.Executors$RunnableAdapter.call (Executors.java:511)
    at java.util.concurrent.FutureTask.run (FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:624)
    at java.lang.Thread.run (Thread.java:750)

What did you see instead?

TEST_LIST_NEW was created with the expected content, showing that everything is correctly wired, but the format used by my already-existing text file is not parsed. The documentation however doesn't hint at anything peculiar, only brings up 'plaintext', which my file is.

timtebeek commented 7 months ago

It looks like your file isn't parsed as plain text, but instead as a Quark; https://docs.openrewrite.org/concepts-explanations/quarks

You might have better luck when you specifically mark that file as a file type that should be parsed as plain text: https://openrewrite.github.io/rewrite-maven-plugin/run-mojo.html#plainTextMasks

Hope that helps and let us know if this issue can be closed.

maxime-michel commented 7 months ago

Thanks for your help and sorry I didn't find this configuration option on my own. Unfortunately in the same test project as in my initial post, running mvn org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.activeRecipes=info.magnolia.rewrite.AddToList -Drewrite.plainTextMasks="**/TEST_LIST" is still not enough. The exception with the Quark is now gone, but the desired addition to the file is not made. The logs do not say much other than the configuration is properly applied, and there is no exclusion that may get in the way:

[DEBUG]   (f) exclusions = []
[DEBUG]   (f) plainTextMasks = [**/TEST_LIST]

timtebeek commented 7 months ago

Hmm; it seems our docs don't format a particular option well that I don't yet see you using: https://github.com/openrewrite/rewrite/blob/22eef9abff1494d650d54425682db9dc7994fa60/rewrite-core/src/main/java/org/openrewrite/text/AppendToTextFile.java#L55-L66

While perhaps surprising, the default extistingFileStrategy is Leave, which does not change your file. https://github.com/openrewrite/rewrite/blob/22eef9abff1494d650d54425682db9dc7994fa60/rewrite-core/src/main/java/org/openrewrite/text/AppendToTextFile.java#L138-L147

Perhaps it helps to set an explicit strategy of Continue if your file already existings when the first recipe runs.

maxime-michel commented 7 months ago

Awesome, thank you, so if I got it right we could consider the following improvements for this to be a better experience for the next user:

fix the formatting on the recipe page
make it more obvious that existing files will by default be untouched, or maybe consider changing the default?
make a reference to plainTextMasks on the page

I know I'm looking at it from a very specific angle, and probably missing some context on why some things are the way they are, so please let me know your thoughts on the above? I may then contribute.

timtebeek commented 7 months ago

Thanks for your suggestions ! Definitely agree we need to fix the formatting for that option table in the presence of newlines. That'll be in our RecipeMarkdownGenerator. /cc @mike-solomon

Changing the default might surprise existing users, so I'd think that's best avoided (unfortunately).

A reference to plainTextMasks could be helpful. That could probably go into the description as a link to the Maven and Gradle plugin configuration pages in the docs. https://github.com/openrewrite/rewrite/blob/22eef9abff1494d650d54425682db9dc7994fa60/rewrite-core/src/main/java/org/openrewrite/text/AppendToTextFile.java#L74-L75

Any help with those updates much appreciated; Mike can help review as I'm off for the next couple of weeks.

maxime-michel commented 7 months ago

Here'd be my suggestion. Will take a look at the Markdown generator later, though I guess the first step there would be an exact description of the issue as well as an issue in the repo itself.

timtebeek commented 7 months ago

Much appreciated! Mike can likely help with the reviews in my absence. :)

timtebeek commented 5 months ago

Thanks a lot for the fix to the docs! I think we can close this issue now, but let me know if I missed something.

openrewrite / rewrite