ChangePropertyKey not respecting sequentiality

[JoseLora]

Hi folks! I don't understand sequentiality in some OpenRewrite recipes. I reproduce this simple example:

public Sequential() {
    this.doNext(new ChangePropertyKey("common.tracing", "common.execution.context", true, null, null));
    this.doNext(new DeleteProperty("common.sleuth.send", null, null, null));
    this.doNext(new ChangePropertyKey("common.sleuth", "common.tracing", true, null, null));
  }

And this test should pass, but it fails because where I expect common.tracing appears as common.execution.context. If the changes are applied sequentially, it has no sense!

void changeOnlyDesiredProperties() {
    this.rewriteRun(yaml("""
        common:
          cache:
            enabled: true
          sleuth:
            send: true
            sender:
              http:
                username: user
                password: pwd
                connectionRequestTimeout: 5000
                timeout: 3000
                readTimeout: 3000
          blockhound.enabled: true
        """,
        """
            common:
              cache:
                enabled: true
              tracing:
                sender:
                  http:
                    username: user
                    password: pwd
                    connectionRequestTimeout: 5000
                    timeout: 3000
                    readTimeout: 3000
              blockhound.enabled: true
                    """));
  }

Slack Message

[timtebeek]

We had a similar fix just last week for the Maven recipe RenamePropertyKey; Perhaps a similar approach would work here for this Yaml recipe.

Would you be interested in contributing a fix to OpenRewrite?

[JoseLora]

@timtebeek just tried a bit but it's very difficult for me to understand that recipe from scratch without more knowledge and practice... We are just using provided recipes and developing only simple custom ones, but I think this is a very difficult one.

I can provide the test to include in org.openrewrite.yaml.ChangePropertyKeyTest:

    @Test
    @Issue("https://github.com/openrewrite/rewrite/issues/3270")
    void ensureCorrectSequentiality() {
        @Language("yaml")
        String yamlRecipe = """
          ---
          type: specs.openrewrite.org/v1beta/recipe
          name: org.openrewrite.ChangePropertyKeySequentialityTest
          displayName: Test ChangePropertyKey sequentiality
          recipeList:
              - org.openrewrite.yaml.ChangePropertyKey:
                  oldPropertyKey: "common.tracing"
                  newPropertyKey: "common.execution.context"
              - org.openrewrite.yaml.DeleteProperty:
                  propertyKey: "common.sleuth.send"
              - org.openrewrite.yaml.ChangePropertyKey:
                  oldPropertyKey: "common.sleuth"
                  newPropertyKey: "common.tracing"
          """;

        rewriteRun(
          spec -> spec.recipeFromYaml(yamlRecipe, "org.openrewrite.ChangePropertyKeySequentialityTest"),
          yaml("""
              common:
                cache:
                  enabled: true
                sleuth:
                  send: true
                  sender:
                    http:
                      username: user
                      password: pwd
                      connectionRequestTimeout: 5000
                      timeout: 3000
                      readTimeout: 3000
                blockhound.enabled: true
              """,
            """
              common:
                cache:
                  enabled: true
                tracing:
                  sender:
                    http:
                      username: user
                      password: pwd
                      connectionRequestTimeout: 5000
                      timeout: 3000
                      readTimeout: 3000
                blockhound.enabled: true
                      """));
    }        

[timtebeek]

No problem at all @JoseLora ! Thanks for already providing the test; You're welcome to open up a draft pull request, even if you are unsure whether you will be able to contribute a fix. Be sure to allow commits from maintainers, and it will be easy for us to pick up from there.

[RossZhang-cpu]

Hi guys @timtebeek @JoseLora , would you mind letting me take this issue? I am willing to spend some time to investigate this one.

[RossZhang-cpu]

We had a similar fix just last week for the Maven recipe RenamePropertyKey; Perhaps a similar approach would work here for this Yaml recipe.

Would you be interested in contributing a fix to OpenRewrite?

Hi @timtebeek, It looks like there isn't same functional method(maybeUpdateModel()) in YamlVisitor.class. I am thinking of having to implemet one same functionality method for YamlVisitor. Just want to check with you whether my understanding is correct?

[timtebeek]

Hi @RossZhang-cpu ; I think that maybeUpdateModel is pretty unique to our MavenVisitor, and only needed there to update a marker containing information derived from the Maven pom.xml file contents. For Yaml our model is simpler, and does not have that concept of derived information markers that I know of, so no need to write an equivalent maybeUpdateModel.

In this case I suspect it's mostly an issue with the recipes themselves, but perhaps we can start with a draft PR containing just the above test, and explore what's needed with a debugger from there on out.

[RossZhang-cpu]

Thank you @timtebeek for classification. I raised a draft PR only containing test, please kindly help to check.

[RossZhang-cpu]

Hi @timtebeek, after debugging I found the defaut value of cycle is 2, which result into another Recipe Execution and change the property key again. Please see the snapshots below:

Here maxCycles is 2 will cause another unexpected Recipe execution.

It seems can't customize cycles value from the method entrance in current code. Do you have any clue about it?

[knutwannheden]

Yes, the test framework always runs 2 cycles. The reason is that any recipe can request that another cycle should be run, so the test framework always runs 2 cycles to make sure that the recipe behaves correctly in this scenario.

Here I believe the recipe is actually behaving as expected as the third recipe produces a new common.tracing property which in the second cycle will be transformed by the first recipe.

To avoid this the simplest solution is to set maxCycles: 1` on the YAML recipe, which I believe would be adequate in this use case.

So in summary I believe there is nothing to be fixed for this particular issue. By creating a test case and stepping through with the debugger and finding what caused this behavior I think you learned a lot about OpenRewrite and hope you don't feel your time was completely wasted and can instead apply that knowledge to look into another issue that is bothering you.

Unless you or @timtebeek see something else that needs fixing here, I suggest that we close the issue and the PR again.

[RossZhang-cpu]

Thank you @knutwannheden for sharing information. Even debugger takes me sometime, I am still feel happy to learn something new during that time.

And I have tried to set cycle to be 1 in test code, it works fine.

rewriteRun(
                spec -> {
                    spec.recipeFromYaml(yamlRecipe, "org.openrewrite.ChangePropertyKeySequentialityTest");
                    spec.cycles(1);
                    }

But may I know how to implement you mentioned before set on YAML recipe

To avoid this the simplest solution is to set maxCycles: 1` on the YAML recipe

[knutwannheden]

Since Recipe declares a maxCycles() method I was thinking we probably allow declarative YAML recipes to specify a corresponding value. But it turns out we don't. This could be an interesting addition for use cases as in this issue. Let's see what @timtebeek thinks about this.

[timtebeek]

So Knut's proposal is up for discussion in:

• https://github.com/openrewrite/rewrite/pull/3850

With that I think we've all learned something here and I think this issue can be closed. Thanks for taking a look @RossZhang-cpu ! Always good seeing how you dive into issues here.