Skip to content

Commit

Permalink
MergeYaml does not process multiline scalar value blocks properly whe…
Browse files Browse the repository at this point in the history
…n multiline block is located in a subkey (#4728)

* Add `upgradeSlackNotificationVersion2` test for MergeYaml

* Improve MergeYamlVisitor for multiline value scalars

* Revert space

* Remove linebreak helper methods from StringUtils + use more inlining

* Keep `LINE_BREAK` private in StringUtils

* Make `substringOfBeforeFirstLineBreak` and `substringOfAfterFirstLineBreak` private
  • Loading branch information
jevanlingen authored Nov 28, 2024
1 parent 287e247 commit d2c0087
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public static String trimIndent(String text) {
* @param text A string with zero or more line breaks.
* @return The minimum count of white space characters preceding each line of content.
*/
private static int minCommonIndentLevel(String text) {
public static int minCommonIndentLevel(String text) {
int minIndent = Integer.MAX_VALUE;
int whiteSpaceCount = 0;
boolean contentEncountered = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.jspecify.annotations.Nullable;
import org.openrewrite.Cursor;
import org.openrewrite.internal.ListUtils;
import org.openrewrite.internal.StringUtils;
import org.openrewrite.style.GeneralFormatStyle;
import org.openrewrite.yaml.tree.Yaml;

Expand Down Expand Up @@ -134,7 +135,8 @@ public Yaml visitMapping(Yaml.Mapping existingMapping, P p) {

if (getCursor().getMessage(REMOVE_PREFIX, false)) {
List<Yaml.Mapping.Entry> entries = ((Yaml.Mapping) getCursor().getValue()).getEntries();
return mapping.withEntries(mapLast(mapping.getEntries(), it -> it.withPrefix(linebreak() + grabAfterFirstLineBreak(entries.get(entries.size() - 1)))));
return mapping.withEntries(mapLast(mapping.getEntries(), it ->
it.withPrefix(linebreak() + substringOfAfterFirstLineBreak(entries.get(entries.size() - 1).getPrefix()))));
}

return mapping;
Expand Down Expand Up @@ -166,8 +168,8 @@ private Yaml.Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor
if (keyMatches(existingEntry, incomingEntry)) {
Yaml.Block value = incomingEntry.getValue();
if (shouldAutoFormat && incomingEntry.getValue() instanceof Yaml.Scalar && hasLineBreak(((Yaml.Scalar) value).getValue())) {
Yaml.Block newValue = value.withMarkers(value.getMarkers().add(new MultilineScalarChanged(randomId(), false)));
value = autoFormat(newValue, p);
MultilineScalarChanged marker = new MultilineScalarChanged(randomId(), false, calculateMultilineIndent(incomingEntry));
value = autoFormat(value.withMarkers(value.getMarkers().add(marker)), p);
}
Yaml mergedYaml = new MergeYamlVisitor<>(existingEntry.getValue(), value, acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
.visitNonNull(existingEntry.getValue(), p, new Cursor(cursor, existingEntry));
Expand All @@ -185,7 +187,8 @@ private Yaml.Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor
}
}
if (shouldAutoFormat && it.getValue() instanceof Yaml.Scalar && hasLineBreak(((Yaml.Scalar) it.getValue()).getValue())) {
it = it.withValue(it.getValue().withMarkers(it.getValue().getMarkers().add(new MultilineScalarChanged(randomId(), true))));
MultilineScalarChanged marker = new MultilineScalarChanged(randomId(), true, calculateMultilineIndent(it));
it = it.withValue(it.getValue().withMarkers(it.getValue().getMarkers().add(marker)));
}
return shouldAutoFormat ? autoFormat(it, p, cursor) : it;
}));
Expand Down Expand Up @@ -220,13 +223,13 @@ private Yaml.Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor
// get comment from next element in same mapping block
for (int i = 0; i < entries.size() - 1; i++) {
if (entries.get(i).getValue().equals(getCursor().getValue())) {
comment = grabBeforeFirstLineBreak(entries.get(i + 1));
comment = substringOfBeforeFirstLineBreak(entries.get(i + 1).getPrefix());
break;
}
}
// or retrieve it for last item from next element (could potentially be much higher in the tree)
if (comment == null && hasLineBreak(entries.get(entries.size() - 1).getPrefix())) {
comment = grabBeforeFirstLineBreak(entries.get(entries.size() - 1));
comment = substringOfBeforeFirstLineBreak(entries.get(entries.size() - 1).getPrefix());
}
}

Expand All @@ -247,7 +250,8 @@ private void removePrefixForDirectChildren(List<Yaml.Mapping.Entry> m1Entries, L
for (int i = 0; i < m1Entries.size() - 1; i++) {
if (m1Entries.get(i).getValue() instanceof Yaml.Mapping && mutatedEntries.get(i).getValue() instanceof Yaml.Mapping &&
((Yaml.Mapping) m1Entries.get(i).getValue()).getEntries().size() < ((Yaml.Mapping) mutatedEntries.get(i).getValue()).getEntries().size()) {
mutatedEntries.set(i + 1, mutatedEntries.get(i + 1).withPrefix(linebreak() + grabAfterFirstLineBreak(mutatedEntries.get(i + 1))));
mutatedEntries.set(i + 1, mutatedEntries.get(i + 1).withPrefix(
linebreak() + substringOfAfterFirstLineBreak(mutatedEntries.get(i + 1).getPrefix())));
}
}
}
Expand Down Expand Up @@ -317,19 +321,26 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
}
}

private String grabBeforeFirstLineBreak(Yaml.Mapping.Entry entry) {
String[] parts = LINE_BREAK.split(entry.getPrefix());
return parts.length > 0 ? parts[0] : "";
}

private String grabAfterFirstLineBreak(Yaml.Mapping.Entry entry) {
String[] parts = LINE_BREAK.split(entry.getPrefix());
return parts.length > 1 ? String.join(linebreak(), Arrays.copyOfRange(parts, 1, parts.length)) : "";
}

private Yaml.Scalar mergeScalar(Yaml.Scalar y1, Yaml.Scalar y2) {
String s1 = y1.getValue();
String s2 = y2.getValue();
return !s1.equals(s2) && !acceptTheirs ? y1.withValue(s2) : y1;
}

private String substringOfBeforeFirstLineBreak(String s) {
String[] lines = LINE_BREAK.split(s);
return lines.length > 0 ? lines[0] : "";
}

private String substringOfAfterFirstLineBreak(String s) {
String[] lines = LINE_BREAK.split(s);
return lines.length > 1 ? String.join(linebreak(), Arrays.copyOfRange(lines, 1, lines.length)) : "";
}

private int calculateMultilineIndent(Yaml.Mapping.Entry entry) {
String[] lines = LINE_BREAK.split(entry.getPrefix());
int keyIndent = (lines.length > 1 ? lines[lines.length - 1] : "").length();
int indent = StringUtils.minCommonIndentLevel(substringOfAfterFirstLineBreak(((Yaml.Scalar) entry.getValue()).getValue()));
return Math.max(indent - keyIndent, 0);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,5 @@
public class MultilineScalarChanged implements Marker {
UUID id;
boolean added;
int indent;
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@
import org.openrewrite.yaml.style.IndentsStyle;
import org.openrewrite.yaml.tree.Yaml;

import java.util.Arrays;
import java.util.Iterator;
import java.util.concurrent.atomic.AtomicReference;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class IndentsVisitor<P> extends YamlIsoVisitor<P> {

private static final Pattern LINE_BREAK = Pattern.compile("\\R");
private static final Pattern COMMENT_PATTERN = Pattern.compile("^(\\s*)(.*\\R?)", Pattern.MULTILINE);

private final IndentsStyle style;
Expand Down Expand Up @@ -100,12 +102,16 @@ public IndentsVisitor(IndentsStyle style, @Nullable Tree stopAfter) {
}
} else if (y instanceof Yaml.Scalar && y.getMarkers().findFirst(MultilineScalarChanged.class).isPresent()) {
int indentValue = indent;

if (!y.getMarkers().findFirst(MultilineScalarChanged.class).get().isAdded() && indent != 0) {
indentValue = indent + style.getIndentSize();
indentValue += style.getIndentSize();
}
indentValue += y.getMarkers().findFirst(MultilineScalarChanged.class).get().getIndent();

String[] lines = LINE_BREAK.split(((Yaml.Scalar) y).getValue());
String newValue = lines[0] +
("\n" + StringUtils.trimIndent(String.join("\n", Arrays.copyOfRange(lines, 1, lines.length))))
.replaceAll("\\R", "\n" + StringUtils.repeat(" ", indentValue));

String newValue = ((Yaml.Scalar) y).getValue().replaceAll("\\R", "\n" + StringUtils.repeat(" ", indentValue));
y = ((Yaml.Scalar) y).withValue(newValue);
}

Expand Down
72 changes: 65 additions & 7 deletions rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -1511,12 +1511,16 @@ void addLiteralStyleBlockWhichDoesAlreadyExist() {
void addLiteralStyleBlock() {
rewriteRun(
spec -> spec
.recipe(new MergeYaml("$.some.very.deep.object",
.recipe(new MergeYaml("$.some.very",
// language=yaml
"""
script: |
#!/bin/bash
echo "hello"
deep:
object:
script: | # yaml comment
#!/bin/bash
echo "hello"
echo "hello"
""",
false, "name",
null)),
Expand All @@ -1534,9 +1538,63 @@ void addLiteralStyleBlock() {
deep:
object:
with: An existing value
script: |
#!/bin/bash
echo "hello"
script: | # yaml comment
#!/bin/bash
echo "hello"
echo "hello"
""")
);
}

@Test
// Mimics `org.openrewrite.github.UpgradeSlackNotificationVersion2Test`
void upgradeSlackNotificationVersion2() {
rewriteRun(
spec -> spec
.recipe(new MergeYaml("$..steps[?(@.uses =~ 'slackapi/slack-github-action@v1.*')]",
// language=yaml
"""
with:
method: chat.postMessage
token: ${{ secrets.SLACK_MORTY_BOT_TOKEN }}
payload: |
channel: "##foo-alerts"
text: ":boom: Unable run dependency check on: <${{ steps.get_failed_check_link.outputs.failed-check-link }}|${{ inputs.organization }}/${{ inputs.repository }}>"
""",
false, "name",
null)),
yaml(
"""
jobs:
build:
steps:
- name: Send notification on error
if: failure() && inputs.send-notification
uses: slackapi/[email protected]
with:
channel-id: "##foo-alerts"
slack-message: ":boom: Unable run dependency check on: <${{ steps.get_failed_check_link.outputs.failed-check-link }}|${{ inputs.organization }}/${{ inputs.repository }}>"
env:
SLACK_BOT_TOKEN: ${{ secrets.SLACK_MORTY_BOT_TOKEN }}
""",
"""
jobs:
build:
steps:
- name: Send notification on error
if: failure() && inputs.send-notification
uses: slackapi/[email protected]
with:
channel-id: "##foo-alerts"
slack-message: ":boom: Unable run dependency check on: <${{ steps.get_failed_check_link.outputs.failed-check-link }}|${{ inputs.organization }}/${{ inputs.repository }}>"
method: chat.postMessage
token: ${{ secrets.SLACK_MORTY_BOT_TOKEN }}
payload: |
channel: "##foo-alerts"
text: ":boom: Unable run dependency check on: <${{ steps.get_failed_check_link.outputs.failed-check-link }}|${{ inputs.organization }}/${{ inputs.repository }}>"
env:
SLACK_BOT_TOKEN: ${{ secrets.SLACK_MORTY_BOT_TOKEN }}
""")
);
}
Expand Down

0 comments on commit d2c0087

Please sign in to comment.