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

Declarative Preconditions - or association #4003

Closed
nmck257 opened this issue Feb 14, 2024 · 7 comments
Closed

Declarative Preconditions - or association #4003

nmck257 opened this issue Feb 14, 2024 · 7 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@nmck257
Copy link
Collaborator

nmck257 commented Feb 14, 2024

What problem are you trying to solve?

Declarative preconditions currently associate via AND if you declare multiple on the same recipe.
If you want to achieve an OR relationship, you can do it, but through a much more verbose "diamond" pattern.

Sample:

---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething
displayName: Do Something
recipeList:
  - org.sample.DoSomething.condition.1
  - org.sample.DoSomething.condition.2
  - org.sample.DoSomething.condition.3

---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething.condition.1
displayName: Do Something (condition 1)
preconditions:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/my.json"
recipeList:
  - org.sample.DoSomething.for.real

---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething.condition.2
displayName: Do Something (condition 2)
preconditions:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/your.json"
recipeList:
  - org.sample.DoSomething.for.real  

---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething.condition.3
displayName: Do Something (condition 3)
preconditions:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/our.json"
recipeList:
  - org.sample.DoSomething.for.real

---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething.for.real
displayName: Do Something (for real)
recipeList:
  - org.openrewrite.json.ChangeKey:
      qwe: qwe

(if you imagine graphing how those 5 recipes relate to each other, it kinda looks like a diamond)

Describe the solution you'd like

---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething
displayName: Do Something
preconditionAssociation: OR
preconditions:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/my.json"
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/your.json"
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/our.json"
recipeList:
  - org.openrewrite.json.ChangeKey:
      qwe: qwe

Have you considered any alternatives or workarounds?

Support for passing nested recipes as recipe arguments in declarative recipes? :)
This could allow something like:

---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething
displayName: Do Something
preconditionAssociation: OR
preconditions:
  - org.openrewrite.Preconditions.Or:
      recipeList:
        - org.openrewrite.FindSourceFiles:
            filePattern: "**/my.json"
        - org.openrewrite.FindSourceFiles:
            filePattern: "**/your.json"
        - org.openrewrite.FindSourceFiles:
            filePattern: "**/our.json"
recipeList:
  - org.openrewrite.json.ChangeKey:
      qwe: qwe

Additional context

cc @sambsnyd

Are you interested in contributing this feature to OpenRewrite?

Assuming we can bless a design, yeah, I suspect the code won't be too treacherous

@nmck257 nmck257 added the enhancement New feature or request label Feb 14, 2024
@timtebeek timtebeek added the question Further information is requested label Feb 29, 2024
@timtebeek timtebeek removed the question Further information is requested label Aug 9, 2024
@timtebeek
Copy link
Contributor

Up to now we've mostly tried to keep logic out of the declarative recipes; one approach I haven't seen listed above is this alternative, non-diamond:

type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething
displayName: Do Something
preconditions:
  - org.sample.FindAnyJson
recipeList:
  - org.openrewrite.json.ChangeKey:
      qwe: qwe
---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.FindAnyJson
recipeList:
  - org.sample.FindMyJson
  - org.sample.FindYourJson
  - org.sample.FindOurJson
---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.FindMyJson
recipeList:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/my.json"
---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.FindYourJson
recipeList:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/your.json"
---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.FindOurJson
recipeList:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/our.json"

(I have not run this example, but I think it should work)

This approach would be something akin to a rake; still somewhat verbose, but at least no duplication in the actual work that ought to be done. Could this be a suitable (and then also documented) alternative to having an explicit OR associativity?

@timtebeek timtebeek added the documentation Improvements or additions to documentation label Aug 9, 2024
@nmck257
Copy link
Collaborator Author

nmck257 commented Aug 10, 2024

Oh, I think the rake could be even simpler:

type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething
displayName: Do Something
preconditions:
  - org.sample.FindAnyJson
recipeList:
  - org.openrewrite.json.ChangeKey:
      qwe: qwe
---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.FindAnyJson
recipeList:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/my.json"
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/your.json"
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/our.json"

At that point, yeah, documenting that pattern feels like a pretty strong alternative to the original ideas in this issue.

@timtebeek
Copy link
Contributor

That's even better indeed; saves a couple of exposed recipes too.

@mike-solomon would you be able to document Nick's pattern for folks that want any precondition to match, as opposed to all?

That would then close this issue without any code changes.

mike-solomon added a commit to openrewrite/rewrite-docs that referenced this issue Aug 12, 2024
@mike-solomon
Copy link
Contributor

That's even better indeed; saves a couple of exposed recipes too.

@mike-solomon would you be able to document Nick's pattern for folks that want any precondition to match, as opposed to all?

That would then close this issue without any code changes.

Yep - I expanded the precondition documentation here: openrewrite/rewrite-docs@8499db8 -- please let me know if that makes sense or if I should rephrase/add additional information.

@timtebeek
Copy link
Contributor

Yes looks good, thanks!

@samuliwritescode
Copy link

Hi all! Unfortunately the documented example above does not work. Used here the rewrite-maven-plugin version 5.39.2 to test this and it looks like if the precondition is in another recipe, then the precondition is not hit at all. For example this simplified case with only one precondition does not change anything in my.json:

type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething
displayName: Do Something
preconditions:
  - org.sample.FindAnyJson
recipeList:
  - org.openrewrite.json.ChangeKey:
      oldKeyPath: $.qwe
      newKey: ewq
---
type: specs.openrewrite.org/v1beta/recipe
name: org.sample.FindAnyJson
recipeList:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/my.json"

while this does:

type: specs.openrewrite.org/v1beta/recipe
name: org.sample.DoSomething
displayName: Do Something
preconditions:
  - org.openrewrite.FindSourceFiles:
      filePattern: "**/my.json"
recipeList:
  - org.openrewrite.json.ChangeKey:
      oldKeyPath: $.qwe
      newKey: ewq

@timtebeek
Copy link
Contributor

Thanks for pointing that out @samuliwritescode ; @DidierLoiseau was kind enough to start this effort to see this fixed:

timtebeek added a commit that referenced this issue Sep 20, 2024
…y work (#4505)

* Added tests for declarative recipes as preconditions

Includes the example from the documentation.

* Allow declarative recipes to be used as preconditions

* Remove arguments to `toArray`

* Remove DeclarativeRecipe specificity

* Improve formatting

---------

Co-authored-by: Tim te Beek <tim@moderne.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

4 participants