Better handling of min version merging in DependencyList (#4997)

[Cherry pick #4987 to 1.9]

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`.

Author: JohnMcPMS

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);
+    }
+}

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,15 +1157,15 @@ 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)
         {
             if (dependency.Type == type && Utility::ICUCaseInsensitiveEquals(dependency.Id(), id))
             {
                 if (!minVersion.empty())
                 {
-                    return dependency.MinVersion.has_value() && dependency.MinVersion.value() == Utility::Version{ minVersion };
+                    return dependency.MinVersion == Utility::Version{ minVersion };
                 }
                 else
                 {

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: