From bbe566eb97156b923df8d25c0e69ba469324b793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Wawrzyk?= Date: Thu, 25 Apr 2024 11:57:36 +0200 Subject: [PATCH] Reduce false positives when updating versions (#373) This PR improves the heuristics to replace versions in bazel files, mainly focusing on maven/jvm. * The VersionOnlyHeuristic now doesn't blindly look for the version anywhere. It only looks for version quoted as a string. This is supposed to find the version if it is extracted to a constant. Heuristic only matches if it is the only occurrence. If there are more, we'd need more advanced parsing to figure out which library is it for. This fixes the case when library that we try to update is not defined anywhere in code (for example it comes from another workspace as a constant), but there is another library that has the same version. Before this change, a completely unrelated library could be bumped. * Comments (strings after `#`) are ignored, and bazel-steward will not attempt to update a version in a commented code. * Changed ordering of heuristics, so that version only is treated as last resort. * Refactoring of tests --- .../app/VersionReplacementHeuristicTest.kt | 154 +++++------------- app/src/test/resources/WORKSPACE.bzlignore | 5 + .../bazelsteward/core/common/TextFile.kt | 21 ++- .../core/replacement/LibraryUpdateResolver.kt | 12 +- .../core/replacement/VersionOnlyHeuristic.kt | 18 +- .../bazelsteward/maven/MavenDependencyKind.kt | 4 +- projectview.bazelproject | 6 + 7 files changed, 100 insertions(+), 120 deletions(-) create mode 100644 projectview.bazelproject diff --git a/app/src/test/kotlin/org/virtuslab/bazelsteward/app/VersionReplacementHeuristicTest.kt b/app/src/test/kotlin/org/virtuslab/bazelsteward/app/VersionReplacementHeuristicTest.kt index 491dab15..96fde500 100644 --- a/app/src/test/kotlin/org/virtuslab/bazelsteward/app/VersionReplacementHeuristicTest.kt +++ b/app/src/test/kotlin/org/virtuslab/bazelsteward/app/VersionReplacementHeuristicTest.kt @@ -9,7 +9,7 @@ import org.virtuslab.bazelsteward.core.common.UpdateSuggestion import org.virtuslab.bazelsteward.core.library.SemanticVersion import org.virtuslab.bazelsteward.core.replacement.LibraryUpdateResolver import org.virtuslab.bazelsteward.core.replacement.PythonFunctionCallHeuristic -import org.virtuslab.bazelsteward.core.replacement.VersionOnlyHeuristic +import org.virtuslab.bazelsteward.core.replacement.VersionOnlyInStringLiteralHeuristic import org.virtuslab.bazelsteward.core.replacement.VersionReplacementHeuristic import org.virtuslab.bazelsteward.core.replacement.WholeLibraryHeuristic import org.virtuslab.bazelsteward.fixture.loadTextFileFromResources @@ -27,6 +27,7 @@ class VersionReplacementHeuristicTest { val positionOf4132: Int = 3058 val positionOf852: Int = 3326 val positionOfJunitJupiter581: Int = 3432 + val positionOf1_99_99: Int = 3526 @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) @@ -35,39 +36,26 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset`() { val library = library("com.7theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion) + val result = resolveUpdates(library) result?.offset shouldBe positionOf235 } @Test - fun `should return correct position offset with wrong artifact`() { + fun `should not return correct position offset with wrong artifact`() { val library = library("com.10theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion) + val result = resolveUpdates(library) - result?.offset shouldBe positionOf235 - } - - @Test - fun `should return correct position offset without artifact`() { - val library = library("", "", "2.3.5") - val suggestedVersion = version("2.4.0") - - val result = resolveUpdates(library, suggestedVersion) - - result?.offset shouldBe positionOf235 + result?.offset shouldBe null } @Test fun `should return correct position offset for artifact with version in variable`() { val library = library("io.grpc", "grpc-core", "1.2.0") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion) + val result = resolveUpdates(library) result?.offset shouldBe positionOf120 } @@ -75,12 +63,29 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset when two libraries have same version`() { val library = library("org.jetbrains.kotlinx", "kotlinx-coroutines-core", "1.6.0") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion) + val result = resolveUpdates(library) result?.offset shouldBe positionOf160 } + + @Test + fun `should not find position of library that is not defined in available project sources despite existing version string`() { + val library = library("com.example", "not-existent", "1.99.99") + + val result = resolveUpdates(library) + + result?.offset shouldBe null + } + + @Test + fun `should ignore commented code`() { + val library = library("org.virtuslab", "bazel-steward", "1.99.99") + + val result = resolveUpdates(library) + + result?.offset shouldBe positionOf1_99_99 + } } @Nested @@ -90,9 +95,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position`() { val library = library("com.7theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) + val result = resolveUpdates(library, WholeLibraryHeuristic) result?.offset shouldBe positionOf235 } @@ -100,9 +104,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return null with wrong artifact`() { val library = library("com.10theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) + val result = resolveUpdates(library, WholeLibraryHeuristic) result shouldBe null } @@ -110,9 +113,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position without artifact`() { val library = library("", "", "2.3.5") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) + val result = resolveUpdates(library, WholeLibraryHeuristic) result?.offset shouldBe positionOf235 } @@ -120,9 +122,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return null for artifact with version in variable`() { val library = library("io.grpc", "grpc-core", "1.2.0") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) + val result = resolveUpdates(library, WholeLibraryHeuristic) result shouldBe null } @@ -130,9 +131,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset when two libraries have same version`() { val library = library("org.jetbrains.kotlinx", "kotlinx-coroutines-core", "1.6.0") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) + val result = resolveUpdates(library, WholeLibraryHeuristic) result?.offset shouldBe positionOf160 } @@ -140,9 +140,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset when two libraries have same version and one is prefix of another`() { val library = library("org.junit.jupiter", "junit-jupiter", "5.8.1") - val suggestedVersion = version("5.9.2") - val result = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) + val result = resolveUpdates(library, WholeLibraryHeuristic) result?.offset shouldBe positionOfJunitJupiter581 } @@ -152,52 +151,20 @@ class VersionReplacementHeuristicTest { @TestInstance(TestInstance.Lifecycle.PER_CLASS) inner class VersionOnlyHeuristicTest { - @Test - fun `should return correct position offset `() { - val library = library("com.7theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - - val result = resolveUpdates(library, suggestedVersion, VersionOnlyHeuristic) - - result?.offset shouldBe positionOf235 - } - - @Test - fun `should return correct position offset with wrong artifact`() { - val library = library("com.10theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - - val result = resolveUpdates(library, suggestedVersion, VersionOnlyHeuristic) - - result?.offset shouldBe positionOf235 - } - - @Test - fun `should return correct position offset without artifact`() { - val library = library("", "", "2.3.5") - val suggestedVersion = version("2.4.0") - - val result = resolveUpdates(library, suggestedVersion, VersionOnlyHeuristic) - - result?.offset shouldBe positionOf235 - } - @Test fun `should return correct position for artifact with version in variable`() { val library = library("io.grpc", "grpc-core", "1.2.0") - val suggestedVersion = version("2.4.0") - val result = resolveUpdates(library, suggestedVersion, VersionOnlyHeuristic) + val result = resolveUpdates(library, VersionOnlyInStringLiteralHeuristic) result?.offset shouldBe positionOf120 } @Test fun `should return null when two libraries have same version`() { - val library = library("org.jetbrains.kotlinx", "kotlinx-coroutines-core", "1.6.0") - val suggestedVersion = version("2.4.0") + val library = library("com.example", "test", "9.9.9") - val result = resolveUpdates(library, suggestedVersion, VersionOnlyHeuristic) + val result = resolveUpdates(library, VersionOnlyInStringLiteralHeuristic) result shouldBe null } @@ -210,9 +177,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset maven artifact`() { val library = library("com.google.guava", "guava-testlib", "31.1.0-jre") - val suggestedVersion = version("32.0.0-jre") - val result = resolveUpdates(library, suggestedVersion, PythonFunctionCallHeuristic) + val result = resolveUpdates(library, PythonFunctionCallHeuristic) result?.offset shouldBe positionOf3200jre } @@ -220,9 +186,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset maven artifact named parameters`() { val library = library("com.google.truth", "truth", "1.1.3") - val suggestedVersion = version("1.2.0") - val result = resolveUpdates(library, suggestedVersion, PythonFunctionCallHeuristic) + val result = resolveUpdates(library, PythonFunctionCallHeuristic) result?.offset shouldBe positionOf113 } @@ -230,9 +195,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset scala dep`() { val library = library("org.scalactic", "scalactic", "3.2.12") - val suggestedVersion = version("4.0.0") - val result = resolveUpdates(library, suggestedVersion, PythonFunctionCallHeuristic) + val result = resolveUpdates(library, PythonFunctionCallHeuristic) result?.offset shouldBe positionOf3212 } @@ -240,9 +204,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return null for wrong scala dep`() { val library = library("org.scalactic", "scalactic", "3.2.90") - val suggestedVersion = version("4.0.0") - val result = resolveUpdates(library, suggestedVersion, PythonFunctionCallHeuristic) + val result = resolveUpdates(library, PythonFunctionCallHeuristic) result shouldBe null } @@ -250,9 +213,8 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset scala dep named parameters`() { val library = library("junit", "junit", "4.13.2") - val suggestedVersion = version("4.14.0") - val result = resolveUpdates(library, suggestedVersion, PythonFunctionCallHeuristic) + val result = resolveUpdates(library, PythonFunctionCallHeuristic) result?.offset shouldBe positionOf4132 } @@ -260,42 +222,13 @@ class VersionReplacementHeuristicTest { @Test fun `should return correct position offset scala dep with scala version`() { val library = library("com.sksamuel.elastic4s", "elastic4s-client-akka_2.12", "8.5.2") - val suggestedVersion = version("8.6.0") - val result = resolveUpdates(library, suggestedVersion, PythonFunctionCallHeuristic) + val result = resolveUpdates(library, PythonFunctionCallHeuristic) result?.offset shouldBe positionOf852 } } - @Nested - @TestInstance(TestInstance.Lifecycle.PER_CLASS) - inner class CompareHeuristicTest { - - @Test - fun `should return same position offset for WholeVersionHeuristic and VersionHeuristic`() { - val library = library("com.7theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - - val result1 = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) - val result2 = resolveUpdates(library, suggestedVersion, VersionOnlyHeuristic) - - result1?.offset shouldBe result2?.offset - } - - @Test - fun `should return different position offset for WholeVersionHeuristic and VersionHeuristic`() { - val library = library("com.10theta", "utilis", "2.3.5") - val suggestedVersion = version("2.4.0") - - val result1 = resolveUpdates(library, suggestedVersion, WholeLibraryHeuristic) - val result2 = resolveUpdates(library, suggestedVersion, VersionOnlyHeuristic) - - result1 shouldBe null - result2?.offset shouldBe positionOf235 - } - } - private fun library(group: String, artifact: String, version: String) = MavenCoordinates( MavenLibraryId(group, artifact), @@ -308,13 +241,12 @@ class VersionReplacementHeuristicTest { private val resolver = LibraryUpdateResolver() - private val allHeuristics = listOf(WholeLibraryHeuristic, VersionOnlyHeuristic, PythonFunctionCallHeuristic).toTypedArray() + private val allHeuristics = listOf(WholeLibraryHeuristic, PythonFunctionCallHeuristic, VersionOnlyInStringLiteralHeuristic).toTypedArray() private fun resolveUpdates( library: MavenCoordinates, - version: SemanticVersion, vararg heuristics: VersionReplacementHeuristic = allHeuristics, ): FileChange? { - return resolver.resolve(files, UpdateSuggestion(library, version), heuristics.toList())?.fileChanges?.firstOrNull() + return resolver.resolve(files, UpdateSuggestion(library, version("999.999.999")), heuristics.toList())?.fileChanges?.firstOrNull() } } diff --git a/app/src/test/resources/WORKSPACE.bzlignore b/app/src/test/resources/WORKSPACE.bzlignore index 7f02d724..38b134ae 100644 --- a/app/src/test/resources/WORKSPACE.bzlignore +++ b/app/src/test/resources/WORKSPACE.bzlignore @@ -100,9 +100,14 @@ maven_install( _scala_dep("com.sksamuel.elastic4s", "elastic4s-client-akka", "8.5.2"), "org.junit.jupiter:junit-jupiter-engine:5.8.1", "org.junit.jupiter:junit-jupiter:5.8.1", + # "org.virtuslab:bazel-steward:1.99.99", + "org.virtuslab:bazel-steward:1.99.99", ], fetch_sources = False, repositories = [ "https://clojars.org/repo", ], ) + +TEST_VERSION = "9.9.9" +TEST_VERSION_2 = "9.9.9" diff --git a/core/src/main/kotlin/org/virtuslab/bazelsteward/core/common/TextFile.kt b/core/src/main/kotlin/org/virtuslab/bazelsteward/core/common/TextFile.kt index 72b68cc6..43ac8c79 100644 --- a/core/src/main/kotlin/org/virtuslab/bazelsteward/core/common/TextFile.kt +++ b/core/src/main/kotlin/org/virtuslab/bazelsteward/core/common/TextFile.kt @@ -7,13 +7,28 @@ interface TextFile { val path: Path val content: String + fun map(f: (String) -> String): TextFile = MappedTextFile(this, f) + + private class MappedTextFile( + private val file: TextFile, + private val transform: (String) -> String, + ) : TextFile { + + override val path: Path + get() = file.path + + override val content: String by lazy { + transform(file.content) + } + + override fun toString(): String = file.toString() + } + private class LazyTextFile(override val path: Path) : TextFile { override val content: String get() = path.readText() - override fun toString(): String { - return path.toString() - } + override fun toString(): String = path.toString() } companion object { diff --git a/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/LibraryUpdateResolver.kt b/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/LibraryUpdateResolver.kt index 275f7154..9fa4428d 100644 --- a/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/LibraryUpdateResolver.kt +++ b/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/LibraryUpdateResolver.kt @@ -9,8 +9,18 @@ class LibraryUpdateResolver { updateSuggestion: UpdateSuggestion, heuristics: List, ): LibraryUpdate? { + val preProcessedFiles = files.map { file -> file.map { stripComments(it) } } return heuristics.firstNotNullOfOrNull { heuristic -> - heuristic.apply(files, updateSuggestion) + heuristic.apply(preProcessedFiles, updateSuggestion) + } + } + + private fun stripComments(s: String): String { + val regex = "#.*".toRegex() + return s.lines().joinToString("\n") { line -> + regex.replace(line) { match -> + " ".repeat(match.value.length) + } } } } diff --git a/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/VersionOnlyHeuristic.kt b/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/VersionOnlyHeuristic.kt index 968714e3..27394968 100644 --- a/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/VersionOnlyHeuristic.kt +++ b/core/src/main/kotlin/org/virtuslab/bazelsteward/core/replacement/VersionOnlyHeuristic.kt @@ -4,17 +4,27 @@ import org.virtuslab.bazelsteward.core.common.FileChange import org.virtuslab.bazelsteward.core.common.TextFile import org.virtuslab.bazelsteward.core.common.UpdateSuggestion -object VersionOnlyHeuristic : VersionReplacementHeuristic { - override val name: String = "version-only" +object VersionOnlyHeuristic : BaseVersionOnlyHeuristic("version-only") { + override fun versionToRegex(currentVersion: String): Regex = + Regex.escape(currentVersion).toRegex() +} + +object VersionOnlyInStringLiteralHeuristic : BaseVersionOnlyHeuristic("version-only-in-string-literal") { + override fun versionToRegex(currentVersion: String): Regex = + """(?<=["'])(${Regex.escape(currentVersion)})(?=["'])""".toRegex() +} + +abstract class BaseVersionOnlyHeuristic(override val name: String) : VersionReplacementHeuristic { override fun apply(files: List, updateSuggestion: UpdateSuggestion): LibraryUpdate? { val currentVersion = updateSuggestion.currentLibrary.version.value - val regex = Regex.escape(currentVersion).toRegex() + val regex = versionToRegex(currentVersion) val matchResult = files.firstNotNullOfOrNull { f -> regex.find(f.content)?.let { MatchedText(it, f.path) } } ?: return null + matchResult.match.next()?.let { return null } val versionOffset = matchResult.offsetLastMatchGroup ?: return null @@ -30,4 +40,6 @@ object VersionOnlyHeuristic : VersionReplacementHeuristic { ), ) } + + protected abstract fun versionToRegex(currentVersion: String): Regex } diff --git a/kinds/maven/src/main/kotlin/org/virtuslab/bazelsteward/maven/MavenDependencyKind.kt b/kinds/maven/src/main/kotlin/org/virtuslab/bazelsteward/maven/MavenDependencyKind.kt index 4a88ad8f..90e19ccf 100644 --- a/kinds/maven/src/main/kotlin/org/virtuslab/bazelsteward/maven/MavenDependencyKind.kt +++ b/kinds/maven/src/main/kotlin/org/virtuslab/bazelsteward/maven/MavenDependencyKind.kt @@ -6,7 +6,7 @@ import org.virtuslab.bazelsteward.core.PathPattern import org.virtuslab.bazelsteward.core.library.Library import org.virtuslab.bazelsteward.core.library.Version import org.virtuslab.bazelsteward.core.replacement.PythonFunctionCallHeuristic -import org.virtuslab.bazelsteward.core.replacement.VersionOnlyHeuristic +import org.virtuslab.bazelsteward.core.replacement.VersionOnlyInStringLiteralHeuristic import org.virtuslab.bazelsteward.core.replacement.VersionReplacementHeuristic import org.virtuslab.bazelsteward.core.replacement.WholeLibraryHeuristic import java.nio.file.Path @@ -43,7 +43,7 @@ class MavenDependencyKind( override val defaultVersionReplacementHeuristics: List = listOf( WholeLibraryHeuristic, - VersionOnlyHeuristic, PythonFunctionCallHeuristic, + VersionOnlyInStringLiteralHeuristic, ) } diff --git a/projectview.bazelproject b/projectview.bazelproject new file mode 100644 index 00000000..a7ea40a7 --- /dev/null +++ b/projectview.bazelproject @@ -0,0 +1,6 @@ +targets: + //... + +derive_targets_from_directories: false +produce_trace_log: false +build_manual_targets: false