fix: MergeYaml block indentation not as expected

rewrite-core/src/main/java/org/openrewrite/internal/StringUtils.java

@@ -25,12 +25,12 @@
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
-import java.util.Map;
-import java.util.SortedMap;
-import java.util.TreeMap;
 import java.util.function.Predicate;
+import java.util.regex.Pattern;
 
 public class StringUtils {
+    private static final Pattern LINE_BREAK = Pattern.compile("\\R");
+
     private StringUtils() {
     }
 
@@ -158,50 +158,6 @@ private static int minCommonIndentLevel(String text) {
         return minIndent;
     }
 
-    public static int mostCommonIndent(SortedMap<Integer, Long> indentFrequencies) {
-        // the frequency with which each indent level is an integral divisor of longer indent levels
-        SortedMap<Integer, Integer> indentFrequencyAsDivisors = new TreeMap<>();
-        for (Map.Entry<Integer, Long> indentFrequency : indentFrequencies.entrySet()) {
-            int indent = indentFrequency.getKey();
-            int freq;
-            switch (indent) {
-                case 0:
-                    freq = indentFrequency.getValue().intValue();
-                    break;
-                case 1:
-                    // gcd(1, N) == 1, so we can avoid the test for this case
-                    freq = (int) indentFrequencies.tailMap(indent).values().stream().mapToLong(l -> l).sum();
-                    break;
-                default:
-                    freq = (int) indentFrequencies.tailMap(indent).entrySet().stream()
-                            .filter(inF -> gcd(inF.getKey(), indent) != 0)
-                            .mapToLong(Map.Entry::getValue)
-                            .sum();
-            }
-
-            indentFrequencyAsDivisors.put(indent, freq);
-        }
-
-        if (indentFrequencies.getOrDefault(0, 0L) > 1) {
-            return 0;
-        }
-
-        return indentFrequencyAsDivisors.entrySet().stream()
-                .max((e1, e2) -> {
-                    int valCompare = e1.getValue().compareTo(e2.getValue());
-                    return valCompare != 0 ?
-                            valCompare :
-                            // take the smallest indent otherwise, unless it would be zero
-                            e1.getKey() == 0 ? -1 : e2.getKey().compareTo(e1.getKey());
-                })
-                .map(Map.Entry::getKey)
-                .orElse(0);
-    }
-
-    static int gcd(int n1, int n2) {
-        return n2 == 0 ? n1 : gcd(n2, n1 % n2);
-    }
-
     /**
      * Check if the String is null or has only whitespaces.
      * <p>
@@ -760,4 +716,8 @@ public static int indexOfNextNonWhitespace(int cursor, String source) {
     public static String formatUriForPropertiesFile(String uri) {
         return uri.replaceAll("(?<!\\\\)://", "\\\\://");
     }
+
+    public static boolean hasLineBreak(@Nullable String s) {
+        return s != null && LINE_BREAK.matcher(s).find();
+    }
 }

rewrite-yaml/src/main/java/org/openrewrite/yaml/CoalescePropertiesVisitor.java

@@ -23,7 +23,7 @@
 import java.util.List;
 
 public class CoalescePropertiesVisitor<P> extends YamlIsoVisitor<P> {
-    private final FindIndentYamlVisitor<P> findIndent = new FindIndentYamlVisitor<>(0);
+    private final FindIndentYamlVisitor<P> findIndent = new FindIndentYamlVisitor<>();
 
     public CoalescePropertiesVisitor() {
     }
@@ -55,8 +55,7 @@ public Yaml.Mapping visitMapping(Yaml.Mapping mapping, P p) {
                         entries.add(entry.withKey(coalescedKey)
                                 .withValue(subEntry.getValue()));
 
-                        int indentToUse = findIndent.getMostCommonIndent() > 0 ?
-                                findIndent.getMostCommonIndent() : 4;
+                        int indentToUse = findIndent.getMostCommonIndent() > 0 ? findIndent.getMostCommonIndent() : 4;
                         doAfterVisit(new ShiftFormatLeftVisitor<>(subEntry.getValue(), indentToUse));
 
                         changed = true;

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

@@ -22,10 +22,6 @@
 import org.openrewrite.internal.ListUtils;
 import org.openrewrite.style.GeneralFormatStyle;
 import org.openrewrite.yaml.tree.Yaml;
-import org.openrewrite.yaml.tree.Yaml.Document;
-import org.openrewrite.yaml.tree.Yaml.Mapping;
-import org.openrewrite.yaml.tree.Yaml.Mapping.Entry;
-import org.openrewrite.yaml.tree.Yaml.Scalar;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -34,21 +30,22 @@
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
-import static java.lang.System.lineSeparator;
 import static org.openrewrite.Cursor.ROOT_VALUE;
+import static org.openrewrite.Tree.randomId;
 import static org.openrewrite.internal.ListUtils.*;
+import static org.openrewrite.internal.StringUtils.hasLineBreak;
 import static org.openrewrite.yaml.MergeYaml.REMOVE_PREFIX;
+
 /**
  * Visitor class to merge two yaml files.
  *
+ * @param <P> An input object that is passed to every visit method.
  * @implNote Loops recursively through the documents, for every part a new MergeYamlVisitor instance will be created.
  * As inline comments are put on the prefix of the next element (which can be an item very much higher in the tree),
  * the following solutions are chosen to merge the comments as well:
  * <ul>
  * <li>when an element has new items, the comment of the next element is copied to the previous element
  * <li>the original comment will be removed (either by traversing the children or by using cursor messages)
- *
- * @param <P> An input object that is passed to every visit method.
  */
 @RequiredArgsConstructor
 public class MergeYamlVisitor<P> extends YamlVisitor<P> {
@@ -88,8 +85,8 @@ public MergeYamlVisitor(Yaml scope, @Language("yml") String yamlString, boolean
                         .findFirst()
                         .map(Yaml.Documents.class::cast)
                         .map(docs -> {
-                            // Any comments will have been put on the parent Document node, preserve by copying to the mapping
-                            Document doc = docs.getDocuments().get(0);
+                            // Any comments will have been put on the parent Yaml.Document node, preserve by copying to the mapping
+                            Yaml.Document doc = docs.getDocuments().get(0);
                             if (doc.getBlock() instanceof Yaml.Mapping) {
                                 Yaml.Mapping m = (Yaml.Mapping) doc.getBlock();
                                 return m.withEntries(ListUtils.mapFirst(m.getEntries(), entry -> entry.withPrefix(doc.getPrefix())));
@@ -162,34 +159,41 @@ private boolean keyMatches(Yaml.Mapping m1, Yaml.Mapping m2) {
                 .orElse(false);
     }
 
-    private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor cursor) {
+    private Yaml.Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor cursor) {
+        // Merge same key, different value together
         List<Yaml.Mapping.Entry> mergedEntries = map(m1.getEntries(), existingEntry -> {
-            for (Entry incomingEntry : m2.getEntries()) {
+            for (Yaml.Mapping.Entry incomingEntry : m2.getEntries()) {
                 if (keyMatches(existingEntry, incomingEntry)) {
-                    return existingEntry.withValue((Yaml.Block)
-                            new MergeYamlVisitor<>(existingEntry.getValue(), incomingEntry.getValue(), acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
-                                    .visitNonNull(existingEntry.getValue(), p, new Cursor(cursor, existingEntry)));
+                    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);
+                    }
+                    Yaml mergedYaml = new MergeYamlVisitor<>(existingEntry.getValue(), value, acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
+                            .visitNonNull(existingEntry.getValue(), p, new Cursor(cursor, existingEntry));
+                    return existingEntry.withValue((Yaml.Block) mergedYaml);
                 }
             }
             return existingEntry;
         });
 
-        List<Entry> mutatedEntries = concatAll(mergedEntries, map(m2.getEntries(), it -> {
-            for (Entry existingEntry : m1.getEntries()) {
+        // Merge existing and new entries together
+        List<Yaml.Mapping.Entry> mutatedEntries = concatAll(mergedEntries, map(m2.getEntries(), it -> {
+            for (Yaml.Mapping.Entry existingEntry : m1.getEntries()) {
                 if (keyMatches(existingEntry, it)) {
                     return null;
                 }
             }
-            // workaround: autoFormat cannot handle new inserted values very well
-            if (!mergedEntries.isEmpty() && it.getValue() instanceof Yaml.Scalar && hasLineBreak(mergedEntries.get(0), 2)) {
-                return it.withPrefix(linebreak() + grabAfterFirstLineBreak(mergedEntries.get(0)));
+            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))));
             }
             return shouldAutoFormat ? autoFormat(it, p, cursor) : it;
         }));
 
+        // copy comment to previous element if needed
         if (m1.getEntries().size() < mutatedEntries.size() && !getCursor().isRoot()) {
             Cursor c = getCursor().dropParentUntil(it -> {
-                if (ROOT_VALUE.equals(it) || it instanceof Document) {
+                if (ROOT_VALUE.equals(it) || it instanceof Yaml.Document) {
                     return true;
                 }
 
@@ -205,11 +209,11 @@ private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor curso
                 return false;
             });
 
-            if (c.getValue() instanceof Document || c.getValue() instanceof Yaml.Mapping) {
+            if (c.getValue() instanceof Yaml.Document || c.getValue() instanceof Yaml.Mapping) {
                 String comment = null;
 
-                if (c.getValue() instanceof Document) {
-                    comment = ((Document) c.getValue()).getEnd().getPrefix();
+                if (c.getValue() instanceof Yaml.Document) {
+                    comment = ((Yaml.Document) c.getValue()).getEnd().getPrefix();
                 } else {
                     List<Yaml.Mapping.Entry> entries = ((Yaml.Mapping) c.getValue()).getEntries();
 
@@ -221,7 +225,7 @@ private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor curso
                         }
                     }
                     // 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), 1)) {
+                    if (comment == null && hasLineBreak(entries.get(entries.size() - 1).getPrefix())) {
                         comment = grabBeforeFirstLineBreak(entries.get(entries.size() - 1));
                     }
                 }
@@ -242,7 +246,7 @@ private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor curso
     private void removePrefixForDirectChildren(List<Yaml.Mapping.Entry> m1Entries, List<Yaml.Mapping.Entry> mutatedEntries) {
         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()) {
+                ((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))));
             }
         }
@@ -313,10 +317,6 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
         }
     }
 
-    private boolean hasLineBreak(Yaml.Mapping.Entry entry, int atLeastParts) {
-        return LINE_BREAK.matcher(entry.getPrefix()).find() && LINE_BREAK.split(entry.getPrefix()).length >= atLeastParts;
-    }
-
     private String grabBeforeFirstLineBreak(Yaml.Mapping.Entry entry) {
         String[] parts = LINE_BREAK.split(entry.getPrefix());
         return parts.length > 0 ? parts[0] : "";
@@ -327,7 +327,7 @@ private String grabAfterFirstLineBreak(Yaml.Mapping.Entry entry) {
         return parts.length > 1 ? String.join(linebreak(), Arrays.copyOfRange(parts, 1, parts.length)) : "";
     }
 
-    private Scalar mergeScalar(Yaml.Scalar y1, Yaml.Scalar y2) {
+    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;

rewrite-yaml/src/main/java/org/openrewrite/yaml/MultilineScalarChanged.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright 2024 the original author or authors.
+ * <p>
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * https://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.openrewrite.yaml;
+
+import lombok.Value;
+import lombok.With;
+import org.openrewrite.marker.Marker;
+
+import java.util.UUID;
+
+/**
+ * Multiline scalars are added directly to the tree, which leads to a wrong ident level.
+ */
+@Value
+@With
+public class MultilineScalarChanged implements Marker {
+    UUID id;
+    boolean added;
+}

rewrite-yaml/src/main/java/org/openrewrite/yaml/format/IndentsVisitor.java

@@ -19,6 +19,7 @@
 import org.openrewrite.Cursor;
 import org.openrewrite.Tree;
 import org.openrewrite.internal.StringUtils;
+import org.openrewrite.yaml.MultilineScalarChanged;
 import org.openrewrite.yaml.YamlIsoVisitor;
 import org.openrewrite.yaml.style.IndentsStyle;
 import org.openrewrite.yaml.tree.Yaml;
@@ -29,6 +30,9 @@
 import java.util.regex.Pattern;
 
 public class IndentsVisitor<P> extends YamlIsoVisitor<P> {
+
+    private static final Pattern COMMENT_PATTERN = Pattern.compile("^(\\s*)(.*\n?)", Pattern.MULTILINE);
+
     private final IndentsStyle style;
 
     @Nullable
@@ -46,7 +50,7 @@ public IndentsVisitor(IndentsStyle style, @Nullable Tree stopAfter) {
             Yaml y = c.getValue();
             String prefix = y.getPrefix();
 
-            if (prefix.contains("\n")) {
+            if (StringUtils.hasLineBreak(prefix)) {
                 int indent = findIndent(prefix);
                 if (indent != 0) {
                     c.putMessage("lastIndent", indent);
@@ -68,7 +72,7 @@ public IndentsVisitor(IndentsStyle style, @Nullable Tree stopAfter) {
 
         Yaml y = tree;
         int indent = getCursor().getNearestMessage("lastIndent", 0);
-        if (y.getPrefix().contains("\n") && !isUnindentedTopLevel()) {
+        if (StringUtils.hasLineBreak(y.getPrefix()) && !isUnindentedTopLevel()) {
             if (y instanceof Yaml.Sequence.Entry) {
                 indent = getCursor().getParentOrThrow().getMessage("sequenceEntryIndent", indent);
 
@@ -95,12 +99,24 @@ public IndentsVisitor(IndentsStyle style, @Nullable Tree stopAfter) {
                 y = y.withPrefix(indentComments(y.getPrefix(), indent));
             }
         }
+
+        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();
+            }
+
+            String newValue = ((Yaml.Scalar) y).getValue().replaceAll("\\R", "\n" + StringUtils.repeat(" ", indentValue));
+            y = ((Yaml.Scalar) y).withValue(newValue);
+        }
+
         return y;
     }
 
     private boolean isUnindentedTopLevel() {
         return getCursor().getParentOrThrow().getValue() instanceof Yaml.Document ||
-               getCursor().getParentOrThrow(2).getValue() instanceof Yaml.Document;
+                getCursor().getParentOrThrow(2).getValue() instanceof Yaml.Document;
     }
 
     @Override
@@ -120,7 +136,7 @@ private boolean isUnindentedTopLevel() {
     }
 
     private String indentTo(String prefix, int column) {
-        if (prefix.contains("\n")) {
+        if (StringUtils.hasLineBreak(prefix)) {
             int indent = findIndent(prefix);
             if (indent != column) {
                 int shift = column - indent;
@@ -130,7 +146,6 @@ private String indentTo(String prefix, int column) {
         return indentComments(prefix, column);
     }
 
-    private static final Pattern COMMENT_PATTERN = Pattern.compile("^(\\s*)(.*\n?)", Pattern.MULTILINE);
     private String indentComments(String prefix, int column) {
         // If the prefix contains a newline followed by a comment ensure the comment begins at the indentation column
         if (prefix.contains("#")) {
@@ -143,8 +158,7 @@ private String indentComments(String prefix, int column) {
                 String comment = m.group(2);
                 int newlineCount = StringUtils.countOccurrences(whitespace, "\n");
                 if (firstLine && newlineCount == 0) {
-                    if(getCursor().getValue() instanceof Yaml.Documents ||
-                       getCursor().getValue() instanceof Yaml.Document) {
+                    if (getCursor().getValue() instanceof Yaml.Documents || getCursor().getValue() instanceof Yaml.Document) {
                         // Comments on a top-level
                         result.append(indent);
                     } else {