fix(analyzer): Support uppercase-letters in Go module version #7888

wkl3nk · 2023-11-17T10:08:36Z

Uppercase-letters in version strings of dependent Go modules caused the analyzer to crash.
With this fix, uppercase-letters are now properly escaped as any other paths of the Go modules in the file system.

sschuberth · 2023-11-17T10:32:28Z

Thanks for the contribution! Please add Fixes #7880. as a trailing paragraph to the commit message body in order to auto-close the issue upon merge.

plugins/package-managers/go/src/main/kotlin/utils/GoSupport.kt

plugins/package-managers/go/src/main/kotlin/GoMod.kt

plugins/package-managers/go/src/funTest/kotlin/utils/GoSupportFunTest.kt

fviernau · 2023-11-17T10:33:24Z

Nice finding, thank @wkl3nk !

plugins/package-managers/go/src/funTest/kotlin/utils/GoSupportFunTest.kt

plugins/package-managers/go/src/main/kotlin/utils/GoSupport.kt

plugins/package-managers/go/src/funTest/kotlin/utils/GoSupportFunTest.kt

sschuberth · 2023-11-17T10:44:14Z

plugins/package-managers/go/src/main/kotlin/utils/GoSupport.kt

+    val escaped = StringBuilder()
+    for (char in version) {
+        if (char.isUpperCase()) {
+            escaped.append("!${char.lowercaseChar()}")
+        } else {
+            escaped.append(char)
+        }
+    }
+
+    return escaped.toString()


Alternative implementation:

return version.replace(Regex("[A-Z]")) { "!${it.value.lowercase()}" }

This could also extract Regex("[A-Z]") to a private property to the Regex is not recreated on each function call.

Come on. Don't need a Regex for such an easy thing.

Sure you don't. But I'd bet it's even faster (if the Regex is precompiled to a constant) and it's more readable (or at least shorter, if you're not used to Regexes) than your solution that manually iterates over each character in the string. But I'm also fine with keeping your approach.

Okay, using a Regex now, extended the test case to make sure all patterns matching are replaced.

codecov · 2023-11-17T10:56:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (67194c5) 66.96% compared to head (c5342de) 66.90%.

❗ Current head c5342de differs from pull request most recent head 655eada. Consider uploading reports for the commit 655eada to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7888      +/-   ##
============================================
- Coverage     66.96%   66.90%   -0.07%     
  Complexity     2041     2041              
============================================
  Files           356      356              
  Lines         17084    17100      +16     
  Branches       2443     2443              
============================================
  Hits          11440    11440              
- Misses         4623     4639      +16     
  Partials       1021     1021

Flag	Coverage Δ
test	`36.04% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

plugins/package-managers/go/src/main/kotlin/GoMod.kt

wkl3nk · 2023-11-17T14:47:23Z

@sschuberth @fviernau Thanks for your comments, would you have a look at the PR again, please? And don't be too hard, I did not code Kotlin before ...

plugins/package-managers/go/src/main/kotlin/GoMod.kt

plugins/package-managers/go/src/test/kotlin/GoModTest.kt

fviernau · 2023-11-17T15:33:48Z

plugins/package-managers/go/src/main/kotlin/utils/GoSupport.kt

@@ -24,4 +24,4 @@ package org.ossreviewtoolkit.plugins.packagemanagers.go.utils
 * also any suffix starting with '+', because build metadata is not involved in version comparison according to
 * https://go.dev/ref/mod#incompatible-versions.
 */
-fun normalizeModuleVersion(moduleVersion: String): String = moduleVersion.removePrefix("v").substringBefore("+")
+internal fun normalizeModuleVersion(moduleVersion: String): String = moduleVersion.removePrefix("v").substringBefore("+")


IMO the consistency argument (as written) has vanished a bit, since escapeVersion() no more is in this file.
In my view, reducing visibility does not necessarily be done in this PR. But I'm ok to keep it.

However, the commit message should adhere to conventional commits.

I propose to just say

refactor(go)!: Reduce the visibility of `normalizeModuleVersion()` There is no need to expose this function.

Yes. Removed the change, so this way sticking to just one commit in the PR.

sschuberth · 2023-11-17T16:07:27Z

plugins/package-managers/go/src/main/kotlin/GoMod.kt

+internal fun escapeVersion(version: String): String {
+    require("!" !in version) { "Module versions must not contain exclamation marks: $version"}
+
+    val escaped = buildString {


Nit: You can omit the escaped variable and directly return from here: return buildString {.

sschuberth · 2023-11-17T16:08:56Z

plugins/package-managers/go/src/test/kotlin/GoModTest.kt

+    "escapeVersion" should {
+        "escape uppercase letters" {
+            val version = "v0.1.0-M4.0.20231102094829-08e0c3cd016c"
+            val escapedVersion = escapeVersion(version)


Nit: The empty line before this one is still missing (the code that performs the action to test should stand by its own).

sschuberth

LGTM, I'll fixup some nits as a follow-up.

sschuberth · 2023-11-20T10:32:47Z

LGTM, I'll fixup some nits as a follow-up.

I'll make my nit-fixes in-place in this PR as detekt fails.

Uppercase-letters in version strings of dependent Go modules caused the analyzer to crash. With this fix, uppercase-letters are now properly escaped as any other paths of the Go modules in the file system. Fixes #7880. Signed-off-by: Wolfgang Klenk <wolfgang.klenk2@bosch.com>

wkl3nk requested a review from a team as a code owner November 17, 2023 10:08

fviernau requested changes Nov 17, 2023

View reviewed changes

sschuberth requested changes Nov 17, 2023

View reviewed changes

fviernau reviewed Nov 17, 2023

View reviewed changes

plugins/package-managers/go/src/main/kotlin/GoMod.kt Outdated Show resolved Hide resolved

fviernau requested changes Nov 17, 2023

View reviewed changes

plugins/package-managers/go/src/main/kotlin/GoMod.kt Show resolved Hide resolved

plugins/package-managers/go/src/test/kotlin/GoModTest.kt Show resolved Hide resolved

plugins/package-managers/go/src/test/kotlin/GoModTest.kt Outdated Show resolved Hide resolved

fviernau previously approved these changes Nov 17, 2023

View reviewed changes

fviernau enabled auto-merge (rebase) November 17, 2023 15:16

auto-merge was automatically disabled November 17, 2023 15:20
Head branch was pushed to by a user without write access

wkl3nk dismissed fviernau’s stale review via b1f6110 November 17, 2023 15:20

fviernau reviewed Nov 17, 2023

View reviewed changes

sschuberth reviewed Nov 17, 2023

View reviewed changes

wkl3nk requested a review from sschuberth November 20, 2023 09:50

sschuberth previously approved these changes Nov 20, 2023

View reviewed changes

fviernau previously approved these changes Nov 20, 2023

View reviewed changes

fviernau enabled auto-merge (rebase) November 20, 2023 10:23

sschuberth dismissed stale reviews from fviernau and themself via 655eada November 20, 2023 10:38

sschuberth approved these changes Nov 20, 2023

View reviewed changes

fviernau approved these changes Nov 20, 2023

View reviewed changes

fviernau merged commit a9bd271 into oss-review-toolkit:main Nov 20, 2023
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(analyzer): Support uppercase-letters in Go module version #7888

fix(analyzer): Support uppercase-letters in Go module version #7888

wkl3nk commented Nov 17, 2023

sschuberth commented Nov 17, 2023

fviernau commented Nov 17, 2023

sschuberth Nov 17, 2023

wkl3nk Nov 17, 2023

sschuberth Nov 17, 2023

wkl3nk Nov 20, 2023

codecov bot commented Nov 17, 2023 •

edited

Loading

wkl3nk commented Nov 17, 2023

fviernau Nov 17, 2023 •

edited

Loading

wkl3nk Nov 20, 2023

sschuberth Nov 17, 2023

wkl3nk Nov 20, 2023

sschuberth Nov 17, 2023

wkl3nk Nov 20, 2023

sschuberth left a comment

sschuberth commented Nov 20, 2023

fix(analyzer): Support uppercase-letters in Go module version #7888

fix(analyzer): Support uppercase-letters in Go module version #7888

Conversation

wkl3nk commented Nov 17, 2023

sschuberth commented Nov 17, 2023

fviernau commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2023 • edited Loading

Codecov Report

wkl3nk commented Nov 17, 2023

fviernau Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth left a comment

Choose a reason for hiding this comment

sschuberth commented Nov 20, 2023

codecov bot commented Nov 17, 2023 •

edited

Loading

fviernau Nov 17, 2023 •

edited

Loading