Skip to content

Commit 34acf1c

Browse files
timtebeektimtebeek
authored
Fix MergeYaml wildcard, flow-style sequence, and comment handling (#7291)
* Fix MergeYaml issues with wildcards, flow-style sequences, and trailing comments - Fix JsonPathMatcher wildcard handling to expand trailing wildcards (e.g. `$.list.services.*`) to individual child entries (#3310) - Fix MergeYamlVisitor.mergeSequence() to handle flow-style sequences by using comma separators instead of newline-based indent (#2834) - Fix MergeYamlVisitor.mergeMapping() to preserve inline comments stored on Document.End when appending to deeply nested mappings (#5135) - Add documenting test for DeleteKey + MergeYaml 2-cycle behavior (#3950) - Add confirmation test for fixed multi-document merge (#3539) * Fix flow-style sequence merging to handle commas and prefixes correctly The YAML AST stores flow-style separators as trailingCommaPrefix on entries and spaces as the scalar block's prefix, not the entry's prefix. Updated mergeSequence() to correctly set trailingCommaPrefix on the last existing entry and the block prefix on new entries. Added tests for merging multi-element flow-style sequences and sequences where some incoming values already exist.
1 parent 456976e commit 34acf1c

3 files changed

Lines changed: 296 additions & 4 deletions

File tree

‎rewrite-yaml/src/main/java/org/openrewrite/yaml/JsonPathMatcher.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,9 @@ public Object visitIndexes(JsonPathParser.IndexesContext ctx) {
447447
return mapping.getEntries();
448448
} else if (scope instanceof Yaml.Mapping.Entry) {
449449
Yaml.Mapping.Entry member = (Yaml.Mapping.Entry) scope;
450+
if (member.getValue() instanceof Yaml.Mapping) {
451+
return ((Yaml.Mapping) member.getValue()).getEntries();
452+
}
450453
return member.getValue();
451454
} else if (scope instanceof Yaml.Sequence) {
452455
List<Object> matches = new ArrayList<>();
@@ -474,7 +477,14 @@ public Object visitIndexes(JsonPathParser.IndexesContext ctx) {
474477
List<Object> matches = new ArrayList<>();
475478
if (stop != null && stop == getExpressionContext(ctx)) {
476479
// Return the values of each result when the JsonPath ends with a wildcard.
477-
results.forEach(o -> matches.add(getValue(o)));
480+
for (Object o : results) {
481+
if (o instanceof List) {
482+
// Already expanded (e.g., entries from a Mapping), flatten directly.
483+
matches.addAll((List<Object>) o);
484+
} else {
485+
matches.add(getValue(o));
486+
}
487+
}
478488
} else {
479489
// Unwrap lists of results from visitProperty to match the position of the cursor.
480490
for (Object result : results) {

‎rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -234,16 +234,33 @@ private Yaml.Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor
234234
List<Yaml.Mapping.Entry> entries = ((Yaml.Mapping) c.getValue()).getEntries();
235235

236236
// Get comment from next element in same mapping block
237+
boolean foundDirectSibling = false;
237238
for (int i = 0; i < entries.size() - 1; i++) {
238239
if (entries.get(i).getValue().equals(getCursor().getValue())) {
239240
comment = substringOfBeforeFirstLineBreak(entries.get(i + 1).getPrefix());
241+
foundDirectSibling = true;
240242
break;
241243
}
242244
}
243245
// OR retrieve it for last item from next element (could potentially be much higher in the tree).
244246
if (comment == null && hasLineBreak(entries.get(entries.size() - 1).getPrefix())) {
245247
comment = substringOfBeforeFirstLineBreak(entries.get(entries.size() - 1).getPrefix());
246248
}
249+
250+
// If the current mapping is not a direct child of the found parent mapping,
251+
// fall back to the Document.End prefix. This handles the case where the mapping
252+
// being merged is deeply nested (e.g., inside a sequence entry) and the inline
253+
// comment is stored on the Document.End node.
254+
if (!foundDirectSibling && !isNotEmpty(comment)) {
255+
Cursor docCursor = c.dropParentUntil(it -> ROOT_VALUE.equals(it) || it instanceof Yaml.Document);
256+
if (docCursor.getValue() instanceof Yaml.Document) {
257+
Yaml.Document doc = docCursor.getValue();
258+
if (!preserveDocumentSeparator(doc)) {
259+
comment = doc.getEnd().getPrefix();
260+
c = docCursor;
261+
}
262+
}
263+
}
247264
}
248265

249266
if (isNotEmpty(comment)) {
@@ -293,9 +310,16 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
293310
}
294311
}
295312

296-
String existingEntryPrefix = s1.getEntries().get(0).getPrefix();
297-
String currentIndent = existingEntryPrefix.substring(existingEntryPrefix.lastIndexOf('\n'));
298-
List<Yaml.Sequence.Entry> newEntries = ListUtils.map(incomingEntries, it -> it.withPrefix(currentIndent));
313+
boolean isFlowStyle = s1.getOpeningBracketPrefix() != null;
314+
List<Yaml.Sequence.Entry> newEntries;
315+
if (isFlowStyle) {
316+
newEntries = ListUtils.map(incomingEntries, it ->
317+
it.withPrefix("").withBlock(it.getBlock().withPrefix(" ")).withTrailingCommaPrefix(null));
318+
} else {
319+
String existingEntryPrefix = s1.getEntries().get(0).getPrefix();
320+
String newEntryPrefix = existingEntryPrefix.substring(existingEntryPrefix.lastIndexOf('\n'));
321+
newEntries = ListUtils.map(incomingEntries, it -> it.withPrefix(newEntryPrefix));
322+
}
299323
List<Yaml.Sequence.Entry> mutatedEntries = concatAll(s1.getEntries(), newEntries, it -> {
300324
if (it.getBlock() instanceof Yaml.Scalar) {
301325
return ((Yaml.Scalar) it.getBlock()).getValue();
@@ -306,6 +330,15 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
306330
return "";
307331
}).ls;
308332

333+
// For flow-style sequences, ensure commas are correct: add a trailing comma
334+
// on the entry before the first new entry, and remove any trailing comma from the last entry
335+
if (isFlowStyle && !newEntries.isEmpty() && mutatedEntries.size() > s1.getEntries().size()) {
336+
int lastExistingIdx = s1.getEntries().size() - 1;
337+
mutatedEntries.set(lastExistingIdx, mutatedEntries.get(lastExistingIdx).withTrailingCommaPrefix(""));
338+
int lastIdx = mutatedEntries.size() - 1;
339+
mutatedEntries.set(lastIdx, mutatedEntries.get(lastIdx).withTrailingCommaPrefix(null));
340+
}
341+
309342
return s1.withEntries(mutatedEntries);
310343
}
311344

‎rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java‎

Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,11 @@
1919
import org.openrewrite.DocumentExample;
2020
import org.openrewrite.Issue;
2121
import org.openrewrite.Validated;
22+
import org.openrewrite.config.CompositeRecipe;
2223
import org.openrewrite.test.RewriteTest;
2324

25+
import java.util.List;
26+
2427
import static org.assertj.core.api.Assertions.assertThat;
2528
import static org.openrewrite.yaml.Assertions.yaml;
2629
import static org.openrewrite.yaml.MergeYaml.InsertMode.After;
@@ -3503,4 +3506,250 @@ void insertBeforeAfterPlainScalar() {
35033506
)
35043507
);
35053508
}
3509+
3510+
@Issue("https://github.com/openrewrite/rewrite/issues/3950")
3511+
@Test
3512+
void deleteKeyFollowedByMergeYamlCompletesInTwoCycles() {
3513+
rewriteRun(
3514+
spec -> spec
3515+
.expectedCyclesThatMakeChanges(2)
3516+
.recipe(
3517+
new CompositeRecipe(
3518+
List.of(
3519+
new DeleteKey("$.*", null),
3520+
new MergeYaml("$", "foo: bar", null, null, "test.yml", null, null, null)
3521+
)
3522+
)
3523+
),
3524+
yaml(
3525+
"""
3526+
x: y
3527+
""",
3528+
"""
3529+
foo: bar
3530+
""",
3531+
spec -> spec.path("test.yml")
3532+
)
3533+
);
3534+
}
3535+
3536+
@Issue("https://github.com/openrewrite/rewrite/issues/3539")
3537+
@Test
3538+
void mergeExistingKeyInMultiDocumentYaml() {
3539+
rewriteRun(
3540+
spec -> spec.recipe(new MergeYaml(
3541+
"$",
3542+
//language=yaml
3543+
"""
3544+
app:
3545+
core:
3546+
key2: value02
3547+
""",
3548+
false,
3549+
null,
3550+
null,
3551+
null,
3552+
null,
3553+
null
3554+
)),
3555+
yaml(
3556+
"""
3557+
app:
3558+
app.key: app
3559+
---
3560+
com:
3561+
key1: value1
3562+
app:
3563+
core:
3564+
key1: value01
3565+
""",
3566+
"""
3567+
app:
3568+
app.key: app
3569+
core:
3570+
key2: value02
3571+
---
3572+
com:
3573+
key1: value1
3574+
app:
3575+
core:
3576+
key1: value01
3577+
key2: value02
3578+
"""
3579+
)
3580+
);
3581+
}
3582+
3583+
@Issue("https://github.com/openrewrite/rewrite/issues/3310")
3584+
@Test
3585+
void wildcardMatchesEachChildMapping() {
3586+
rewriteRun(
3587+
spec -> spec.recipe(new MergeYaml(
3588+
"$.list.services.*",
3589+
//language=yaml
3590+
"beta: fixedValue",
3591+
false,
3592+
null,
3593+
null,
3594+
null,
3595+
null,
3596+
null
3597+
)),
3598+
yaml(
3599+
"""
3600+
list:
3601+
services:
3602+
foo:
3603+
alpha: randomValue1
3604+
bar:
3605+
alpha: randomValue2
3606+
""",
3607+
"""
3608+
list:
3609+
services:
3610+
foo:
3611+
alpha: randomValue1
3612+
beta: fixedValue
3613+
bar:
3614+
alpha: randomValue2
3615+
beta: fixedValue
3616+
"""
3617+
)
3618+
);
3619+
}
3620+
3621+
@Issue("https://github.com/openrewrite/rewrite/issues/2834")
3622+
@Test
3623+
void mergeFlowStyleSequence() {
3624+
rewriteRun(
3625+
spec -> spec.recipe(new MergeYaml(
3626+
"$.value",
3627+
//language=yaml
3628+
"[18]",
3629+
false,
3630+
null,
3631+
null,
3632+
null,
3633+
null,
3634+
null
3635+
)),
3636+
yaml(
3637+
"""
3638+
value: [17]
3639+
""",
3640+
"""
3641+
value: [17, 18]
3642+
"""
3643+
)
3644+
);
3645+
}
3646+
3647+
@Issue("https://github.com/openrewrite/rewrite/issues/2834")
3648+
@Test
3649+
void mergeFlowStyleSequenceMultipleIncoming() {
3650+
rewriteRun(
3651+
spec -> spec.recipe(new MergeYaml(
3652+
"$.value",
3653+
//language=yaml
3654+
"[17, 18]",
3655+
false,
3656+
null,
3657+
null,
3658+
null,
3659+
null,
3660+
null
3661+
)),
3662+
yaml(
3663+
"""
3664+
value: [17]
3665+
""",
3666+
"""
3667+
value: [17, 18]
3668+
"""
3669+
)
3670+
);
3671+
}
3672+
3673+
@Issue("https://github.com/openrewrite/rewrite/issues/2834")
3674+
@Test
3675+
void mergeFlowStyleSequenceNewBeforeExisting() {
3676+
rewriteRun(
3677+
spec -> spec.recipe(new MergeYaml(
3678+
"$.value",
3679+
//language=yaml
3680+
"[16, 17]",
3681+
false,
3682+
null,
3683+
null,
3684+
null,
3685+
null,
3686+
null
3687+
)),
3688+
yaml(
3689+
"""
3690+
value: [17]
3691+
""",
3692+
"""
3693+
value: [17, 16]
3694+
"""
3695+
)
3696+
);
3697+
}
3698+
3699+
@Issue("https://github.com/openrewrite/rewrite/issues/2834")
3700+
@Test
3701+
void mergeFlowStyleSequenceNoDuplicate() {
3702+
rewriteRun(
3703+
spec -> spec.recipe(new MergeYaml(
3704+
"$.value",
3705+
//language=yaml
3706+
"[17]",
3707+
false,
3708+
null,
3709+
null,
3710+
null,
3711+
null,
3712+
null
3713+
)),
3714+
yaml(
3715+
"""
3716+
value: [17]
3717+
"""
3718+
)
3719+
);
3720+
}
3721+
3722+
@Issue("https://github.com/openrewrite/rewrite/issues/5135")
3723+
@Test
3724+
void mergeYamlPreservesInlineCommentOnDocumentEnd() {
3725+
rewriteRun(
3726+
spec -> spec.recipe(new MergeYaml(
3727+
"$..containers",
3728+
//language=yaml
3729+
"imagePullPolicy: Always",
3730+
false,
3731+
null,
3732+
null,
3733+
null,
3734+
null,
3735+
null
3736+
)),
3737+
yaml(
3738+
"""
3739+
kind: Pod
3740+
spec:
3741+
containers:
3742+
- name: <container name> # comment
3743+
""",
3744+
"""
3745+
kind: Pod
3746+
spec:
3747+
containers:
3748+
- name: <container name> # comment
3749+
imagePullPolicy: Always
3750+
"""
3751+
)
3752+
);
3753+
}
3754+
35063755
}

0 commit comments

Comments
 (0)
⚡