From 03df8247284d8b74aabe810871e6da9e0f5dbc5f Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Tue, 19 Nov 2024 11:25:43 -0800 Subject: [PATCH] Better handling of min version merging in DependencyList (#4987) Likely fix for #4972 ## Change Use `std::optional` overloaded operator to handle all of the comparisons in `DependencyList::Add`. The operator already properly handles all of the cases, including treating `std::nullopt` as always less than a defined value. Also optimize a few other places around a reference to `MinVersion`. ## Validation Added a unit test covering the cases where `Add` needs to merge the minimum version value. --- src/AppInstallerCLITests/Dependencies.cpp | 63 +++++++++++++++++++ .../Manifest/ManifestCommon.cpp | 23 ++----- .../Public/winget/ManifestCommon.h | 8 +-- 3 files changed, 73 insertions(+), 21 deletions(-) diff --git a/src/AppInstallerCLITests/Dependencies.cpp b/src/AppInstallerCLITests/Dependencies.cpp index 5000330894..09340a07a1 100644 --- a/src/AppInstallerCLITests/Dependencies.cpp +++ b/src/AppInstallerCLITests/Dependencies.cpp @@ -209,3 +209,66 @@ TEST_CASE("DependencyNodeProcessor_NoMatches", "[dependencies]") REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::DependenciesFlowNoMatches)) != std::string::npos); REQUIRE(result == DependencyNodeProcessorResult::Error); } + +TEST_CASE("DependencyList_Add_MinVersion", "[dependencies]") +{ + DependencyType type = DependencyType::Package; + std::string identifier = "Identifier"; + + DependencyList list; + Dependency dependencyWithoutMinVersion{ type, identifier }; + Dependency dependencyWithLowerMinVersion{ type, identifier, "1.0" }; + Dependency dependencyWithHigherMinVersion{ type, identifier, "3.0" }; + + Dependency dependencyToAdd{ type, identifier, "2.0" }; + + SECTION("Existing dependency has no min version, added does") + { + list.Add(dependencyWithoutMinVersion); + list.Add(dependencyToAdd); + + const Dependency* dependency = list.HasDependency(dependencyToAdd); + REQUIRE(dependency != nullptr); + REQUIRE(dependency->MinVersion.has_value()); + REQUIRE(dependency->MinVersion == dependencyToAdd.MinVersion); + } + SECTION("Existing dependency has lower min version") + { + list.Add(dependencyWithLowerMinVersion); + list.Add(dependencyToAdd); + + const Dependency* dependency = list.HasDependency(dependencyToAdd); + REQUIRE(dependency != nullptr); + REQUIRE(dependency->MinVersion.has_value()); + REQUIRE(dependency->MinVersion == dependencyToAdd.MinVersion); + } + SECTION("Existing dependency has higher min version") + { + list.Add(dependencyWithHigherMinVersion); + list.Add(dependencyToAdd); + + const Dependency* dependency = list.HasDependency(dependencyToAdd); + REQUIRE(dependency != nullptr); + REQUIRE(dependency->MinVersion.has_value()); + REQUIRE(dependency->MinVersion == dependencyWithHigherMinVersion.MinVersion); + } + SECTION("Existing dependency has no min version, neither does added") + { + list.Add(dependencyWithoutMinVersion); + list.Add(dependencyWithoutMinVersion); + + const Dependency* dependency = list.HasDependency(dependencyToAdd); + REQUIRE(dependency != nullptr); + REQUIRE(!dependency->MinVersion.has_value()); + } + SECTION("Existing dependency has min version, added does not") + { + list.Add(dependencyWithHigherMinVersion); + list.Add(dependencyWithoutMinVersion); + + const Dependency* dependency = list.HasDependency(dependencyToAdd); + REQUIRE(dependency != nullptr); + REQUIRE(dependency->MinVersion.has_value()); + REQUIRE(dependency->MinVersion == dependencyWithHigherMinVersion.MinVersion); + } +} diff --git a/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp b/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp index 108097561f..16e759441c 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp @@ -1113,22 +1113,11 @@ namespace AppInstaller::Manifest { Dependency* existingDependency = this->HasDependency(newDependency); - if (existingDependency != NULL) { - if (newDependency.MinVersion) + if (existingDependency != NULL) + { + if (newDependency.MinVersion > existingDependency->MinVersion) { - if (existingDependency->MinVersion) - { - const auto& newDependencyVersion = Utility::Version(newDependency.MinVersion.value()); - const auto& existingDependencyVersion = Utility::Version(existingDependency->MinVersion.value()); - if (newDependencyVersion > existingDependencyVersion) - { - existingDependency->MinVersion.value() = newDependencyVersion.ToString(); - } - } - else - { - existingDependency->MinVersion.value() = newDependency.MinVersion.value(); - } + existingDependency->MinVersion = newDependency.MinVersion; } } else @@ -1168,7 +1157,7 @@ namespace AppInstaller::Manifest } // for testing purposes - bool DependencyList::HasExactDependency(DependencyType type, string_t id, string_t minVersion) + bool DependencyList::HasExactDependency(DependencyType type, const string_t& id, const string_t& minVersion) { for (const auto& dependency : m_dependencies) { @@ -1176,7 +1165,7 @@ namespace AppInstaller::Manifest { if (!minVersion.empty()) { - return dependency.MinVersion.has_value() && dependency.MinVersion.value() == Utility::Version{ minVersion }; + return dependency.MinVersion == Utility::Version{ minVersion }; } else { diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h b/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h index a501191eb6..fd11c47de7 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h +++ b/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h @@ -271,7 +271,7 @@ namespace AppInstaller::Manifest const string_t& Id() const { return m_id; }; std::optional MinVersion; - Dependency(DependencyType type, string_t id, string_t minVersion) : Type(type), m_id(std::move(id)), MinVersion(Utility::Version(minVersion)), m_foldedId(FoldCase(m_id)) {} + Dependency(DependencyType type, string_t id, string_t minVersion) : Type(type), m_id(std::move(id)), MinVersion(Utility::Version(std::move(minVersion))), m_foldedId(FoldCase(m_id)) {} Dependency(DependencyType type, string_t id) : Type(type), m_id(std::move(id)), m_foldedId(FoldCase(m_id)){} Dependency(DependencyType type) : Type(type) {} @@ -285,9 +285,9 @@ namespace AppInstaller::Manifest return m_foldedId < rhs.m_foldedId; } - bool IsVersionOk(Utility::Version version) + bool IsVersionOk(const Utility::Version& version) { - return MinVersion <= Utility::Version(version); + return MinVersion <= version; } // m_foldedId should be set whenever Id is set @@ -313,7 +313,7 @@ namespace AppInstaller::Manifest void ApplyToAll(std::function func) const; bool Empty() const; void Clear(); - bool HasExactDependency(DependencyType type, string_t id, string_t minVersion = ""); + bool HasExactDependency(DependencyType type, const string_t& id, const string_t& minVersion = ""); size_t Size(); private: