From 1deb4a8b11a116ef902e8b0ac5fa3d3ec38dc008 Mon Sep 17 00:00:00 2001 From: Kaleb Luedtke Date: Fri, 22 Nov 2024 22:34:08 -0600 Subject: [PATCH] "Pad" shorter versions with empty parts when comparing (#5001) - [x] I have signed the [Contributor License Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs). - [x] This pull request is related to an issue. - Resolves #4998 When versions are created, any empty parts are trimmed off. This means that `22.0.0` is stored as `[22]` in memory. This causes an issue when comparing to a version that had additional parts, but should be sorted lower than the version which is trimmed, such as `22.0.0-rc`. This is due to the comparator seeing that `[22]` has no more parts to compare after the first iteration, while `[22, 0, 0-rc]` does, causing it to hit the condition where whichever version has more parts is considered to be greater. This PR updates the logic so that the version comparison effectively contains as many empty parts as are needed to fully complete the comparison. This retains the logic that if a part is not equal to the part it is being compared against, the result of that comparison will be returned but eliminates the need for checking if the versions have the same number of parts since, effectively, they do; it's just that the extra parts needed to complete the comparison are all empty. Additional tests have been added to cover this corner case. If possible, I would ask that this also be cherry-picked into any future 1.9 releases (assuming this PR passes review and merges) ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/5001) --- src/AppInstallerCLITests/SQLiteIndex.cpp | 4 +-- src/AppInstallerCLITests/Versions.cpp | 19 +++++++++++-- .../Public/AppInstallerVersions.h | 7 +++-- src/AppInstallerSharedLib/Versions.cpp | 27 +++++-------------- 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/AppInstallerCLITests/SQLiteIndex.cpp b/src/AppInstallerCLITests/SQLiteIndex.cpp index fb381ebb72..0166cebac1 100644 --- a/src/AppInstallerCLITests/SQLiteIndex.cpp +++ b/src/AppInstallerCLITests/SQLiteIndex.cpp @@ -1911,8 +1911,8 @@ TEST_CASE("SQLiteIndex_Search_VersionSorting", "[sqliteindex]") { { UtilityVersion("15.0.0"), Channel("") }, { UtilityVersion("14.0.0"), Channel("") }, - { UtilityVersion("13.2.0-bugfix"), Channel("") }, { UtilityVersion("13.2.0"), Channel("") }, + { UtilityVersion("13.2.0-bugfix"), Channel("") }, { UtilityVersion("13.0.0"), Channel("") }, { UtilityVersion("16.0.0"), Channel("alpha") }, { UtilityVersion("15.8.0"), Channel("alpha") }, @@ -1966,8 +1966,8 @@ TEST_CASE("SQLiteIndex_PathString_VersionSorting", "[sqliteindex]") { { UtilityVersion("15.0.0"), Channel("") }, { UtilityVersion("14.0.0"), Channel("") }, - { UtilityVersion("13.2.0-bugfix"), Channel("") }, { UtilityVersion("13.2.0"), Channel("") }, + { UtilityVersion("13.2.0-bugfix"), Channel("") }, { UtilityVersion("13.0.0"), Channel("") }, { UtilityVersion("16.0.0"), Channel("alpha") }, { UtilityVersion("15.8.0"), Channel("alpha") }, diff --git a/src/AppInstallerCLITests/Versions.cpp b/src/AppInstallerCLITests/Versions.cpp index 54975eda49..5a05be6db1 100644 --- a/src/AppInstallerCLITests/Versions.cpp +++ b/src/AppInstallerCLITests/Versions.cpp @@ -154,6 +154,20 @@ TEST_CASE("VersionCompare", "[versions]") RequireLessThan("0.0.1-beta", "0.0.2-alpha"); RequireLessThan("13.9.8", "14.1"); + // Ensure that versions with non-digit characters in their parts are sorted correctly + RequireLessThan("1-rc", "1"); + RequireLessThan("1.2-rc", "1.2"); + RequireLessThan("1.0-rc", "1.0"); + RequireLessThan("1.0.0-rc", "1"); + RequireLessThan("22.0.0-rc.1", "22.0.0"); + RequireLessThan("22.0.0-rc.1", "22.0.0.1"); + RequireLessThan("22.0.0-rc.1", "22.0.0.1-rc"); + + // Ensure that Sub-RC versions are sorted correctly + RequireLessThan("22.0.0-rc.1", "22.0.0-rc.1.1"); + RequireLessThan("22.0.0-rc.1.1", "22.0.0-rc.1.2"); + RequireLessThan("22.0.0-rc.1.2", "22.0.0-rc.2"); + RequireEqual("1.0", "1.0.0"); // Ensure whitespace doesn't affect equality @@ -175,15 +189,16 @@ TEST_CASE("VersionAndChannelSort", "[versions]") { { Version("15.0.0"), Channel("") }, { Version("14.0.0"), Channel("") }, - { Version("13.2.0-bugfix"), Channel("") }, + { Version("13.2.1-bugfix"), Channel("") }, { Version("13.2.0"), Channel("") }, + { Version("13.2.0-rc"), Channel("") }, { Version("13.0.0"), Channel("") }, { Version("16.0.0"), Channel("alpha") }, { Version("15.8.0"), Channel("alpha") }, { Version("15.1.0"), Channel("beta") }, }; - std::vector reorderList = { 4, 2, 1, 7, 6, 3, 5, 0 }; + std::vector reorderList = { 4, 2, 1, 7, 6, 3, 8, 5, 0 }; REQUIRE(sortedList.size() == reorderList.size()); std::vector jumbledList; diff --git a/src/AppInstallerSharedLib/Public/AppInstallerVersions.h b/src/AppInstallerSharedLib/Public/AppInstallerVersions.h index 9df7cab413..49dd4f1e99 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerVersions.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerVersions.h @@ -18,12 +18,11 @@ namespace AppInstaller::Utility // // Versions are compared by: // for each part in each version - // if both sides have no more parts, return equal - // else if one side has no more parts, it is less - // else if integers not equal, return comparison of integers + // if one side has no more parts, perform the remaining comparisons against an empty part + // if integers not equal, return comparison of integers // else if only one side has a non-empty string part, it is less // else if string parts not equal, return comparison of strings - // if all parts are same, use approximate comparator if applicable + // if each part has been compared, use approximate comparator if applicable // // Note: approximate to another approximate version is invalid. // approximate to Unknown is invalid. diff --git a/src/AppInstallerSharedLib/Versions.cpp b/src/AppInstallerSharedLib/Versions.cpp index 2c9972c561..f268146f27 100644 --- a/src/AppInstallerSharedLib/Versions.cpp +++ b/src/AppInstallerSharedLib/Versions.cpp @@ -137,16 +137,12 @@ namespace AppInstaller::Utility return (thisIsUnknown && !otherIsUnknown); } - for (size_t i = 0; i < m_parts.size(); ++i) + const Part emptyPart{}; + for (size_t i = 0; i < std::max(m_parts.size(), other.m_parts.size()); ++i) { - if (i >= other.m_parts.size()) - { - // All parts equal to this point - break; - } - - const Part& partA = m_parts[i]; - const Part& partB = other.m_parts[i]; + // Whichever version is shorter, we need to pad it with empty parts + const Part& partA = (i >= m_parts.size()) ? emptyPart : m_parts[i]; + const Part& partB = (i >= other.m_parts.size()) ? emptyPart : other.m_parts[i]; if (partA < partB) { @@ -159,17 +155,8 @@ namespace AppInstaller::Utility // else parts are equal, so continue to next part } - // All parts tested were equal - if (m_parts.size() == other.m_parts.size()) - { - return ApproximateCompareLessThan(other); - } - else - { - // Else this is only less if there are more parts in other. - return m_parts.size() < other.m_parts.size(); - } - + // All parts were compared and found to be equal + return ApproximateCompareLessThan(other); } bool Version::operator>(const Version& other) const