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

feat: Added recipe to comment deprecated and removed properties witho… #635

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

Conversation

ashakirin
Copy link
Contributor

@ashakirin ashakirin commented Nov 22, 2024

Added recipe to comment deprecated and removed properties without alternative

ashakirin and others added 6 commits November 22, 2024 16:24
…ropertiesTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ropertiesTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…roperties.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…roperties.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ropertiesTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek self-requested a review November 23, 2024 15:27
@timtebeek timtebeek added the recipe Recipe requested label Nov 23, 2024
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the addition already @ashakirin ! I spoke to @BoykoAlex last Thursday who had been looking for something similar for removed properties. I think this could factor in to the solution there.

As a bit of background we automatically generate recipes for properties that have moved, based on meta data available in the Spring Boot jars, as seen here:

Files.writeString(config, """
# This file is automatically generated by the GeneratePropertiesMigratorConfiguration class.
# Do not edit this file manually. Update the Spring Boot property metadata upstream instead.
---
type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.spring.boot%1$s.SpringBootProperties_%1$s_%2$s
displayName: Migrate Spring Boot properties to %1$s.%2$s
description: Migrate properties found in `application.properties` and `application.yml`.
tags:
- spring
- boot
recipeList:""".formatted(majorMinor[0], majorMinor[1]),
StandardOpenOption.APPEND);
Files.writeString(config, replacements.stream()
.map(r -> """
- org.openrewrite.java.spring.ChangeSpringPropertyKey:
oldPropertyKey: %s
newPropertyKey: %s
""".formatted(
r.name(), requireNonNull(r.deprecation()).replacement())
)
.collect(joining("", "\n", "\n")),
StandardOpenOption.APPEND);

I think we can similarly generate recipes that add comments for meta data without a listed replacement, as for instance seen here:
https://github.com/spring-projects/spring-boot/blob/32433e84f3048c397aef3c3a8d63a0ae1ca7d5fc/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/resources/META-INF/additional-spring-configuration-metadata.json#L351-L359

If we are to go that route then there is no need for this recipe to include a List<String>, as matching a single key is good enough when we generate the recipes.

@BoykoAlex what are your thoughts on this?

@BoykoAlex
Copy link
Contributor

@timtebeek @ashakirin I think I'm inclined to look at the recipe generation first and then come up with the appropriate shape of the recipe putting a comment. I don't know if Spring Boot Properties metadata json contains info about removed properties... Perhaps we'd need to compute removed properties based on X.Y and X.Y+1 metadata comparison? Not sure... Anyway, if it is simpler to consume removed properties one-by-one from the generator code then a single property key parameter. If it comes in as a list/collection already then a list of keys is good. To be honest a list parameter has wider usage options and likely more performant so I'd say I'm fine with its current shape... This recipe feels like its home should be rewrite-properties project under rewrite repo...

@timtebeek
Copy link
Contributor

Thanks for the reply! I'd prefer to keep it here for now, as it uses ChangeSpringPropertyValue which has some Spring specific matching, and works across both Yaml and Properties.

As for removed properties I think those are called out as errors without replacements, sometimes with a reason, as shown on the link above. When there's a reason we can add that inline; if there's none then the error at least indicates a user should have a look to this likely removal.

@ashakirin
Copy link
Contributor Author

I would like to split it in some small steps:

  1. add recipes for inline comments (current PR)
  2. add inline comment for deprecated properties without replacements, like @timtebeek suggested (seems to be easy)
  3. analyse options for removed properties: when we can recognize them using deprecations with errors, should be trivial as well. For removed properties we can even comment the property itself + add inline comment or just leave inline comment in other format, WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants