Skip to content

convert Spring Boot application-*.properties to application-*.yaml - #1066

Open
barbulescu wants to merge 26 commits into
openrewrite:mainfrom
barbulescu:properties-2-yaml
Open

barbulescu wants to merge 26 commits into
openrewrite:mainfrom
barbulescu:properties-2-yaml

Conversation

@barbulescu

@barbulescu barbulescu commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What's changed?

Created a recipe for migrating pplication-.properties to application-.yaml.

What's your motivation?

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

Did I miss any test cases? Can it be done simpler?

Anyone you would like to review specifically?

@timtebeek

Checklist

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

@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Jul 10, 2026
@timtebeek
timtebeek self-requested a review July 13, 2026 15:24
@barbulescu
barbulescu marked this pull request as draft July 13, 2026 19:26
@barbulescu

Copy link
Copy Markdown
Contributor Author

@timtebeek I tried to cleanup a bit the initial version and I did a rebase to fix merge conflicts - please update

@barbulescu

Copy link
Copy Markdown
Contributor Author

This test started failing randomly but it does look to be related to my changes:

ReplaceExtendWithAndContextConfigurationTest > extendWithContextConfigurationKeptWhenUsingLoaderArgument() FAILED
    java.lang.IllegalStateException: LST contains missing or invalid type information
    Identifier->FieldAccess->Assignment->Annotation->ClassDeclaration->CompilationUnit
    /*~~(Identifier type is missing or malformed)~~>*/SpringBootContextLoader
    https://docs.openrewrite.org/reference/faq#im-seeing-lst-contains-missing-or-invalid-type-information-in-my-recipe-unit-tests-how-to-resolve
        at org.openrewrite.java.Assertions.assertValidTypes(Assertions.java:131)
        at org.openrewrite.java.Assertions.validateTypes(Assertions.java:68)
        at org.openrewrite.java.Assertions$$Lambda/0x000000900139d548.accept(Unknown Source)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:350)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:159)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:154)
        at org.openrewrite.java.spring.boot2.ReplaceExtendWithAndContextConfigurationTest.extendWithContextConfigurationKeptWhenUsingLoaderArgument(ReplaceExtendWithAndContextConfigurationTest.java:80)

@barbulescu
barbulescu marked this pull request as ready for review July 14, 2026 06:22
@barbulescu

Copy link
Copy Markdown
Contributor Author

@timtebeek when you have time, please take a look and let me know if it is anything I can improve here

timtebeek and others added 7 commits August 18, 2026 22:09
- Skip files split by `#---`/`!---` separators with a message, as the
  documents would otherwise merge into one mapping and later duplicate
  keys silently win
- Quote mapping keys as well as values, so keys such as `no` or `on` are
  not re-typed by YAML
- Do not delete the `.properties` file when the generated YAML fails to
  parse
- Anchor the Java reference pattern so `custom-application.properties`
  no longer blocks `application.properties`
Comment on lines +99 to +101
trackExistingYaml(acc, source);
trackCandidate(acc, source, ctx);
collectJavaReferences(acc, source);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like how you're naming the different use cases here, and split out their implementation. Thanks!

@timtebeek

Copy link
Copy Markdown
Member

Thanks again @barbulescu ; still slowly trying to work my way through a massive backlog of reviews. I've applied some initial fixes already, but also want to explore a way to reduce the complexity here by creating a flattened yaml structure first, and then apply existing recipe to (un)fold as needed for the nested structure. Might be a little while longer, not least because I have some PTO coming up. 🙏🏻

@timtebeek

Copy link
Copy Markdown
Member

I spent some time exploring whether the conversion could emit a flat YAML document (a.b.c: value) and then delegate the nesting to org.openrewrite.yaml.UnfoldProperties, instead of PropertiesToYamlConverter hand-building and rendering its own tree. Short version: it works, but it costs more code than it saves, so I'm not proposing we change this PR.

It does work. Pipeline: flatten (comment-free) → parse → UnfoldProperties → re-quote keys → re-attach comments → print. All 44 tests in PropertiesToYamlConverterTest / ConvertPropertiesToYamlTest pass unchanged — byte-identical output, including comments, prefix conflicts, escaped keys and every sequence case.

The one non-obvious trick: comments have to be withheld until after the merge. MergeDuplicateSectionsVisitor bails out of an entire mapping on the first entry prefix containing a comment (// TODO We don't support comments yet), so unfolding a commented document leaves duplicate root keys — and OriginTrackedYamlLoader calls LoaderOptions.setAllowDuplicateKeys(false), so that output would fail to load at startup.

Why I'd still keep the current renderer:

  1. It's more code, not less — 245 → 286 non-comment lines. The tree/render half goes away, but a comment re-attachment pass and a key re-quoting pass arrive, and they're bigger.
  2. Sequences stay bespoke. Nothing upstream understands my.servers[0].host, so groupSequences and the sequence rendering survive either way.
  3. Two upstream gaps have to be patched locally. UnfoldProperties.createNestedEntry hardcodes Style.PLAIN, so segments like no get re-typed and a:b breaks; and the comment issue above.
  4. The exclusions option can't express the a=1 / a.b=2 conflict precisely. It matches by segment sublist anywhere in a key, so with a=1, a.b=2, x.a.b=3 the unrelated x.a.b also stops unfolding — you get x:\n a.b: 3 instead of fully nested. Still binds the same under relaxed binding, but it's a deviation, and it isn't fixable with a better exclusion string.

So the current deterministic renderer looks like the right call to me. The reusable takeaway is the two upstream issues worth filing against rewrite-yaml (comment-aware section merging — the @ExpectedToFail mergeDuplicatedSectionsWithComments test is already the reproducer — and per-segment scalar style in createNestedEntry); if those land, this becomes worth revisiting.

@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready to Review

Development

Successfully merging this pull request may close these issues.

2 participants