commit 03df8247284d8b74aabe810871e6da9e0f5dbc5f
parent 5645e0cd4def124ce717f1532f1a216564b951e1
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Tue, 19 Nov 2024 11:25:43 -0800
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.
Diffstat:
3 files changed, 73 insertions(+), 21 deletions(-)
diff --git 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
@@ -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
@@ -271,7 +271,7 @@ namespace AppInstaller::Manifest
const string_t& Id() const { return m_id; };
std::optional<Utility::Version> 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<void(const Dependency&)> 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:
