openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.21k stars 329 forks source link

Singlesource applicability with FindSourceFiles now applies changes to all files #2919

Closed gsmet closed 1 year ago

gsmet commented 1 year ago

Hi,

I'm not sure if it's a problem with OpenRewrite itself or the Rewrite Maven plugin but after an upgrade to 4.41.0, the Rewrite Maven plugin starts pushing unwanted changes to my YAML and properties files.

When rewriting https://github.com/quarkiverse/quarkus-github-app with our Quarkus upgrade script, I end up with the following diff between a rewrite with 4.39.0 and a rewrite with 4.41.0:

(the private key is a test one that isn't used anywhere, no worries here)

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 84703f7..b2452e3 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -30,6 +30,7 @@ jobs:
       fail-fast: false
       matrix:
         java:
+          
           - {
             version: "11"
           }
diff --git a/.github/workflows/downstream.yml b/.github/workflows/downstream.yml
index fdfd744..3a918c0 100644
--- a/.github/workflows/downstream.yml
+++ b/.github/workflows/downstream.yml
@@ -31,6 +31,7 @@ jobs:
       fail-fast: false
       matrix:
         project:
+          
           - {
             organization: "quarkusio",
             repository: "quarkus-github-bot"
-quarkus.github-app.private-key=-----BEGIN RSA PRIVATE KEY-----\
-MIIEowIBAAKCAQEAtsCKYkMzgqSsbkcPThTjeYnd6r5RAS4q5SyiOyIf34nYH5D0\
-jy8Dlt1s8u69KmBoM9Neesqf2Yh80b/UNUtea9knpgR4ME+MIg1LdsLnUWoZuPBl\
-lLaMfwRkN4+ZbTJ7Uo98UUlxoTcCjZVzopSoIazuUF5u+01sx6QEcHB312s42FVL\
-gkd8T36wvIywseOPKr2oyCMQ2AdcxL/cEHwx2hcQAe+eTipsxNy4atIhbDWrML5W\
-v7cYlyn9/sKgxkXtW3eInP3VI3FS+IH+cjwO4FS6ez8TT6USfaCjDb4LgdEKt/de\
-Ds4TbqwM2vjqedLNBhE7t4V2Uf3STdFq+cKHwwIDAQABAoIBAE07MUA1hh8/2F8C\
-SMWGrls+mDoME8+b4yTWp/i1gyLE7kDo0XFxPOMU0GYZ3nd6Jo9AVD0wRD16IMXD\
-e7rKDy0kqEzQtroz49TAKZQW6grN+/DcJxGh094ZzQBt/zjWjKdnW/I+R3cJ+Fo9\
-PpEGoccZfd0ZC23IWqBEAFxEK2Eth7CbJW5eMJElyqSZa9jiF6wdnryH4Xww38iE\
-zv5tL2Ieg03lxj0mdeyzFyIzJTk3PEAFLIjIiducIhSWyJHltpKQEZ3iFMzKfx7l\
-U0KGUYECgYEA5xcnO9nv58QdcMXobHWethh+a2QTWmEXbE/Lnunqgq+bTgJUbgXR\
-Fi+awb+6GI8lJh16oBbzSRGe2pFQcQa9S84+CnAyVVyras4WC5bcl38q46wh0IRW\
-2sqvJfAI/CJZxgLVceELJ4hn9t8/7Ucoh6ZYYSsVnwtvvrmzMsC1z/0CgYEAynOC\
-v0O+FC5MI+p6HQXdK8JWNecIMEoat5RpmF6nvYSRmHliggoeAV5Of7b6Y0JBRgGm\
-+wECWCKMfaJ1BdCOncci+YkLguSBeJGbDkr65En/zzPxtuvYqcg05x50RWdbLQ5N\
-PF6FHSC8FILpfpJ5f/W8rfNNmU4eTKS4DxUr4r8CgYEA4vfqoO48ovYLtGetEFm1\
-uEP2ZqO0HmCeENOOulYk7pZrgwLmyekMoy2+Ye1daiGt6vGpLvNbn7ievS1cRKbJ\
-5Vp7tOTditmpww0GuftCTcmo5lR6IcLZS6smu6w2Ju3WHpVJ7r+JpRpkgiRjNTle\
-pVzMESOv6LXi2wCo8IA2EkECgYAOyzQRr+yS4vMzaK31svj/epr8I17I0JF1OsYg\
-mUIeqjJNdwlIwV6B8RdBY+iWGkBU0kgWbXNzZ0rm31k3zI6vXt7iZy5NKU+AtPsk\
-pzwANJwZ0wzltgRGG9gpz2Lls3DJMRNZxvppL3wu74YKdr+kJxvbhjz0Z+304dCF\
-YaGsVwKBgA6KSf1pPD93W/v5tV0WnhBZGKb3sX34JLQoCJPS5w6nTjHnuwpP9g0K\
-C8HXKULXuX/rb/gz99PNcPQ9GqB0Rw/ho1KhO3VLJjBiSh/hiASKuFGs2y59UXYt\
-mvqX3jAqi6ksGD5WPG6G2CdL/n30e5/lc10hq7SiwV7z9njW/8Pu\
------END RSA PRIVATE KEY-----
+quarkus.github-app.private-key=-----BEGIN RSA PRIVATE KEY-----MIIEowIBAAKCAQEAtsCKYkMzgqSsbkcPThTjeYnd6r5RAS4q5SyiOyIf34nYH5D0jy8Dlt1s8u69KmBoM9Neesqf2Yh80b/UNUtea9knpgR4ME+MIg1LdsLnUWoZuPBllLaMfwRkN4+ZbTJ7Uo98UUlxoTcCjZVzopSoIazuUF5u+01sx6QEcHB312s42FVLgkd8T36wvIywseOPKr2oyCMQ2AdcxL/cEHwx2hcQAe+eTipsxNy4atIhbDWrML5Wv7cYlyn9/sKgxkXtW3eInP3VI3FS+IH+cjwO4FS6ez8TT6USfaCjDb4LgdEKt/deDs4TbqwM2vjqedLNBhE7t4V2Uf3STdFq+cKHwwIDAQABAoIBAE07MUA1hh8/2F8CSMWGrls+mDoME8+b4yTWp/i1gyLE7kDo0XFxPOMU0GYZ3nd6Jo9/I+R3cJ+Fo9PpEGoccZfd0ZC23IWqBEAFxEK2Eth7CbJW5eMJElyqSZa9jiF6wdnryH4Xww38iEzv5tL2Ieg03lxj0mdeyzFyIzJTk3PEAFLIjIiducIhSWyJHltpKQEZ3iFMzKfx7lqLIczsyLRouc64+T7lW1PO6SQshHlvc6CtJaHHf98iFMDwdDlSolF/PfNLE5C6lAU0KGUYECgYEA5xcnO9nv58QdcMXobHWethh+a2QTWmEXbE/Lnunqgq+bTgJUbgXRFi+awb+6GI8lJh16oBbzSRGe2pFQcQa9S84+CnAyVVyras4WC5bcl38q46wh0IRW2sqvJfAI/CJZxgLVceELJ4hn9t8/7Ucoh6ZYYSsVnwtvvrmzMsC1z/0CgYEAynOCv0O+FC5MI+p6HQXdK8JWNecIMEoat5RpmF6nvYSRmHliggoeAV5Of7b6Y0JBRgGm+wECWCKMfaJ1BdCOncci+YkLguSBeJGbDkr65En/zzPxtuvYqcg05x50RWdbLQ5NPF6FHSC8FILpfpJ5f/W8rfNNmU4eTKS4DxUr4r8CgYEA4vfqoO48ovYLtGetEFm1uEP2ZqO0HmCeENOOulYk7pZrgwLmyekMoy2+Ye1daiGt6vGpLvNbn7ievS1cRKbJ5Vp7tOTditmpww0GuftCTcmo5lR6IcLZS6smu6w2Ju3WHpVJ7r+JpRpkgiRjNTlepVzMESOv6LXi2wCo8IA2EkECgYAOyzQRr+yS4vMzaK31svj/epr8I17I0JF1OsYgmUIeqjJNdwlIwV6B8RdBY+iWGkBU0kgWbXNzZ0rm31k3zI6vXt7iZy5NKU+AtPskpzwANJwZ0wzltgRGG9gpz2Lls3DJMRNZxvppL3wu74YKdr+kJxvbhjz0Z+304dCFYaGsVwKBgA6KSf1pPD93W/v5tV0WnhBZGKb3sX34JLQoCJPS5w6nTjHnuwpP9g0KC8HXKULXuX/rb/gz99PNcPQ9GqB0Rw/ho1KhO3VLJjBiSh/hiASKuFGs2y59UXYtmvqX3jAqi6ksGD5WPG6G2CdL/n30e5/lc10hq7SiwV7z9njW/8Pu-----END RSA PRIVATE KEY-----

So OpenRewrite is somehow:

That's not something I want so I'm wondering what is going on. Let me know if it's more of a core issue that should be moved there.

Thanks.

timtebeek commented 1 year ago

Thank you for reporting this here! I'm adding a couple of hopefully helpful links here to aid troubleshooting: You report going from https://github.com/openrewrite/rewrite-maven-plugin/compare/v4.39.0...v4.41.0 Which in turn goes from rewrite https://github.com/openrewrite/rewrite/compare/v7.35.0...v7.37.3 .

I don't immediately see any relevant changes in the 261 commits. Would you mind testing any intermediate maven plugin versions to help narrow it down? Or provide the command you're using to perform your upgrades?

gsmet commented 1 year ago

hi @timtebeek ,

Sure I will bissect tomorrow and see if I can pinpoint it to one particular version.

timtebeek commented 1 year ago

Thanks a lot!

gsmet commented 1 year ago

I think I found out what the issue is.

My recipes targeting singleSource with wildcards are applied to all the files of the project, totally ignoring the applicability, AFAICS:

I get:

[WARNING] Changes have been made to runtime/src/main/java/io/quarkiverse/githubapp/runtime/github/GitHubConfigFileProviderImpl.java by:
[WARNING]     io.quarkus.updates.core.quarkus30.Quarkus3
[WARNING]         io.quarkus.updates.core.quarkus30.JavaxToJakartaDocumentation
[WARNING]             org.openrewrite.text.FindAndReplace: {find=javax.json.bind.config., replace=jakarta.json.bind.config.}
[WARNING]                 org.openrewrite.Recipe$AdHocRecipe
...

(so OpenRewrite adjusting a .java file)

while I have for io.quarkus.updates.core.quarkus30.JavaxToJakartaDocumentation the following recipe:

type: specs.openrewrite.org/v1beta/recipe
name: io.quarkus.updates.core.quarkus30.JavaxToJakartaDocumentation
applicability:
  singleSource:
    - org.openrewrite.FindSourceFiles:
        filePattern: "**/*.adoc"
recipeList:
  - org.openrewrite.text.FindAndReplace:
      find: javax.json.bind.config.
      replace: jakarta.json.bind.config.
  ...

So this rule should only be applied to *.adoc files of my project but it is applied to Java files, properties, YAML..., thus modifying files that shouldn't have been modified (and me noticing it).

The upgrade starts being extremely slow as it applies all our text transformations to all the files instead of targeting only the ones in the applicability.singleSource.

I bissected and AFAICS the regression was introduced in OpenRewrite 7.37.0 (and it is still a problem in 7.37.3). 7.36.1 is working as expected.

So unless I'm doing something incorrect (but that was working before), a regression was probably introduced in 7.37.0 regarding applicability singleSource.

For reference, our recipes are hosted here: https://github.com/quarkusio/quarkus-updates/blob/main/recipes/src/main/resources/quarkus-updates/core/3alpha.yaml

timtebeek commented 1 year ago

Thanks a lot both for bisecting where the behavior changed as well as providing the additional context around the recipe & applicability test used. It looks like you're using a pattern similar to what we have in the documentation, but curiously that's also the only place I see FindSourceFiles used in that way. 🤔 That kind of makes me doubt if that's the correct way to limit recipe execution for text replacements to certain files. For instance: elsewhere we use HasSourcePath to limit recipes to certain files, which could be helpful here as well.

Would you mind giving this a try with HasSourcePath? That could indicate this is a "bug" in the documentation instead.

shanman190 commented 1 year ago

So this was because applicability tests didn't apply correctly and was fixed here (which is in the commit range of differences that @timtebeek tagged in his earlier comment):

https://github.com/openrewrite/rewrite/commit/d974a7d7975bec7980d358cdb123b1875223df22

Applicability tests were inappropriately selecting sources under the single source case, especially, which is what led to your issue here. If I were you, I'd try this again on the newest maven plugin and you should have much better luck.

gsmet commented 1 year ago

@shanman190 I don't think it's related given my problem is specifically with 7.37.0..7.37.3 and the commit you pointed out was introduced in 7.36.0.

@timtebeek I'll give it a try but IIRC, the pattern we are using was pointed out by someone from the OpenRewrite team so we are probably not the only ones doing that. I'll post about my findings soon.

shanman190 commented 1 year ago

@gsmet, I missed that on the first read; apologies about missing that!

gsmet commented 1 year ago

@timtebeek the issue is that I cannot use HasSourcePath in my YAML descriptor directly as it is not a Recipe, it's a TreeVisitor.

My understanding is that using FindSourceFiles was the blessed way to apply recipes to a bunch of files.

gsmet commented 1 year ago

@timtebeek any news? Asking that because I'm hitting another issue with 7.36.0 that seems to be fixed with the latest and I'm in an uncomfortable situation.

Thanks!

timtebeek commented 1 year ago

Understandable that you're looking to see this resolved; I don't have any immediate answers, but I've asked @kunli2 if he can chime in to see what (if any) changes need to be made to get you on the latest versions again.

jbessels commented 1 year ago

I started with Open Rewrite 4.40.0 iirc and did a migrate Java 11 with it. Wanted to do a migrate Java 17 but got the "[OpenRewrite: The contents of a quark are unknown, so the charset is unknown] bug. This was fixed in 4.41.1, so upgraded. Could now do the migration. Also noticed that in the Yaml files all new lines were indeed removed and now its all on 1 line.

gsmet commented 1 year ago

I would bet it could be related to https://github.com/openrewrite/rewrite/commit/9b72d9abd9dfed058abc3b48bb8b990ab18fef20 .

I will try the version before this commit then the version after and report back.

gsmet commented 1 year ago

After some bisecting, I can confirm that https://github.com/openrewrite/rewrite/commit/9b72d9abd9dfed058abc3b48bb8b990ab18fef20 is the culprit.

timtebeek commented 1 year ago

Thanks a lot for your efforts to really pin this down; hope @kunli2 is able to use that information for a fix or workaround.

kunli2 commented 1 year ago

Thanks for all the useful findings so far, I start working on this now.

kunli2 commented 1 year ago

this is fixed by https://github.com/openrewrite/rewrite/pull/2962, I have verified the https://github.com/quarkiverse/quarkus-github-app and didn't see any unexpected changes.

gsmet commented 1 year ago

Thanks @kunli2 , I tested latest main and AFAICS the issue is gone!

Looking forward to having a version of the Maven plugin coming with the fix!