Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MergeYaml recipe: Handle ParseError LSTs #5097

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jevanlingen
Copy link
Contributor

@jevanlingen jevanlingen commented Feb 26, 2025

What's changed?

  • The validate function can now handle wrong input yaml (it fails gracefully, as it is supposed to be)
  • The incoming field is parsed once it is needed in the MergeYaml recipe.

What's your motivation?

The MergeYaml recipe throws a ClassCastException if invalid Yaml is provided (for instance a ParseError LST). Normally the YamlIsoVisitor does only run for yaml sources:

@Override
public boolean isAcceptable(SourceFile sourceFile, P p) {
    return sourceFile instanceof Yaml.Documents;
}

But the MergeYaml recipe does already do some parsing work for the incoming field, which is executed by the JVM before the isAcceptable has been called. This PR changes this behaviour to start parsing it once it is used.

In addition, this change improves performance for all use of the MergeYaml recipe with a $ key. In that case, the incoming field is not used at all, allowing an unnecessary parse step to be skipped.

@jevanlingen jevanlingen self-assigned this Feb 26, 2025
@jevanlingen jevanlingen added bug Something isn't working recipe Requested Recipe labels Feb 26, 2025
@jevanlingen jevanlingen changed the title Parse the incoming yaml the first time we use this field MergeYaml recipe: Parse the incoming yaml the first time we use this field Feb 26, 2025
@knutwannheden
Copy link
Contributor

I am wondering if we should attempt to parse the YAML as part of validate() 🤔

@jevanlingen
Copy link
Contributor Author

jevanlingen commented Feb 26, 2025

I am wondering if we should attempt to parse the YAML as part of validate() 🤔

Good one, I totally missed that one. We actually do this already, but the current implementation of the validate method does not handle invalid yaml input either:

@Test
void handleParseError() {
    rewriteRun(
      spec -> spec
        .recipe(new MergeYaml(
          "$.some.object",
          // language=yaml
          """
            script: |-ParseError
            """,
          false,
          "name",
          null,
          null,
          null
        ))
    );
}

Throws:

java.lang.ClassCastException: class org.openrewrite.tree.ParseError cannot be cast to class org.openrewrite.yaml.tree.Yaml$Documents (org.openrewrite.tree.ParseError and org.openrewrite.yaml.tree.Yaml$Documents are in unnamed module of loader 'app')

	at org.openrewrite.yaml.MergeYaml.lambda$validate$0(MergeYaml.java:98)
	at java.base/java.util.Optional.map(Optional.java:260)
	at org.openrewrite.yaml.MergeYaml.lambda$validate$1(MergeYaml.java:98)
        ..

I will make changes accordingly, so it does not break!

Edit: Done! I kept the other changes regarding the incoming field as it is still an improvement to the original code.

@jevanlingen jevanlingen changed the title MergeYaml recipe: Parse the incoming yaml the first time we use this field MergeYaml recipe: Handle ParseError LSTs Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working recipe Requested Recipe
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants