Skip to content

Commit

Permalink
When merging yaml files comments on the last element of existing bloc…
Browse files Browse the repository at this point in the history 
…k are placed at the wrong line (#4671)

* Add `existingEntryBlockWithComment` test

* Cleanup MergeYaml code

* Little progress...

* Little progress...

* improvement

* improvement

* improvement

* Fix null Yaml.Document.End objects

* improvement

* got it first working

* improvement

* improvement

* why??

* Add more logging

* Got comment working

* Improvement

* Improvement

* Found a non-working test case...

* Fix non-working test case...

* Remove test no longer needed

* Improvement

* Dont replace comments when cursor is a root level

* Put Yaml prefixes back

* More comment, more difficult

* Improvement

* Improvement of test

* Use System.lineSeparator()

* improvement

* Add `existingEntryBlockWithComment` test

* Cleanup MergeYaml code

* Little progress...

* Little progress...

* improvement

* improvement

* improvement

* Fix null Yaml.Document.End objects

* improvement

* got it first working

* improvement

* improvement

* why??

* Add more logging

* Got comment working

* Improvement

* Improvement

* Found a non-working test case...

* Fix non-working test case...

* Remove test no longer needed

* Improvement

* Dont replace comments when cursor is a root level

* Put Yaml prefixes back

* More comment, more difficult

* Improvement

* Improvement of test

* Use System.lineSeparator()

* improvement

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

Co-authored-by: Jacob van Lingen <[email protected]>

* Don't use CRLF on Windows unless that is the style of the yaml document being refactored.

---------

Co-authored-by: Sam Snyder <[email protected]>
  • Loading branch information
@jevanlingen @sambsnyd
jevanlingen and sambsnyd authored Nov 22, 2024
1 parent 01813e3 commit a586c31
Show file tree
Hide file tree
Showing 6 changed files with 370 additions and 48 deletions.
2 changes: 1 addition & 1 deletion rewrite-core/src/main/java/org/openrewrite/Cursor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public Cursor getRoot() {
/**
* @return true if this cursor is the root of the tree, false otherwise
*/
final boolean isRoot() {
final public boolean isRoot() {
return ROOT_VALUE.equals(value);
}

Expand Down
11 changes: 7 additions & 4 deletions rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ public String getDescription() {
}

final static String FOUND_MATCHING_ELEMENT = "FOUND_MATCHING_ELEMENT";
final static String REMOVE_PREFIX = "REMOVE_PREFIX";

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
Expand All @@ -96,7 +97,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
.map(docs -> {
// Any comments will have been put on the parent Document node, preserve by copying to the mapping
Yaml.Document doc = docs.getDocuments().get(0);
if(doc.getBlock() instanceof Yaml.Mapping) {
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())));
} else if (doc.getBlock() instanceof Yaml.Sequence) {
Expand All @@ -110,9 +111,11 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
@Override
public Yaml.Document visitDocument(Yaml.Document document, ExecutionContext ctx) {
if ("$".equals(key)) {
return document.withBlock((Yaml.Block) new MergeYamlVisitor<>(document.getBlock(), yaml,
Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty).visitNonNull(document.getBlock(),
ctx, getCursor()));
Yaml.Document d = document.withBlock((Yaml.Block)
new MergeYamlVisitor<>(document.getBlock(), yaml, Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty)
.visitNonNull(document.getBlock(), ctx, getCursor())
);
return getCursor().getMessage(REMOVE_PREFIX, false) ? d.withEnd(d.getEnd().withPrefix("")) : d;
}
Yaml.Document d = super.visitDocument(document, ctx);
if (d == document && !getCursor().getMessage(FOUND_MATCHING_ELEMENT, false)) {
Expand Down
197 changes: 158 additions & 39 deletions rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,47 @@
*/
package org.openrewrite.yaml;

import lombok.AllArgsConstructor;
import lombok.RequiredArgsConstructor;
import org.intellij.lang.annotations.Language;
import org.jspecify.annotations.Nullable;
import org.openrewrite.Cursor;
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;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

@AllArgsConstructor
import static java.lang.System.lineSeparator;
import static org.openrewrite.Cursor.ROOT_VALUE;
import static org.openrewrite.internal.ListUtils.*;
import static org.openrewrite.yaml.MergeYaml.REMOVE_PREFIX;
/**
* Visitor class to merge two yaml files.
*
* @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> {
private final Yaml scope;

private static final Pattern LINE_BREAK = Pattern.compile("\\R");

private final Yaml existing;
private final Yaml incoming;
private final boolean acceptTheirs;

Expand All @@ -40,15 +64,33 @@ public class MergeYamlVisitor<P> extends YamlVisitor<P> {

private boolean shouldAutoFormat = true;

public MergeYamlVisitor(Yaml.Block block, Yaml incoming, boolean acceptTheirs, @Nullable String objectIdentifyingProperty, boolean shouldAutoFormat) {
this(block, incoming, acceptTheirs, objectIdentifyingProperty);
this.shouldAutoFormat = shouldAutoFormat;
}

@Nullable
private String linebreak = null;

private String linebreak() {
if (linebreak == null) {
linebreak = Optional.ofNullable(getCursor().firstEnclosing(Yaml.Documents.class))
.map(docs -> docs.getStyle(GeneralFormatStyle.class))
.map(format -> format.isUseCRLFNewLines() ? "\r\n" : "\n")
.orElse("\n");
}
return linebreak;
}

public MergeYamlVisitor(Yaml scope, @Language("yml") String yamlString, boolean acceptTheirs, @Nullable String objectIdentifyingProperty) {
this(scope,
new YamlParser().parse(yamlString)
.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
Yaml.Document doc = docs.getDocuments().get(0);
if(doc.getBlock() instanceof Yaml.Mapping) {
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())));
} else if (doc.getBlock() instanceof Yaml.Sequence) {
Expand All @@ -64,23 +106,23 @@ public MergeYamlVisitor(Yaml scope, @Language("yml") String yamlString, boolean

@Override
public Yaml visitScalar(Yaml.Scalar existingScalar, P p) {
if (scope.isScope(existingScalar) && incoming instanceof Yaml.Scalar) {
if (existing.isScope(existingScalar) && incoming instanceof Yaml.Scalar) {
return mergeScalar(existingScalar, (Yaml.Scalar) incoming);
}
return super.visitScalar(existingScalar, p);
}

@Override
public Yaml visitSequence(Yaml.Sequence existingSeq, P p) {
if (scope.isScope(existingSeq)) {
if (existing.isScope(existingSeq)) {
if (incoming instanceof Yaml.Mapping) {
// Distribute the incoming mapping to each entry in the sequence
return existingSeq.withEntries(ListUtils.map(existingSeq.getEntries(), (i, existingSeqEntry) -> {
Yaml.Block b = (Yaml.Block) new MergeYamlVisitor<>(existingSeqEntry.getBlock(), incoming,
acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
.visitNonNull(existingSeqEntry.getBlock(), p, new Cursor(getCursor(), existingSeqEntry));
return existingSeqEntry.withBlock(b);
}));
return existingSeq.withEntries(map(existingSeq.getEntries(), (i, existingSeqEntry) ->
existingSeqEntry.withBlock((Yaml.Block)
new MergeYamlVisitor<>(existingSeqEntry.getBlock(), incoming, acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
.visitNonNull(existingSeqEntry.getBlock(), p, new Cursor(getCursor(), existingSeqEntry))
)
));
} else if (incoming instanceof Yaml.Sequence) {
return mergeSequence(existingSeq, (Yaml.Sequence) incoming, p, getCursor());
}
Expand All @@ -90,14 +132,21 @@ public Yaml visitSequence(Yaml.Sequence existingSeq, P p) {

@Override
public Yaml visitMapping(Yaml.Mapping existingMapping, P p) {
if (scope.isScope(existingMapping) && incoming instanceof Yaml.Mapping) {
return mergeMapping(existingMapping, (Yaml.Mapping) incoming, p, getCursor());
if (existing.isScope(existingMapping) && incoming instanceof Yaml.Mapping) {
Yaml.Mapping mapping = mergeMapping(existingMapping, (Yaml.Mapping) incoming, p, getCursor());

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;
}
return super.visitMapping(existingMapping, p);
}

private static boolean keyMatches(Yaml.Mapping.Entry e1, Yaml.Mapping.Entry e2) {
return e1.getKey().getValue().equals(e2.getKey().getValue());
private static boolean keyMatches(Yaml.Mapping.@Nullable Entry e1, Yaml.Mapping.@Nullable Entry e2) {
return e1 != null && e2 != null && e1.getKey().getValue().equals(e2.getKey().getValue());
}

private boolean keyMatches(Yaml.Mapping m1, Yaml.Mapping m2) {
Expand All @@ -113,33 +162,92 @@ private boolean keyMatches(Yaml.Mapping m1, Yaml.Mapping m2) {
.orElse(false);
}

private Yaml.Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor cursor) {
List<Yaml.Mapping.Entry> mutatedEntries = ListUtils.map(m1.getEntries(), existingEntry -> {
for (Yaml.Mapping.Entry incomingEntry : m2.getEntries()) {
private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor cursor) {
List<Yaml.Mapping.Entry> mergedEntries = map(m1.getEntries(), existingEntry -> {
for (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)));
return existingEntry.withValue((Yaml.Block)
new MergeYamlVisitor<>(existingEntry.getValue(), incomingEntry.getValue(), acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
.visitNonNull(existingEntry.getValue(), p, new Cursor(cursor, existingEntry)));
}
}
return existingEntry;
});

mutatedEntries = ListUtils.concatAll(mutatedEntries, ListUtils.map(m2.getEntries(), incomingEntry -> {
for (Yaml.Mapping.Entry existingEntry : m1.getEntries()) {
if (keyMatches(existingEntry, incomingEntry)) {
List<Entry> mutatedEntries = concatAll(mergedEntries, map(m2.getEntries(), it -> {
for (Entry existingEntry : m1.getEntries()) {
if (keyMatches(existingEntry, it)) {
return null;
}
}
if (shouldAutoFormat) {
incomingEntry = autoFormat(incomingEntry, p, cursor);
// 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)));
}
return incomingEntry;
return shouldAutoFormat ? autoFormat(it, p, cursor) : it;
}));

if (m1.getEntries().size() < mutatedEntries.size() && !getCursor().isRoot()) {
Cursor c = getCursor().dropParentUntil(it -> {
if (ROOT_VALUE.equals(it) || it instanceof Document) {
return true;
}

if (it instanceof Yaml.Mapping) {
List<Yaml.Mapping.Entry> entries = ((Yaml.Mapping) it).getEntries();
// last member should search further upwards until two entries are found
if (entries.get(entries.size() - 1).equals(getCursor().getParentOrThrow().getValue())) {
return false;
}
return entries.size() > 1;
}

return false;
});

if (c.getValue() instanceof Document || c.getValue() instanceof Yaml.Mapping) {
String comment = null;

if (c.getValue() instanceof Document) {
comment = ((Document) c.getValue()).getEnd().getPrefix();
} else {
List<Yaml.Mapping.Entry> entries = ((Yaml.Mapping) c.getValue()).getEntries();

// 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));
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), 1)) {
comment = grabBeforeFirstLineBreak(entries.get(entries.size() - 1));
}
}

if (comment != null) {
Yaml.Mapping.Entry last = mutatedEntries.get(mutatedEntries.size() - 1);
mutatedEntries.set(mutatedEntries.size() - 1, last.withPrefix(comment + last.getPrefix()));
c.putMessage(REMOVE_PREFIX, true);
}
}
}

removePrefixForDirectChildren(m1.getEntries(), mutatedEntries);

return m1.withEntries(mutatedEntries);
}

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()) {
mutatedEntries.set(i + 1, mutatedEntries.get(i + 1).withPrefix(linebreak() + grabAfterFirstLineBreak(mutatedEntries.get(i + 1))));
}
}
}

private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cursor cursor) {
if (acceptTheirs) {
return s1;
Expand All @@ -163,22 +271,20 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
}
}

return s1.withEntries(ListUtils.concatAll(s1.getEntries(),
ListUtils.map(incomingEntries, incomingEntry -> autoFormat(incomingEntry, p, cursor))));
return s1.withEntries(concatAll(s1.getEntries(), map(incomingEntries, it -> autoFormat(it, p, cursor))));
} else {
if (objectIdentifyingProperty == null) {
// No identifier set to match entries on, so cannot continue
return s1;
} else {
List<Yaml.Sequence.Entry> mutatedEntries = ListUtils.map(s2.getEntries(), entry -> {
List<Yaml.Sequence.Entry> mutatedEntries = map(s2.getEntries(), entry -> {
Yaml.Mapping incomingMapping = (Yaml.Mapping) entry.getBlock();
for (Yaml.Sequence.Entry existingEntry : s1.getEntries()) {
Yaml.Mapping existingMapping = (Yaml.Mapping) existingEntry.getBlock();
if (keyMatches(existingMapping, incomingMapping)) {
Yaml.Sequence.Entry e1 = existingEntry.withBlock(mergeMapping(existingMapping, incomingMapping, p, cursor));
if(e1 == existingEntry) {
if (e1 == existingEntry) {
// Made no change, no need to consider the entry "mutated"
//noinspection DataFlowIssue
return null;
}
return e1;
Expand All @@ -190,10 +296,9 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
return s1;
}

List<Yaml.Sequence.Entry> entries = ListUtils.concatAll(
s1.getEntries().stream().filter(entry -> !mutatedEntries.contains(entry))
.collect(Collectors.toList()),
ListUtils.map(mutatedEntries, entry -> autoFormat(entry, p, cursor)));
List<Yaml.Sequence.Entry> entries = concatAll(
s1.getEntries().stream().filter(entry -> !mutatedEntries.contains(entry)).collect(Collectors.toList()),
map(mutatedEntries, entry -> autoFormat(entry, p, cursor)));

if (entries.size() != s1.getEntries().size()) {
return s1.withEntries(entries);
Expand All @@ -208,7 +313,21 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
}
}

private Yaml.Scalar mergeScalar(Yaml.Scalar y1, Yaml.Scalar y2) {
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] : "";
}

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 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;
Expand Down
Loading

0 comments on commit a586c31

Please sign in to comment.