From 6e242226ebe5f4c7cf7ef7160532deeb8cbf65be Mon Sep 17 00:00:00 2001 From: ntwigg Date: Fri, 25 Apr 2025 13:27:07 -0700 Subject: [PATCH 1/6] Create a reproducer test case for #529 --- .../diffplug/selfie/SelfieImplementations.kt | 4 +- .../diffplug/selfie/SnapshotFacetBugTest.kt | 103 ++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt diff --git a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt b/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt index 0b641dfd..c6ce1d7f 100644 --- a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt +++ b/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt @@ -1,5 +1,5 @@ /* - * Copyright (C) 2024 DiffPlug + * Copyright (C) 2024-2025 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -249,7 +249,7 @@ private fun toBeDidntMatch(expected: T?, actual: T, format: LiteralFor } } } -private fun assertEqual(expected: Snapshot?, actual: Snapshot, storage: SnapshotSystem) { +internal fun assertEqual(expected: Snapshot?, actual: Snapshot, storage: SnapshotSystem) { when (expected) { null -> throw storage.fs.assertFailed(storage.mode.msgSnapshotNotFound()) actual -> return diff --git a/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt b/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt new file mode 100644 index 00000000..f7c36940 --- /dev/null +++ b/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt @@ -0,0 +1,103 @@ +/* + * Copyright (C) 2025 DiffPlug + * + * 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 + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * 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 com.diffplug.selfie + +import com.diffplug.selfie.guts.FS +import com.diffplug.selfie.guts.SnapshotSystem +import com.diffplug.selfie.guts.TypedPath +import io.kotest.assertions.throwables.shouldThrow +import io.kotest.matchers.shouldBe +import kotlin.test.Test + +class SnapshotFacetBugTest { + /** + * This test reproduces the bug where adding a new facet to a snapshot that was already stored on + * disk causes a StringIndexOutOfBoundsException during comparison. + */ + @Test + fun testAddingNewFacetToStoredSnapshot() { + // Create a simple mock FS + val mockFs = + object : FS { + override fun fileExists(typedPath: TypedPath): Boolean = false + override fun fileWalk(typedPath: TypedPath, walk: (Sequence) -> T): T = + throw UnsupportedOperationException("Not needed for this test") + override fun fileReadBinary(typedPath: TypedPath): ByteArray = + throw UnsupportedOperationException("Not needed for this test") + override fun fileWriteBinary(typedPath: TypedPath, content: ByteArray) { + // Not needed for this test + } + override fun assertFailed(message: String, expected: Any?, actual: Any?): Throwable = + AssertionError(message) + } + + // Create a simple mock SnapshotSystem + val mockSystem = + object : SnapshotSystem { + override val fs = mockFs + override val mode = Mode.readonly + override val layout = + object : com.diffplug.selfie.guts.SnapshotFileLayout { + override val rootFolder = TypedPath.ofFolder("/test/") + override val fs = mockFs + override val allowMultipleEquivalentWritesToOneLocation = false + override val javaDontUseTripleQuoteLiterals = false + override fun sourcePathForCall( + call: com.diffplug.selfie.guts.CallLocation + ): TypedPath = TypedPath.ofFile("/test/Test.kt") + override fun sourcePathForCallMaybe( + call: com.diffplug.selfie.guts.CallLocation + ): TypedPath? = TypedPath.ofFile("/test/Test.kt") + override fun checkForSmuggledError() { + // Not needed for this test + } + } + override fun sourceFileHasWritableComment( + call: com.diffplug.selfie.guts.CallStack + ): Boolean = false + override fun writeInline( + literalValue: com.diffplug.selfie.guts.LiteralValue<*>, + call: com.diffplug.selfie.guts.CallStack + ) { + // Not needed for this test + } + override fun writeToBeFile( + path: TypedPath, + data: ByteArray, + call: com.diffplug.selfie.guts.CallStack + ) { + // Not needed for this test + } + override fun diskThreadLocal() = + throw UnsupportedOperationException("Not needed for this test") + } + + // Step 1: Create a simple snapshot (this would be the one stored on disk) + val storedSnapshot = Snapshot.of("") + + // Step 2: Create a new snapshot with an added facet + val updatedSnapshot = Snapshot.of("").plusFacet("new-facet", "new-facet-value") + + // Step 3: This should throw a StringIndexOutOfBoundsException due to the bug + val exception = + shouldThrow { + assertEqual(storedSnapshot, updatedSnapshot, mockSystem) + } + + // Verify the exception message matches the expected error + exception.message shouldBe "String index out of range: -1" + } +} From f3efe59fd84ccf97dbb939340166f7b41d5bb45a Mon Sep 17 00:00:00 2001 From: ntwigg Date: Fri, 25 Apr 2025 13:32:01 -0700 Subject: [PATCH 2/6] Add a smaller test for maintaining this in the long-term. --- .../diffplug/selfie/SelfieImplementations.kt | 2 +- .../selfie/SelfieImplementationsTest.kt | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt diff --git a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt b/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt index c6ce1d7f..89d07623 100644 --- a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt +++ b/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt @@ -212,7 +212,7 @@ class StringSelfie( * Returns a serialized form of only the given facets if they are available, silently omits missing * facets. */ -private fun serializeOnlyFacets(snapshot: Snapshot, keys: Collection): String { +internal fun serializeOnlyFacets(snapshot: Snapshot, keys: Collection): String { val writer = StringBuilder() for (key in keys) { if (key.isEmpty()) { diff --git a/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt b/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt new file mode 100644 index 00000000..7b0654cd --- /dev/null +++ b/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2025 DiffPlug + * + * 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 + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * 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 com.diffplug.selfie + +import io.kotest.assertions.throwables.shouldThrow +import io.kotest.matchers.shouldBe +import kotlin.test.Test + +class SelfieImplementationsTest { + @Test + fun issue_529() { + val empty = Snapshot.of("") + val emptyPlusFacet = Snapshot.of("").plusFacet("new-facet", "new-facet-value") + val exception = + shouldThrow { + serializeOnlyFacets(empty, listOf("new-facet")) + } + exception.message shouldBe "String index out of range: -1" + } +} From 46a387a4f80291d91e68592858dfd8c5fe03fe2f Mon Sep 17 00:00:00 2001 From: ntwigg Date: Fri, 25 Apr 2025 13:35:44 -0700 Subject: [PATCH 3/6] Update changelog. --- jvm/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/jvm/CHANGELOG.md b/jvm/CHANGELOG.md index 3fe27bdd..8a102a7e 100644 --- a/jvm/CHANGELOG.md +++ b/jvm/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed - Off-by-one in the error message for a VCR key mismatch. ([#526](https://github.com/diffplug/selfie/pull/526)) +- Fix `StringIndexOutOfBoundsException` when an empty snapshot had a facet added. (fixes [#529](https://github.com/diffplug/selfie/issues/529)) ## [2.5.1] - 2025-03-04 ### Fixed From 9351c9e93549d273a7056fb3a6eaf563dc14aef7 Mon Sep 17 00:00:00 2001 From: ntwigg Date: Fri, 25 Apr 2025 13:41:16 -0700 Subject: [PATCH 4/6] Remove the giant test that Cline made. --- .../diffplug/selfie/SnapshotFacetBugTest.kt | 103 ------------------ 1 file changed, 103 deletions(-) delete mode 100644 jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt diff --git a/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt b/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt deleted file mode 100644 index f7c36940..00000000 --- a/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SnapshotFacetBugTest.kt +++ /dev/null @@ -1,103 +0,0 @@ -/* - * Copyright (C) 2025 DiffPlug - * - * 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 - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * 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 com.diffplug.selfie - -import com.diffplug.selfie.guts.FS -import com.diffplug.selfie.guts.SnapshotSystem -import com.diffplug.selfie.guts.TypedPath -import io.kotest.assertions.throwables.shouldThrow -import io.kotest.matchers.shouldBe -import kotlin.test.Test - -class SnapshotFacetBugTest { - /** - * This test reproduces the bug where adding a new facet to a snapshot that was already stored on - * disk causes a StringIndexOutOfBoundsException during comparison. - */ - @Test - fun testAddingNewFacetToStoredSnapshot() { - // Create a simple mock FS - val mockFs = - object : FS { - override fun fileExists(typedPath: TypedPath): Boolean = false - override fun fileWalk(typedPath: TypedPath, walk: (Sequence) -> T): T = - throw UnsupportedOperationException("Not needed for this test") - override fun fileReadBinary(typedPath: TypedPath): ByteArray = - throw UnsupportedOperationException("Not needed for this test") - override fun fileWriteBinary(typedPath: TypedPath, content: ByteArray) { - // Not needed for this test - } - override fun assertFailed(message: String, expected: Any?, actual: Any?): Throwable = - AssertionError(message) - } - - // Create a simple mock SnapshotSystem - val mockSystem = - object : SnapshotSystem { - override val fs = mockFs - override val mode = Mode.readonly - override val layout = - object : com.diffplug.selfie.guts.SnapshotFileLayout { - override val rootFolder = TypedPath.ofFolder("/test/") - override val fs = mockFs - override val allowMultipleEquivalentWritesToOneLocation = false - override val javaDontUseTripleQuoteLiterals = false - override fun sourcePathForCall( - call: com.diffplug.selfie.guts.CallLocation - ): TypedPath = TypedPath.ofFile("/test/Test.kt") - override fun sourcePathForCallMaybe( - call: com.diffplug.selfie.guts.CallLocation - ): TypedPath? = TypedPath.ofFile("/test/Test.kt") - override fun checkForSmuggledError() { - // Not needed for this test - } - } - override fun sourceFileHasWritableComment( - call: com.diffplug.selfie.guts.CallStack - ): Boolean = false - override fun writeInline( - literalValue: com.diffplug.selfie.guts.LiteralValue<*>, - call: com.diffplug.selfie.guts.CallStack - ) { - // Not needed for this test - } - override fun writeToBeFile( - path: TypedPath, - data: ByteArray, - call: com.diffplug.selfie.guts.CallStack - ) { - // Not needed for this test - } - override fun diskThreadLocal() = - throw UnsupportedOperationException("Not needed for this test") - } - - // Step 1: Create a simple snapshot (this would be the one stored on disk) - val storedSnapshot = Snapshot.of("") - - // Step 2: Create a new snapshot with an added facet - val updatedSnapshot = Snapshot.of("").plusFacet("new-facet", "new-facet-value") - - // Step 3: This should throw a StringIndexOutOfBoundsException due to the bug - val exception = - shouldThrow { - assertEqual(storedSnapshot, updatedSnapshot, mockSystem) - } - - // Verify the exception message matches the expected error - exception.message shouldBe "String index out of range: -1" - } -} From 0c45d92b0879402a9a2723c38f99413344916b5e Mon Sep 17 00:00:00 2001 From: ntwigg Date: Fri, 25 Apr 2025 13:41:26 -0700 Subject: [PATCH 5/6] Update our tight test. --- .../com/diffplug/selfie/SelfieImplementationsTest.kt | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt b/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt index 7b0654cd..7e6635e9 100644 --- a/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt +++ b/jvm/selfie-lib/src/commonTest/kotlin/com/diffplug/selfie/SelfieImplementationsTest.kt @@ -15,7 +15,6 @@ */ package com.diffplug.selfie -import io.kotest.assertions.throwables.shouldThrow import io.kotest.matchers.shouldBe import kotlin.test.Test @@ -24,10 +23,8 @@ class SelfieImplementationsTest { fun issue_529() { val empty = Snapshot.of("") val emptyPlusFacet = Snapshot.of("").plusFacet("new-facet", "new-facet-value") - val exception = - shouldThrow { - serializeOnlyFacets(empty, listOf("new-facet")) - } - exception.message shouldBe "String index out of range: -1" + val mismathedKeys = listOf("new-facet") + serializeOnlyFacets(empty, mismathedKeys) shouldBe "" + serializeOnlyFacets(emptyPlusFacet, mismathedKeys) shouldBe "╔═ [new-facet] ═╗\nnew-facet-value" } } From ffc20fdde8329898b68e4f97db6b4b12e7bc59b5 Mon Sep 17 00:00:00 2001 From: ntwigg Date: Fri, 25 Apr 2025 13:41:51 -0700 Subject: [PATCH 6/6] Fix `serializeOnlyFacets` to handle empty, needed for pure additions. --- .../kotlin/com/diffplug/selfie/SelfieImplementations.kt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt b/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt index 89d07623..088b7aa2 100644 --- a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt +++ b/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/SelfieImplementations.kt @@ -226,7 +226,10 @@ internal fun serializeOnlyFacets(snapshot: Snapshot, keys: Collection): // this codepath is triggered by the `key.isEmpty()` line above writer.subSequence(EMPTY_KEY_AND_FACET.length, writer.length - 1).toString() } else { - writer.setLength(writer.length - 1) + // Check if the writer is empty to avoid StringIndexOutOfBoundsException + if (writer.length > 0) { + writer.setLength(writer.length - 1) + } writer.toString() } } @@ -249,7 +252,7 @@ private fun toBeDidntMatch(expected: T?, actual: T, format: LiteralFor } } } -internal fun assertEqual(expected: Snapshot?, actual: Snapshot, storage: SnapshotSystem) { +private fun assertEqual(expected: Snapshot?, actual: Snapshot, storage: SnapshotSystem) { when (expected) { null -> throw storage.fs.assertFailed(storage.mode.msgSnapshotNotFound()) actual -> return