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

[Port from snakeyaml-engine] Allow to avoid dumping anchors #272

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
2 changes: 2 additions & 0 deletions api/snakeyaml-engine-kmp.api
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public final class it/krzeminski/snakeyaml/engine/kmp/api/DumpSettings {
public static final fun builder ()Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
public final fun getCustomProperty (Lit/krzeminski/snakeyaml/engine/kmp/api/SettingKey;)Ljava/lang/Object;
public final fun isCanonical ()Z
public final fun isDereferenceAliases ()Z
public final fun isExplicitEnd ()Z
public final fun isExplicitStart ()Z
public final fun isMultiLineFlow ()Z
Expand All @@ -57,6 +58,7 @@ public final class it/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder {
public final fun setCustomProperty (Lit/krzeminski/snakeyaml/engine/kmp/api/SettingKey;Ljava/lang/Object;)Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
public final fun setDefaultFlowStyle (Lit/krzeminski/snakeyaml/engine/kmp/common/FlowStyle;)Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
public final fun setDefaultScalarStyle (Lit/krzeminski/snakeyaml/engine/kmp/common/ScalarStyle;)Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
public final fun setDereferenceAliases (Z)Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
public final fun setDumpComments (Z)Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
public final fun setExplicitEnd (Z)Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
public final fun setExplicitRootTag (Lit/krzeminski/snakeyaml/engine/kmp/nodes/Tag;)Lit/krzeminski/snakeyaml/engine/kmp/api/DumpSettingsBuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class DumpSettings internal constructor(
private val customProperties: Map<SettingKey, Any>,
@JvmField val indentWithIndicator: Boolean,
@JvmField val dumpComments: Boolean,
val isDereferenceAliases: Boolean,
) {

fun getCustomProperty(key: SettingKey): Any? = customProperties[key]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ class DumpSettingsBuilder internal constructor() {
private var maxSimpleKeyLength = 128
private var indentWithIndicator = false
private var dumpComments = false
private var isDereferenceAliases = false
private var schema: Schema = JsonSchema()

/**
Expand Down Expand Up @@ -328,6 +329,23 @@ class DumpSettingsBuilder internal constructor() {
return this
}

/**
*
* Disable usage of anchors and aliases while serialising an instance. Recursive objects will not
* work when they are disabled. (Forces Serializer to skip emitting anchors names, emit Node
* content instead of Alias, fail with SerializationException if serialized structure is
* recursive.)
*
* @param dereferenceAliases - true to use copies of objects instead of references to the same
* instance
* @return the builder with the provided value
*/
fun setDereferenceAliases(dereferenceAliases: Boolean): DumpSettingsBuilder {
this.isDereferenceAliases = dereferenceAliases
return this
}


/**
* Create immutable DumpSettings
*
Expand Down Expand Up @@ -358,6 +376,7 @@ class DumpSettingsBuilder internal constructor() {
customProperties = customProperties,
indentWithIndicator = indentWithIndicator,
dumpComments = dumpComments,
isDereferenceAliases = isDereferenceAliases,
)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package it.krzeminski.snakeyaml.engine.kmp.serializer

import it.krzeminski.snakeyaml.engine.kmp.internal.IdentityHashCode
import it.krzeminski.snakeyaml.engine.kmp.internal.identityHashCode

/**
* A set that compares objects by their identities, not values.
* It's an attempt to reimplement `Collections.newSetFromMap(new IdentityHashMap<Node, Boolean>())`
* from the JVM.
*/
internal class IdentitySet<T> {
private val contents: MutableSet<IdentityHashCode> = mutableSetOf()

fun add(obj: T) {
contents.add(identityHashCode(obj))
}

fun contains(obj: T): Boolean {
return contents.contains(identityHashCode(obj))
}

fun clear() {
contents.clear()
}

fun remove(obj: T) {
contents.remove(identityHashCode(obj))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import it.krzeminski.snakeyaml.engine.kmp.comments.CommentLine
import it.krzeminski.snakeyaml.engine.kmp.common.Anchor
import it.krzeminski.snakeyaml.engine.kmp.emitter.Emitable
import it.krzeminski.snakeyaml.engine.kmp.events.*
import it.krzeminski.snakeyaml.engine.kmp.exceptions.YamlEngineException
import it.krzeminski.snakeyaml.engine.kmp.nodes.*

/**
Expand All @@ -33,6 +34,8 @@ class Serializer(
) {
private val serializedNodes: MutableSet<Node> = mutableSetOf()
private val anchors: MutableMap<Node, Anchor?> = mutableMapOf()
private val isDereferenceAliases: Boolean = settings.isDereferenceAliases
private val recursive: IdentitySet<Node> = IdentitySet()

/**
* Serialize document
Expand All @@ -55,6 +58,7 @@ class Serializer(
emitable.emit(DocumentEndEvent(settings.isExplicitEnd))
serializedNodes.clear()
anchors.clear()
recursive.clear()
}

/** Emit [StreamStartEvent] */
Expand Down Expand Up @@ -110,8 +114,18 @@ class Serializer(
private fun serializeNode(node: Node) {
val realNode = if (node is AnchorNode) node.realNode else node

val tAlias = (anchors[realNode])
if (serializedNodes.contains(realNode)) {

if (isDereferenceAliases && recursive.contains(node)) {
throw YamlEngineException("Cannot dereference aliases for recursive structures.")
}
recursive.add(node)
val tAlias = if (!isDereferenceAliases) {
anchors[realNode]
} else {
null
}

if (!isDereferenceAliases && serializedNodes.contains(realNode)) {
emitable.emit(AliasEvent(tAlias))
} else {
serializedNodes.add(realNode)
Expand Down Expand Up @@ -184,6 +198,7 @@ class Serializer(
NodeType.ANCHOR -> {}
}
}
recursive.remove(node)
}

private fun serializeComments(comments: List<CommentLine>?) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package it.krzeminski.snakeyaml.engine.kmp.serializer

import io.kotest.core.spec.style.FunSpec
import io.kotest.matchers.shouldBe

data class TestClass(val value: String)

class IdentitySetTest : FunSpec({
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests fail for Wasm target with a mysterious

 JsException: Exception was thrown while running JavaScript code
     at kotlin.captureStackTrace(file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:16)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.captureStackTrace__externalAdapter(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.Throwable.<init>(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.Throwable.<init>(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.js.JsException.<init>(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.wasm.internal.throwJsException(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlinx.coroutines.intrinsics.startUndispatchedOrReturnIgnoreTimeout(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlinx.coroutines.setupTimeout(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at 8.doResume(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)
     at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlinx.coroutines.withTimeoutOrNull(wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-0169defe)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for a failing test:

     TypeError: _this.contains is not a function
         at it.krzeminski.snakeyaml.engine.kmp.internal.contains_$external_fun (file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:4069:101)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.internal.identityHashCode (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[16416]:0x192c25)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySet.add (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[16281]:0x1913da)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySetTest$<init>$lambda$slambda$lambda.invoke (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26212]:0x23704c)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySetTest$<init>$lambda$slambda$lambda.invoke (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26213]:0x237077)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.kotlin.js.__callFunction_(()->Unit) (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[10224]:0x1393a7)
         at file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:125:121                                        
         at it.krzeminski.snakeyaml.engine.kmp.serializer.jsCatch (file:///Users/piotr/snakeyaml-engine-kmp/build/js/packages/snakeyaml-engine-kmp-wasm-js-test/kotlin/snakeyaml-engine-kmp-wasm-js-test.uninstantiated.mjs:4109:89)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.jsCatch__externalAdapter (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26210]:0x236fee)
         at <it.krzeminski:snakeyaml-engine-kmp_test>.it.krzeminski.snakeyaml.engine.kmp.serializer.IdentitySetTest$<init>$lambda$slambda.doResume (wasm://wasm/<it.krzeminski:snakeyaml-engine-kmp_test>-016926d2:wasm-function[26217]:0x237199)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turned out that there's a bug here:


WeakMap has a has method instead of contains. But even if I fix it, I'm getting further issues with set.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @aSemy in case you have any useful insight there. I see you contributed this part, and it looks like it's being exercised in the tests only now (which is weird since I thought BaseRepresenter is covered fairly well). Do you remember how you tackled implementing such internals in Wasm? Any good reference impls you know? I see korlibs still doesn't have it implemented: https://github.com/korlibs/korlibs-datastructure/blob/b3f589fe2ae7e90b878fce3f586e1f8caba07baf/korlibs-datastructure/src%40wasm/korlibs/datastructure/internal/InternalJs.kt#L25
Ideally we reproduce any issues related to identityHashCode with a new test on the main branch, and fix it there, then this PR should just work. I'll try to handle it, but I'm wondering of you have some useful thoughts/hints here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this helps with the first problem, but it uncovers another one. I'll describe this next problem better.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracting this problem and fixing it to a separate PR: #273

val objectFoo1 = TestClass(value = "foo")
val objectFoo2 = TestClass(value = "foo")
val objectBar = TestClass(value = "bar")

test("expecting existence of the same object wrt. identity") {
val identitySet = IdentitySet<TestClass>()
identitySet.add(objectFoo1)
identitySet.contains(objectFoo1) shouldBe true
}

test("comparing two objects that are equal according to 'equals'") {
val identitySet = IdentitySet<TestClass>()
identitySet.add(objectFoo1)
identitySet.contains(objectFoo2) shouldBe false
}

test("comparing two objects that are not equal according to 'equals'") {
val identitySet = IdentitySet<TestClass>()
identitySet.add(objectFoo1)
identitySet.contains(objectBar) shouldBe false
}

test("clearing the set") {
val identitySet = IdentitySet<TestClass>()
identitySet.add(objectFoo1)
identitySet.contains(objectFoo1) shouldBe true
identitySet.clear()
identitySet.contains(objectFoo1) shouldBe false
}

test("removing the same object") {
val identitySet = IdentitySet<TestClass>()
identitySet.add(objectFoo1)
identitySet.contains(objectFoo1) shouldBe true
identitySet.remove(objectFoo1)
identitySet.contains(objectFoo1) shouldBe false
}

test("removing object that is equal according to 'equals'") {
val identitySet = IdentitySet<TestClass>()
identitySet.add(objectFoo1)
identitySet.contains(objectFoo1) shouldBe true
identitySet.remove(objectFoo2)
identitySet.contains(objectFoo1) shouldBe true
}
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package it.krzeminski.snakeyaml.engine.kmp.usecases.references

import it.krzeminski.snakeyaml.engine.kmp.api.Dump
import it.krzeminski.snakeyaml.engine.kmp.api.DumpSettings
import it.krzeminski.snakeyaml.engine.kmp.api.Load
import it.krzeminski.snakeyaml.engine.kmp.api.LoadSettings
import it.krzeminski.snakeyaml.engine.kmp.common.FlowStyle
import it.krzeminski.snakeyaml.engine.kmp.exceptions.YamlEngineException
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Assertions.fail
import org.junit.jupiter.api.Tag
import org.junit.jupiter.api.Test
import org.snakeyaml.engine.v2.utils.TestUtils
import java.io.StringWriter

@Tag("fast")
class DereferenceAliasesTest {
@Test
fun testNoAliases() {
val settings = LoadSettings.builder().build()
val load = Load(settings)
val map = load.loadFromString(TestUtils.getResource("/issues/issue1086-1-input.yaml")) as Map<*, *>?
val setting = DumpSettings.builder().setDefaultFlowStyle(FlowStyle.BLOCK)
.setDereferenceAliases(true).build()
val dump = Dump(setting)
val node = dump.dumpToString(map)
val out = StringWriter()
val expected = TestUtils.getResource("/issues/issue1086-1-expected.yaml")
assertEquals(expected, node)
}

@Test
fun testNoAliasesRecursive() {
val settings = LoadSettings.builder().build()
val load = Load(settings)
val map = load.loadFromString(TestUtils.getResource("/issues/issue1086-2-input.yaml")) as Map<*, *>?
val setting = DumpSettings.builder().setDefaultFlowStyle(FlowStyle.BLOCK)
.setDereferenceAliases(true).build()
val dump = Dump(setting)
try {
dump.dumpToString(map)
fail()
} catch (e: YamlEngineException) {
assertEquals("Cannot dereference aliases for recursive structures.", e.message)
}
}
}
21 changes: 21 additions & 0 deletions src/jvmTest/resources/issues/issue1086-1-expected.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
defines:
serverPattern1:
type: t3
strage: 500
serverPattern2:
type: t2
strage: 250
lbPattern1:
name: lbForPublic
vpc: vpc1
lbPattern2:
name: lbForInternal
vpc: vpc1
current:
assenbled1:
server:
type: t3
strage: 500
lb:
name: lbForPublic
vpc: vpc1
17 changes: 17 additions & 0 deletions src/jvmTest/resources/issues/issue1086-1-input.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
defines:
serverPattern1: &server_HighPerformance
type: t3
strage: 500
serverPattern2: &server_LowPerformance
type: t2
strage: 250
lbPattern1: &lb_Public
name: lbForPublic
vpc: vpc1
lbPattern2: &lb_Internal
name: lbForInternal
vpc: vpc1
current:
assenbled1:
server: *server_HighPerformance
lb: *lb_Public
34 changes: 34 additions & 0 deletions src/jvmTest/resources/issues/issue1086-2-input.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
&id002
bankAccountOwner: &id001
bankAccountOwner: *id001
birthPlace: Leningrad
birthday: 1970-01-12T13:46:40Z
children: &id003
- *id002
- bankAccountOwner: *id001
birthPlace: New York
birthday: 1983-04-24T02:40:00Z
children: []
father: *id001
mother: &id004
bankAccountOwner: *id001
birthPlace: Saint-Petersburg
birthday: 1973-03-03T09:46:40Z
children: *id003
father: null
mother: null
name: Mother
partner: *id001
name: Daughter
partner: null
father: null
mother: null
name: Father
partner: *id004
birthPlace: Munich
birthday: 1979-10-28T23:06:40Z
children: []
father: *id001
mother: *id004
name: Son
partner: null
2 changes: 1 addition & 1 deletion upstream-commit.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3d97ee77ff70ab1093a63e4d3dadda9e885fe857
1af7dcd2c9c64a9d7cd9de3cd5f64bb2fc2aab6a
Loading