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

MergeYaml does not process multiline scalar value blocks properly when multiline block is located in a subkey #4728

Merged
merged 6 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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