Support `@OptionGroup` on `Recipe`

[JLLeitschuh]

Enables composing Recipe of one or more objects annotated with @OptionGroup to support re-use of recipe options between multiple recipes.

Supports serialization/deserialization correctly through the use of Jackson's @JsonUnwrapped annotation.

Signed-off-by: Jonathan Leitschuh [email protected]

What's changed?

Modifies Recipe's getOptionDescriptors to support a new @NestedOption annotation.

What's your motivation?

As I'm building out recipes within rewrite-java-security I'm finding a desire to share the same configuration options between multiple recipes.

In general, for security fix recipe, you only want to apply those recipes when both test and production source is modified. Before 8.0 that heavily refactored recipes and how many source files were loaded into memory, this was possible with the anySource applicability test.

Eg:

type: specs.openrewrite.org/v1beta/recipe
name: com.example.SecurityModifyRecipe
displayName: Apply `SecureTempFileCreation`
description: Applies the `SecureTempFileCreation` to non-test sources first, if changes are made, then apply to all sources.
applicability:
  anySource:
    - org.openrewrite.java.search.IsLikelyNotTest
    - org.openrewrite.java.security.SecureTempFileCreation
recipeList:
  - org.openrewrite.java.security.SecureTempFileCreation

See: https://github.com/openrewrite/rewrite/discussions/2849

However, in 8.0 this was broken. As such, the current advice is to put this filtering into each security recipe individually.

In order to achive this, and reduce code-duplication, being able to compose recipes with a common set of options, will enable that, while reducing code duplication.

Anything in particular you'd like reviewers to focus on?

As this feature will need to integrate with the Moderne SaaS as the interface for how these new composed options are tested, I'd want to sanity check that SaaS invocation of Recipes uses jackson deserialization to instantiate them as I expect. If that is the case, then this feature should work seemlessly with the Moderne SaaS without any needed changes there to support this functionality.

This feature should have no impact on any existing recipes or how those recipes are invoked by the SaaS.

I will test and verify this functionality works as-expected by modifying the recipes in rewrite-java-security to verify this works with the Moderne SaaS as expected.

Anyone you would like to review specifically?

@sambsnyd

Have you considered any alternatives or workarounds?

A single base-class with base options doesn't work as the fields on the base-class still need to be surfaced in the constructor of the subclasses for proper deserialization by Jackson. Additionally, Recipe#getOptionDescriptors currently uses Class#getDeclaredFields which, as documented, does not include inherited fields, so would not be seen when creating the OptionDescriptor

Lots of duplicate code, including duplicating option annotations between multiple recipes. Using multiple hard-coded constants for each duplicated annotation.

This solution is more elegant IMHO.

Any additional context

Checklist

• [x] I've added unit tests to cover both positive and negative cases
• [x] I've read and applied the recipe conventions and best practices
• [x] I've used the IntelliJ IDEA auto-formatter on affected files

[JLLeitschuh]

@sambsnyd thoughts on merging this?

[knutwannheden]

I think @OptionGroup would be a better name and the tight coupling with the @JsonUnwrapped annotation is not so nice. As an alternative we could check if the @JacksonAnnotationsInside meta annotation works for us. https://fasterxml.github.io/jackson-annotations/javadoc/2.9/com/fasterxml/jackson/annotation/JacksonAnnotationsInside.html

Further, I also wonder if we should possibly move the annotation to the option group type instead. Then it would be very similar to how this all works in Quarkus with @ConfigItem and @ConfigGroup, IIRC. Although, I have no idea if this aligns with how @JsonUnwrapped works or not.

[knutwannheden]

I just checked: @JsonUnwrapped has to be declared on the property side. But maybe the meta annotation approach works?

[JLLeitschuh]

Already using @JacksonAnnotationsInside

https://github.com/openrewrite/rewrite/pull/3764/files#diff-e4085063acbc846936645bf5269d26d05193cd41027e06014841b7afa02ee32bR38

[JLLeitschuh]

Take a look at the current code and see if that aligns with your vision or not

[JLLeitschuh]

Anything else needed to get this across the finish line?

[JLLeitschuh]

@timtebeek anything we can do to get this across the finish line?

[JLLeitschuh]

@timtebeek can we push this forward?