From 3e13088784537f6dc58822851f0295a76160c5a0 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sat, 28 Sep 2024 14:47:44 -0400 Subject: [PATCH 1/7] fix --- .../com/onthegomap/planetiler/util/YAML.java | 31 +++++ .../onthegomap/planetiler/util/YamlTest.java | 125 ++++++++++++++++++ 2 files changed, 156 insertions(+) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java index 92e12e561f..3de6db23fa 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java @@ -8,6 +8,8 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.List; +import java.util.Map; import org.snakeyaml.engine.v2.api.Load; import org.snakeyaml.engine.v2.api.LoadSettings; @@ -33,12 +35,41 @@ public static T load(Path file, Class clazz) { public static T load(InputStream stream, Class clazz) { try (stream) { Object parsed = snakeYaml.loadFromInputStream(stream); + handleMergeOperator(parsed); return convertValue(parsed, clazz); } catch (IOException e) { throw new UncheckedIOException(e); } } + private static void handleMergeOperator(Object parsed) { + if (parsed instanceof Map map) { + Object toMerge = map.remove("<<"); + if (toMerge != null) { + mergeInto(map, toMerge); + } + for (var value : map.values()) { + handleMergeOperator(value); + } + } else if (parsed instanceof List list) { + for (var item : list) { + handleMergeOperator(item); + } + } + } + + private static void mergeInto(Map dest, Object source) { + if (source instanceof Map map) { + for (var entry : map.entrySet()) { + dest.putIfAbsent(entry.getKey(), entry.getValue()); + } + } else if (source instanceof List nesteds) { + for (var nested : nesteds.reversed()) { + mergeInto(dest, nested); + } + } + } + public static T load(String config, Class clazz) { try (var stream = new ByteArrayInputStream(config.getBytes(StandardCharsets.UTF_8))) { return load(stream, clazz); diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java index 59c5201bfb..655442f844 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java @@ -45,4 +45,129 @@ void testLoadLargeYamlMap() { } assertEquals(expected, YAML.load(builder.toString(), Map.class)); } + + @Test + void testMergeOperator() { + assertSameYaml(""" + source: &label + a: 1 + dest: + <<: *label + b: 2 + """, """ + source: + a: 1 + dest: + a: 1 + b: 2 + """); + } + + @Test + void testMergeOperatorNested() { + assertSameYaml(""" + source: &label + a: 1 + dest: + l1: + l2: + l3: + <<: *label + b: 2 + """, """ + source: + a: 1 + dest: + l1: + l2: + l3: + a: 1 + b: 2 + """); + } + + @Test + void testMergeOperatorOverride() { + assertSameYaml(""" + source: &label + a: 1 + dest: + <<: *label + a: 2 + """, """ + source: + a: 1 + dest: + a: 2 + """); + } + + @Test + void testMergeOperatorMultiple() { + assertSameYaml(""" + source: &label1 + a: 1 + z: 1 + source2: &label2 + a: 2 + b: 3 + dest: + <<: [*label1, *label2] + b: 4 + c: 5 + """, """ + source: + a: 1 + z: 1 + source2: + a: 2 + b: 3 + dest: + a: 2 + b: 4 + c: 5 + z: 1 + """); + } + + @Test + void testMergeNotAnchor() { + assertSameYaml(""" + <<: + a: 1 + b: 2 + b: 3 + c: 4 + """, """ + a: 1 + b: 3 + c: 4 + """); + } + + @Test + void testMergeOperatorSecond() { + assertSameYaml(""" + source: &label + a: 1 + dest: + c: 3 + <<: *label + b: 2 + """, """ + source: + a: 1 + dest: + a: 1 + b: 2 + c: 3 + """); + } + + private static void assertSameYaml(String a, String b) { + assertEquals( + YAML.load(b, Object.class), + YAML.load(a, Object.class) + ); + } } From 341c65a03081dcc62aca5fcff338e47c09cd9638 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sat, 28 Sep 2024 15:02:31 -0400 Subject: [PATCH 2/7] fix order --- .../java/com/onthegomap/planetiler/util/YAML.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java index 3de6db23fa..89b74dafd4 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java @@ -8,6 +8,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import org.snakeyaml.engine.v2.api.Load; @@ -43,10 +44,14 @@ public static T load(InputStream stream, Class clazz) { } private static void handleMergeOperator(Object parsed) { - if (parsed instanceof Map map) { + if (parsed instanceof Map map) { Object toMerge = map.remove("<<"); if (toMerge != null) { + Map orig = new LinkedHashMap<>(map); + // to preserve the map key order we insert the merged operator objects first, then the original ones + map.clear(); mergeInto(map, toMerge); + mergeInto(map, orig); } for (var value : map.values()) { handleMergeOperator(value); @@ -58,13 +63,12 @@ private static void handleMergeOperator(Object parsed) { } } + @SuppressWarnings("rawtypes") private static void mergeInto(Map dest, Object source) { if (source instanceof Map map) { - for (var entry : map.entrySet()) { - dest.putIfAbsent(entry.getKey(), entry.getValue()); - } + dest.putAll(map); } else if (source instanceof List nesteds) { - for (var nested : nesteds.reversed()) { + for (var nested : nesteds) { mergeInto(dest, nested); } } From d36dad35be08eaaa6fdb82e8506bf2d13a96ef41 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sat, 28 Sep 2024 15:04:42 -0400 Subject: [PATCH 3/7] comment --- .../src/main/java/com/onthegomap/planetiler/util/YAML.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java index 89b74dafd4..b051dc5138 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java @@ -43,6 +43,10 @@ public static T load(InputStream stream, Class clazz) { } } + /** + * SnakeYaml doesn't handle the merge operator so manually post-process + * the parsed yaml object to merge referenced objects into the parent one. + */ private static void handleMergeOperator(Object parsed) { if (parsed instanceof Map map) { Object toMerge = map.remove("<<"); From 086e284dc7c780475029bd1a3cda6e4be891a958 Mon Sep 17 00:00:00 2001 From: zstadler Date: Sat, 28 Sep 2024 22:27:55 +0200 Subject: [PATCH 4/7] Update YamlTest.java (#1041) Add tests from the [YAML Merge Key Draft](https://yaml.org/type/merge.html) --- .../onthegomap/planetiler/util/YamlTest.java | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java index 655442f844..67561f262b 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java @@ -164,6 +164,76 @@ void testMergeOperatorSecond() { """); } + @Test + void testMergeOperatorFromDraft1() { + assertSameYaml(""" + - { x: 1, y: 2 } + - { x: 0, y: 2 } + - { r: 10 } + - { r: 1 } + - # Explicit keys + x: 1 + y: 2 + r: 10 + label: center/big + """, """ + - &CENTER { x: 1, y: 2 } + - &LEFT { x: 0, y: 2 } + - &BIG { r: 10 } + - &SMALL { r: 1 } + - # Merge one map + << : *CENTER + r: 10 + label: center/big + """); + + @Test + void testMergeOperatorFromDraft2() { + assertSameYaml(""" + - { x: 1, y: 2 } + - { x: 0, y: 2 } + - { r: 10 } + - { r: 1 } + - # Explicit keys + x: 1 + y: 2 + r: 10 + label: center/big + """, """ + - &CENTER { x: 1, y: 2 } + - &LEFT { x: 0, y: 2 } + - &BIG { r: 10 } + - &SMALL { r: 1 } + - # Merge multiple maps + << : [ *CENTER, *BIG ] + label: center/big + """); + } + + @Test + void testMergeOperatorFromDraft3() { + assertSameYaml(""" + - { x: 1, y: 2 } + - { x: 0, y: 2 } + - { r: 10 } + - { r: 1 } + - # Explicit keys + x: 1 + y: 2 + r: 10 + label: center/big + """, """ + - &CENTER { x: 1, y: 2 } + - &LEFT { x: 0, y: 2 } + - &BIG { r: 10 } + - &SMALL { r: 1 } + - # Override + << : [ *BIG, *LEFT, *SMALL ] + x: 1 + label: center/big + """); + } + private static void assertSameYaml(String a, String b) { assertEquals( YAML.load(b, Object.class), From 031340795ceb3457bee14954fff008ccd0b62f2f Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sat, 28 Sep 2024 16:41:30 -0400 Subject: [PATCH 5/7] fix merge order --- planetiler-core/pom.xml | 5 +++++ .../com/onthegomap/planetiler/util/YAML.java | 16 ++++++++++------ .../com/onthegomap/planetiler/util/YamlTest.java | 9 +++++---- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/planetiler-core/pom.xml b/planetiler-core/pom.xml index eef160c5d5..0ff8b2ce2a 100644 --- a/planetiler-core/pom.xml +++ b/planetiler-core/pom.xml @@ -129,6 +129,11 @@ org.snakeyaml snakeyaml-engine + + org.yaml + snakeyaml + 2.3 + io.prometheus simpleclient diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java index b051dc5138..0a504a0ef2 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java @@ -51,11 +51,11 @@ private static void handleMergeOperator(Object parsed) { if (parsed instanceof Map map) { Object toMerge = map.remove("<<"); if (toMerge != null) { - Map orig = new LinkedHashMap<>(map); + var orig = new LinkedHashMap<>(map); // to preserve the map key order we insert the merged operator objects first, then the original ones map.clear(); - mergeInto(map, toMerge); - mergeInto(map, orig); + mergeInto(map, toMerge, false); + mergeInto(map, orig, true); } for (var value : map.values()) { handleMergeOperator(value); @@ -68,12 +68,16 @@ private static void handleMergeOperator(Object parsed) { } @SuppressWarnings("rawtypes") - private static void mergeInto(Map dest, Object source) { + private static void mergeInto(Map dest, Object source, boolean replace) { if (source instanceof Map map) { - dest.putAll(map); + if (replace) { + dest.putAll(map); + } else { + map.forEach(dest::putIfAbsent); + } } else if (source instanceof List nesteds) { for (var nested : nesteds) { - mergeInto(dest, nested); + mergeInto(dest, nested, replace); } } } diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java index 67561f262b..f28eb20b00 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java @@ -123,7 +123,7 @@ void testMergeOperatorMultiple() { a: 2 b: 3 dest: - a: 2 + a: 1 # from label1 since it came first b: 4 c: 5 z: 1 @@ -182,10 +182,11 @@ void testMergeOperatorFromDraft1() { - &BIG { r: 10 } - &SMALL { r: 1 } - # Merge one map - << : *CENTER - r: 10 - label: center/big + << : *CENTER + r: 10 + label: center/big """); + } @Test void testMergeOperatorFromDraft2() { From 948d245067723421faf6a259441b5f2b6372e7fb Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sat, 28 Sep 2024 16:54:17 -0400 Subject: [PATCH 6/7] undo --- planetiler-core/pom.xml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/planetiler-core/pom.xml b/planetiler-core/pom.xml index 0ff8b2ce2a..eef160c5d5 100644 --- a/planetiler-core/pom.xml +++ b/planetiler-core/pom.xml @@ -129,11 +129,6 @@ org.snakeyaml snakeyaml-engine - - org.yaml - snakeyaml - 2.3 - io.prometheus simpleclient From 62332215d9c1413f52c838bcddfd1bf0d48f7462 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sun, 29 Sep 2024 05:49:10 -0400 Subject: [PATCH 7/7] recursive ref tests --- .../com/onthegomap/planetiler/util/YAML.java | 30 +++++++--- .../onthegomap/planetiler/util/YamlTest.java | 60 +++++++++++++++++++ 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java index 0a504a0ef2..fa8130cc3f 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/util/YAML.java @@ -1,6 +1,7 @@ package com.onthegomap.planetiler.util; import com.fasterxml.jackson.databind.ObjectMapper; +import com.onthegomap.planetiler.reader.FileFormatException; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -8,9 +9,12 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import org.snakeyaml.engine.v2.api.Load; import org.snakeyaml.engine.v2.api.LoadSettings; @@ -43,32 +47,43 @@ public static T load(InputStream stream, Class clazz) { } } + private static void handleMergeOperator(Object parsed) { + handleMergeOperator(parsed, Collections.newSetFromMap(new IdentityHashMap<>())); + } + /** * SnakeYaml doesn't handle the merge operator so manually post-process * the parsed yaml object to merge referenced objects into the parent one. */ - private static void handleMergeOperator(Object parsed) { + private static void handleMergeOperator(Object parsed, Set parentNodes) { + if (!parentNodes.add(parsed)) { + throw new FileFormatException("Illegal recursive reference in yaml file"); + } if (parsed instanceof Map map) { Object toMerge = map.remove("<<"); if (toMerge != null) { var orig = new LinkedHashMap<>(map); // to preserve the map key order we insert the merged operator objects first, then the original ones map.clear(); - mergeInto(map, toMerge, false); - mergeInto(map, orig, true); + mergeInto(map, toMerge, false, parentNodes); + mergeInto(map, orig, true, parentNodes); } for (var value : map.values()) { - handleMergeOperator(value); + handleMergeOperator(value, parentNodes); } } else if (parsed instanceof List list) { for (var item : list) { - handleMergeOperator(item); + handleMergeOperator(item, parentNodes); } } + parentNodes.remove(parsed); } @SuppressWarnings("rawtypes") - private static void mergeInto(Map dest, Object source, boolean replace) { + private static void mergeInto(Map dest, Object source, boolean replace, Set parentNodes) { + if (!parentNodes.add(source)) { + throw new FileFormatException("Illegal recursive reference in yaml file"); + } if (source instanceof Map map) { if (replace) { dest.putAll(map); @@ -77,9 +92,10 @@ private static void mergeInto(Map dest, Object source, boolean replace) { } } else if (source instanceof List nesteds) { for (var nested : nesteds) { - mergeInto(dest, nested, replace); + mergeInto(dest, nested, replace, parentNodes); } } + parentNodes.remove(source); } public static T load(String config, Class clazz) { diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java index f28eb20b00..f2c19d414d 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/util/YamlTest.java @@ -1,7 +1,9 @@ package com.onthegomap.planetiler.util; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import com.onthegomap.planetiler.reader.FileFormatException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -235,6 +237,64 @@ void testMergeOperatorFromDraft3() { """); } + @Test + void testAnchorAndAliasMap() { + assertSameYaml(""" + source: &label + a: 1 + dest: *label + """, """ + source: + a: 1 + dest: + a: 1 + """); + } + + @Test + void testAnchorAndAliasList() { + assertSameYaml(""" + source: &label + - 1 + dest: *label + """, """ + source: [1] + dest: [1] + """); + } + + @Test + void testAllowRefInMergeDoc() { + assertSameYaml(""" + source: &label + a: &label1 + c: 1 + b: *label1 + d: + <<: *label1 + dest: *label + """, """ + source: {a: {c: 1}, b: {c: 1}, d: {c: 1}} + dest: {a: {c: 1}, b: {c: 1}, d: {c: 1}} + """); + } + + @Test + void testFailsOnRecursiveRefs() { + assertThrows(FileFormatException.class, () -> YAML.load(""" + source: &label + - *label + """, Object.class)); + assertThrows(FileFormatException.class, () -> YAML.load(""" + source: &label + <<: *label + """, Object.class)); + assertThrows(FileFormatException.class, () -> YAML.load(""" + source: &label + a: *label + """, Object.class)); + } + private static void assertSameYaml(String a, String b) { assertEquals( YAML.load(b, Object.class),