fix: add helper method to check if version is below or equal to a specific version #14812
Conversation
📝 WalkthroughWalkthroughThe pull request removes version-handling constants and a method from the Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/app-preview
Jondyr
added
team/studio-core
kind/bug
✅ Actions performedReview triggered.
|
github-actions
bot
added
area/app-preview
Jondyr
force-pushed
the
fix/applicationmetadata-version-mock-breaks-for-preview-after-v4
branch
from
c5f58d9 to
8bcbb4b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 5
🧹 Nitpick comments (4)
backend/tests/Designer.Tests/Helpers/Preview/NugetVersionHelperTests.cs (2)
10-11: Add summary documentation to test method.The test method name is descriptive, but adding an XML documentation comment would improve clarity about what's being tested and why.
[Fact] + /// <summary> + /// Tests that GetMockedAltinnNugetBuildFromVersion returns an empty string for versions below or equal to 8.0.0-preview.14, + /// and returns "8.0.0.0" for versions above that threshold. + /// </summary> public void GetMockedAltinnNugetBuildFromVersion_ShouldReturnEmptyStringForVersionsBelowBreakpoint()
12-22: Remove duplicated assertion and improve test organization.There's a duplicated assertion on line 18, and the test mixes assertions for different outcomes (empty string vs "8.0.0.0"). Consider restructuring the test for better clarity.
{ - Assert.Equal("", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("1.1.1")); - Assert.Equal("", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("1.1.1-preview.150")); - Assert.Equal("", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("1.1.1-preview.0")); - Assert.Equal("8.0.0.0", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("8.1.1-preview.14")); - Assert.Equal("8.0.0.0", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("8.8.8-preview.0")); - Assert.Equal("8.0.0.0", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("8.18.1238")); - Assert.Equal("8.0.0.0", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("8.18.1238")); - Assert.Equal("8.0.0.0", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("8.8.8-preview.342")); - Assert.Equal("8.0.0.0", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("8.0.0-preview.15")); - Assert.Equal("", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion("8.0.0-preview.14")); + // Versions that should return an empty string + string[] versionsExpectingEmpty = new[] + { + "1.1.1", + "1.1.1-preview.150", + "1.1.1-preview.0", + "8.0.0-preview.14" + }; + + foreach (string version in versionsExpectingEmpty) + { + Assert.Equal("", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion(version), $"Version {version} should return empty string"); + } + + // Versions that should return the minimum nuget version + string[] versionsExpectingMinimumVersion = new[] + { + "8.1.1-preview.14", + "8.8.8-preview.0", + "8.18.1238", + "8.8.8-preview.342", + "8.0.0-preview.15" + }; + + foreach (string version in versionsExpectingMinimumVersion) + { + Assert.Equal("8.0.0.0", NugetVersionHelper.GetMockedAltinnNugetBuildFromVersion(version), $"Version {version} should return minimum nuget version"); + } }backend/src/Designer/Helpers/Preview/NugetVersionHelper.cs (2)
38-41: Improve version validation to handle edge cases.The current validation only checks for minimum length and major version, but doesn't verify if the version parts are valid integers.
private static bool IsValidSemVerVersion(string[] versionParts) { - return versionParts.Length >= 3 && Convert.ToInt32(versionParts[0]) >= 8; + if (versionParts.Length < 3) + { + return false; + } + + // Check if first part (major version) is valid and >= 8 + if (!int.TryParse(versionParts[0], out int majorVersion) || majorVersion < 8) + { + return false; + } + + // Check if other required parts are valid integers + return versionParts.Take(3).All(part => int.TryParse(part.Split('-')[0], out _)); }
43-57: Consider modern validation approach instead of Code Contracts.The
Contract.Requiresstatements are part of Code Contracts which is not being actively developed. Consider using guard clauses instead.// <exception cref="FormatException"></exception> // <exception cref="OverflowException"></exception> private static bool IsVersionOrBelow(int[] versionParts, int[] breakpoint) { - Contract.Requires(versionParts.Length == 3); - Contract.Requires(breakpoint.Length == 3); + if (versionParts.Length != 3 || breakpoint.Length != 3) + { + throw new ArgumentException("Version arrays must have exactly 3 elements"); + } + for (int i = 0; i < 3; i++) { if (versionParts[i] > breakpoint[i]) { return false; } } return true; }
🛑 Comments failed to post (5)
backend/tests/Designer.Tests/Helpers/Preview/NugetVersionHelperTests.cs (1)
1-3:
⚠️ Potential issueFix imports ordering to comply with code style.
The pipeline failure indicates an issue with imports ordering.
-using Xunit; -using Altinn.Studio.Designer.Helpers.Preview; +using Altinn.Studio.Designer.Helpers.Preview; +using Xunit;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.using Altinn.Studio.Designer.Helpers.Preview; using Xunit;🧰 Tools
🪛 GitHub Actions: Dotnet format check
[error] 1-1: IMPORTS: Fix imports ordering.
backend/src/Designer/Helpers/Preview/NugetVersionHelper.cs (4)
64-67:
⚠️ Potential issueAdd error handling to GetPreviewVersion.
The method assumes the preview version number is always in the fourth element and is a valid integer.
private static int GetPreviewVersion(string[] versionParts) { - return Convert.ToInt32(versionParts[3]); + if (versionParts.Length < 4) + { + return 0; // Default to 0 if preview version not specified + } + + try + { + return Convert.ToInt32(versionParts[3]); + } + catch (FormatException) + { + return 0; // Return default if conversion fails + } + catch (OverflowException) + { + return 0; // Return default if number is too large + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private static int GetPreviewVersion(string[] versionParts) { if (versionParts.Length < 4) { return 0; // Default to 0 if preview version not specified } try { return Convert.ToInt32(versionParts[3]); } catch (FormatException) { return 0; // Return default if conversion fails } catch (OverflowException) { return 0; // Return default if number is too large } }
19-26:
⚠️ Potential issueAdd null check for the version parameter.
The method doesn't handle null or empty version strings, which could cause a NullReferenceException.
public static string GetMockedAltinnNugetBuildFromVersion(string version) { + if (string.IsNullOrEmpty(version)) + { + return string.Empty; + } string[] versionParts = version.Split('.'); if (!IsValidSemVerVersion(versionParts)) { return string.Empty; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public static string GetMockedAltinnNugetBuildFromVersion(string version) { if (string.IsNullOrEmpty(version)) { return string.Empty; } string[] versionParts = version.Split('.'); if (!IsValidSemVerVersion(versionParts)) { return string.Empty; }
59-62: 🛠️ Refactor suggestion
Make IsPreviewVersion more robust.
The current implementation assumes specific structure and doesn't handle edge cases.
private static bool IsPreviewVersion(string[] versionParts) { - return versionParts[2].Contains("-preview") && versionParts.Length == 4; + // Check if we have enough parts and third part contains preview designation + return versionParts.Length >= 3 && + versionParts[2].Contains("-preview") && + versionParts.Length == 4; // Last check ensures we have preview number }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private static bool IsPreviewVersion(string[] versionParts) { // Check if we have enough parts and third part contains preview designation return versionParts.Length >= 3 && versionParts[2].Contains("-preview") && versionParts.Length == 4; // Last check ensures we have preview number }
28-36: 🛠️ Refactor suggestion
Consider breaking down complex condition and adding exception handling for the conversion.
The current implementation has a complex condition and lacks error handling for the numeric conversion, which could lead to runtime exceptions.
string normalVersion = version.Split('-').First(); - int[] numberVersion = [.. normalVersion.Split('.').Take(3).Select((split) => Convert.ToInt32(split))]; - if (IsVersionOrBelow(numberVersion, [8, 0, 0]) && IsPreviewVersion(versionParts) && GetPreviewVersion(versionParts) < MINIMUM_PREVIEW_NUGET_VERSION) + + try + { + int[] numberVersion = normalVersion.Split('.') + .Take(3) + .Select(split => Convert.ToInt32(split)) + .ToArray(); + + bool isBelowOrEqualToBreakpoint = IsVersionOrBelow(numberVersion, new int[] { 8, 0, 0 }); + bool isPreviewVersion = IsPreviewVersion(versionParts); + bool isBelowMinimumPreviewVersion = isPreviewVersion && GetPreviewVersion(versionParts) < MINIMUM_PREVIEW_NUGET_VERSION; + + if (isBelowOrEqualToBreakpoint && isBelowMinimumPreviewVersion) + { + return string.Empty; + } + } + catch (FormatException) + { + // Invalid version format, return empty + return string.Empty; + } + catch (OverflowException) + { + // Version number too large, return empty + return string.Empty; + } { return string.Empty; } return MINIMUM_NUGET_VERSION;Committable suggestion skipped: line range outside the PR's diff.
framitdavid
added
the
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
framitdavid
deleted the
fix/applicationmetadata-version-mock-breaks-for-preview-after-v4
branch
Description
Adds a new helper function to check if semantic version is below or equal to a version, and only returns empty string for 8.0.0-preview.15 or below.
Related Issue(s)
Issue thread from slack: https://digdir.slack.com/archives/C0760NPT2BE/p1740395650522149
Verification
Documentation
Summary by CodeRabbit