Make adding overlapping ARP range a hard error (#4870)

## Change
Implements two independent but related changes:
1. Makes adding/updating an overlapping ARP range a hard error
2. Adds `AddOrUpdate` functionality to the index so that callers don't
need to know if the manifest is already present

## Validation
Added/updated unit tests for ARP overlap.
Added unit and interop test for AddOrUpdate.

src/AppInstallerCLITests/SQLiteIndex.cpp

@@ -3241,7 +3241,7 @@ TEST_CASE("SQLiteIndex_RemoveManifestArpVersionKeepUsedDeleteUnused", "[sqlitein
     }
 }
 
-TEST_CASE("SQLiteIndex_ManifestArpVersion_CheckConsistency", "[sqliteindex]")
+TEST_CASE("SQLiteIndex_ManifestArpVersionConflict_AddThrows", "[sqliteindex]")
 {
     TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
     INFO("Using temporary file named: " << tempFile.GetPath());
@@ -3267,9 +3267,44 @@ TEST_CASE("SQLiteIndex_ManifestArpVersion_CheckConsistency", "[sqliteindex]")
     // Add a conflicting one
     manifest.Version = "10.1";
 
+    REQUIRE_THROWS_HR(index.AddManifest(manifest, "path2"), APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
+}
+
+TEST_CASE("SQLiteIndex_ManifestArpVersionConflict_UpdateThrows", "[sqliteindex]")
+{
+    TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
+    INFO("Using temporary file named: " << tempFile.GetPath());
+
+    SQLiteIndex index = CreateTestIndex(tempFile, SQLiteVersion::Latest());
+
+    Manifest manifest;
+    manifest.Id = "Foo";
+    manifest.Version = "10.0";
+    manifest.DefaultLocalization.Add<Localization::PackageName>("ArpVersionCheckConsistencyTest");
+    manifest.Moniker = "testmoniker";
+    manifest.Installers.push_back({});
+    manifest.Installers[0].BaseInstallerType = InstallerTypeEnum::Exe;
+    manifest.Installers[0].AppsAndFeaturesEntries.push_back({});
+    manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayVersion = "1.0";
+    manifest.Installers[0].AppsAndFeaturesEntries.push_back({});
+    manifest.Installers[0].AppsAndFeaturesEntries[1].DisplayVersion = "1.1";
+
+    index.AddManifest(manifest, "path");
+    REQUIRE(index.CheckConsistency(true));
+
+    // Add another version
+    manifest.Version = "10.1";
+    manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayVersion = "2.0";
+    manifest.Installers[0].AppsAndFeaturesEntries[1].DisplayVersion = "2.1";
+
     index.AddManifest(manifest, "path2");
+    REQUIRE(index.CheckConsistency(true));
 
-    REQUIRE_FALSE(index.CheckConsistency(true));
+    // Update to a conflict
+    manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayVersion = "1.0";
+    manifest.Installers[0].AppsAndFeaturesEntries[1].DisplayVersion = "2.1";
+
+    REQUIRE_THROWS_HR(index.UpdateManifest(manifest, "path2"), APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
 }
 
 TEST_CASE("SQLiteIndex_ManifestArpVersion_ValidateManifestAgainstIndex", "[sqliteindex]")
@@ -3856,3 +3891,47 @@ TEST_CASE("SQLiteIndex_DependencyWithCaseMismatch", "[sqliteindex][V1_4]")
 
     index.AddManifest(manifest, GetPathFromManifest(manifest));
 }
+
+TEST_CASE("SQLiteIndex_AddOrUpdateManifest", "[sqliteindex]")
+{
+    TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
+    INFO("Using temporary file named: " << tempFile.GetPath());
+
+    std::string manifestPath = "test/id/test.id-1.0.0.yaml";
+    Manifest manifest;
+    manifest.Installers.push_back({});
+    manifest.Id = "test.id";
+    manifest.DefaultLocalization.Add < Localization::PackageName>("Test Name");
+    manifest.Moniker = "testmoniker";
+    manifest.Version = "1.0.0";
+    manifest.Channel = "test";
+    manifest.DefaultLocalization.Add<Localization::Tags>({ "t1", "t2" });
+    manifest.Installers[0].Commands = { "test1", "test2" };
+
+    {
+        auto version = GENERATE(SQLiteVersion{ 1, 0 }, SQLiteVersion::Latest());
+        SQLiteIndex index = SQLiteIndex::CreateNew(tempFile, version);
+
+        REQUIRE(index.AddOrUpdateManifest(manifest, manifestPath));
+    }
+
+    {
+        SQLiteIndex index = SQLiteIndex::Open(tempFile, SQLiteStorageBase::OpenDisposition::ReadWrite);
+
+        // Update with no updates should return false
+        REQUIRE(!index.AddOrUpdateManifest(manifest, manifestPath));
+
+        manifest.DefaultLocalization.Add<Localization::Description>("description2");
+
+        // Update with no indexed updates should return false
+        REQUIRE(!index.AddOrUpdateManifest(manifest, manifestPath));
+
+        // Update with indexed changes
+        manifest.DefaultLocalization.Add<Localization::PackageName>("Test Name2");
+        manifest.Moniker = "testmoniker2";
+        manifest.DefaultLocalization.Add<Localization::Tags>({ "t1", "t2", "t3" });
+        manifest.Installers[0].Commands = {};
+
+        REQUIRE(index.AddOrUpdateManifest(manifest, manifestPath));
+    }
+}

src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp

@@ -109,6 +109,11 @@ namespace AppInstaller::Repository::Microsoft
     SQLiteIndex::IdType SQLiteIndex::AddManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
     {
         std::lock_guard<std::mutex> lockInterface{ *m_interfaceLock };
+        return AddManifestInternalHoldingLock(manifest, relativePath);
+    }
+
+    SQLiteIndex::IdType SQLiteIndex::AddManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
+    {
         AICLI_LOG(Repo, Verbose, << "Adding manifest for [" << manifest.Id << ", " << manifest.Version << "] at relative path [" << relativePath.value_or("") << "]");
 
         SQLite::Savepoint savepoint = SQLite::Savepoint::Create(m_dbconn, "sqliteindex_addmanifest");
@@ -143,6 +148,11 @@ namespace AppInstaller::Repository::Microsoft
     bool SQLiteIndex::UpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
     {
         std::lock_guard<std::mutex> lockInterface{ *m_interfaceLock };
+        return UpdateManifestInternalHoldingLock(manifest, relativePath);
+    }
+
+    bool SQLiteIndex::UpdateManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
+    {
         AICLI_LOG(Repo, Verbose, << "Updating manifest for [" << manifest.Id << ", " << manifest.Version << "] at relative path [" << relativePath.value_or("") << "]");
 
         SQLite::Savepoint savepoint = SQLite::Savepoint::Create(m_dbconn, "sqliteindex_updatemanifest");
@@ -159,6 +169,40 @@ namespace AppInstaller::Repository::Microsoft
         return result;
     }
 
+    bool SQLiteIndex::AddOrUpdateManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath)
+    {
+        AICLI_LOG(Repo, Verbose, << "Adding or Updating manifest from file [" << manifestPath << "]");
+
+        Manifest::Manifest manifest = Manifest::YamlParser::CreateFromPath(manifestPath);
+        return AddOrUpdateManifestInternal(manifest, relativePath);
+    }
+
+    bool SQLiteIndex::AddOrUpdateManifest(const Manifest::Manifest& manifest, const std::filesystem::path& relativePath)
+    {
+        return AddOrUpdateManifestInternal(manifest, relativePath);
+    }
+
+    bool SQLiteIndex::AddOrUpdateManifest(const Manifest::Manifest& manifest)
+    {
+        return AddOrUpdateManifestInternal(manifest, {});
+    }
+
+    bool SQLiteIndex::AddOrUpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
+    {
+        std::lock_guard<std::mutex> lockInterface{ *m_interfaceLock };
+        AICLI_LOG(Repo, Verbose, << "Adding or Updating manifest for [" << manifest.Id << ", " << manifest.Version << "] at relative path [" << relativePath.value_or("") << "]");
+
+        if (m_interface->GetManifestIdByManifest(m_dbconn, manifest))
+        {
+            return UpdateManifestInternalHoldingLock(manifest, relativePath);
+        }
+        else
+        {
+            AddManifestInternalHoldingLock(manifest, relativePath);
+            return true;
+        }
+    }
+
     void SQLiteIndex::RemoveManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath)
     {
         AICLI_LOG(Repo, Verbose, << "Removing manifest from file [" << manifestPath << "]");

src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h

@@ -94,6 +94,18 @@ namespace AppInstaller::Repository::Microsoft
         // The return value indicates whether the index was modified by the function.
         bool UpdateManifest(const Manifest::Manifest& manifest);
 
+        // Adds or updates the manifest with matching { Id, Version, Channel } in the index.
+        // The return value indicates whether the index was modified by the function.
+        bool AddOrUpdateManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath);
+
+        // Updates the manifest with matching { Id, Version, Channel } in the index.
+        // The return value indicates whether the index was modified by the function.
+        bool AddOrUpdateManifest(const Manifest::Manifest& manifest, const std::filesystem::path& relativePath);
+
+        // Updates the manifest with matching { Id, Version, Channel } in the index.
+        // The return value indicates whether the index was modified by the function.
+        bool AddOrUpdateManifest(const Manifest::Manifest& manifest);
+
         // Removes the manifest with matching { Id, Version, Channel } from the index.
         void RemoveManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath);
 
@@ -176,7 +188,10 @@ namespace AppInstaller::Repository::Microsoft
 
         // Internal functions to normalize on the relativePath being present.
         IdType AddManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
+        IdType AddManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
         bool UpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
+        bool UpdateManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
+        bool AddOrUpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
 
         std::unique_ptr<Schema::ISQLiteIndex> m_interface;
         Schema::SQLiteIndexContextData m_contextData;

src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface.h

@@ -26,8 +26,11 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
         // Gets a property already knowing that the manifest id is valid.
         std::optional<std::string> GetPropertyByManifestIdInternal(const SQLite::Connection& connection, SQLite::rowid_t manifestId, PackageVersionProperty property) const override;
 
-    private:
+    private:
+        // Gets the ARP version ranges for the given package identifier.
+        std::vector<Utility::VersionRange> GetArpVersionRanges(const SQLite::Connection& connection, SQLite::rowid_t packageIdentifier) const;
+
         // Semantic check to validate all arp version ranges within the index
         bool ValidateArpVersionConsistency(const SQLite::Connection& connection, bool log) const;
     };
-}
+}

src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface_1_5.cpp

@@ -4,6 +4,7 @@
 #include "Microsoft/Schema/1_5/Interface.h"
 #include "Microsoft/Schema/1_5/ArpVersionVirtualTable.h"
 #include "Microsoft/Schema/1_0/ManifestTable.h"
+#include "Microsoft/Schema/1_0/IdTable.h"
 #include "Microsoft/Schema/1_0/VersionTable.h"
 
 namespace AppInstaller::Repository::Microsoft::Schema::V1_5
@@ -36,8 +37,32 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
         SQLite::rowid_t manifestId = V1_4::Interface::AddManifest(connection, manifest, relativePath);
 
         auto arpVersionRange = manifest.GetArpVersionRange();
-        Manifest::string_t arpMinVersion = arpVersionRange.IsEmpty() ? "" : arpVersionRange.GetMinVersion().ToString();
-        Manifest::string_t arpMaxVersion = arpVersionRange.IsEmpty() ? "" : arpVersionRange.GetMaxVersion().ToString();
+        Manifest::string_t arpMinVersion, arpMaxVersion;
+
+        if (!arpVersionRange.IsEmpty())
+        {
+            // Check to see if adding this version range will create a conflict
+            SQLite::rowid_t packageIdentifier = V1_0::ManifestTable::GetIdById<V1_0::IdTable>(connection, manifestId).value();
+            std::vector<Utility::VersionRange> ranges = GetArpVersionRanges(connection, packageIdentifier);
+            ranges.push_back(arpVersionRange);
+
+            if (Utility::HasOverlapInVersionRanges(ranges))
+            {
+                AICLI_LOG(Repo, Error, << "Overlapped Arp version ranges found for package. All ranges currently in index followed by new range:\n" << [&]() {
+                        std::stringstream stream;
+                        for (const auto& range : ranges)
+                        {
+                            stream << '[' << range.GetMinVersion().ToString() << "] - [" << range.GetMaxVersion().ToString() << "]\n";
+                        }
+                        return std::move(stream).str();
+                    }());
+                THROW_HR(APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
+            }
+
+            arpMinVersion = arpVersionRange.GetMinVersion().ToString();
+            arpMaxVersion = arpVersionRange.GetMaxVersion().ToString();
+        }
+
         SQLite::rowid_t arpMinVersionId = V1_0::VersionTable::EnsureExists(connection, arpMinVersion);
         SQLite::rowid_t arpMaxVersionId = V1_0::VersionTable::EnsureExists(connection, arpMaxVersion);
         V1_0::ManifestTable::UpdateValueIdById<ArpMinVersionVirtualTable>(connection, manifestId, arpMinVersionId);
@@ -82,6 +107,27 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
             indexModified = true;
         }
 
+        if (!arpVersionRange.IsEmpty())
+        {
+            // Check to see if the new set of version ranges created a conflict.
+            // We could have done this before attempting the update but it would be more complex and SQLite gives us easy rollback.
+            SQLite::rowid_t packageIdentifier = V1_0::ManifestTable::GetIdById<V1_0::IdTable>(connection, manifestId).value();
+            std::vector<Utility::VersionRange> ranges = GetArpVersionRanges(connection, packageIdentifier);
+
+            if (Utility::HasOverlapInVersionRanges(ranges))
+            {
+                AICLI_LOG(Repo, Error, << "Overlapped Arp version ranges found for package. Ranges that would be present with attempted upgrade:\n" << [&]() {
+                        std::stringstream stream;
+                        for (const auto& range : ranges)
+                        {
+                            stream << '[' << range.GetMinVersion().ToString() << "] - [" << range.GetMaxVersion().ToString() << "]\n";
+                        }
+                        return std::move(stream).str();
+                    }());
+                THROW_HR(APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
+            }
+        }
+
         if (cleanOldMinVersionId && NotNeeded(connection, V1_0::VersionTable::TableName(), V1_0::VersionTable::ValueName(), oldMinVersionId))
         {
             V1_0::VersionTable::DeleteById(connection, oldMinVersionId);
@@ -181,6 +227,26 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
         }
     }
 
+    std::vector<Utility::VersionRange> Interface::GetArpVersionRanges(const SQLite::Connection& connection, SQLite::rowid_t packageIdentifier) const
+    {
+        std::vector<Utility::VersionRange> ranges;
+        auto versionKeys = GetVersionKeysById(connection, packageIdentifier);
+        for (auto const& versionKey : versionKeys)
+        {
+            auto arpMinVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMinVersion).value_or("");
+            auto arpMaxVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMaxVersion).value_or("");
+
+            // Either both empty or both not empty
+            THROW_HR_IF(E_UNEXPECTED, arpMinVersion.empty() != arpMaxVersion.empty());
+
+            if (!arpMinVersion.empty() && !arpMaxVersion.empty())
+            {
+                ranges.emplace_back(Utility::VersionRange{ Utility::Version{ std::move(arpMinVersion) }, Utility::Version{ std::move(arpMaxVersion) } });
+            }
+        }
+        return ranges;
+    }
+
     bool Interface::ValidateArpVersionConsistency(const SQLite::Connection& connection, bool log) const
     {
         try
@@ -193,21 +259,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
             for (auto const& match : searchResult.Matches)
             {
                 // Get arp version ranges for each package to check
-                std::vector<Utility::VersionRange> ranges;
-                auto versionKeys = GetVersionKeysById(connection, match.first);
-                for (auto const& versionKey : versionKeys)
-                {
-                    auto arpMinVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMinVersion).value_or("");
-                    auto arpMaxVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMaxVersion).value_or("");
-
-                    // Either both empty or both not empty
-                    THROW_HR_IF(E_UNEXPECTED, arpMinVersion.empty() != arpMaxVersion.empty());
-
-                    if (!arpMinVersion.empty() && !arpMaxVersion.empty())
-                    {
-                        ranges.emplace_back(Utility::VersionRange{ Utility::Version{ std::move(arpMinVersion) }, Utility::Version{ std::move(arpMaxVersion) } });
-                    }
-                }
+                std::vector<Utility::VersionRange> ranges = GetArpVersionRanges(connection, match.first);
 
                 // Check overlap
                 if (Utility::HasOverlapInVersionRanges(ranges))

src/WinGetUtil/Exports.cpp

@@ -192,6 +192,26 @@ extern "C"
     }
     CATCH_RETURN()
 
+    WINGET_UTIL_API WinGetSQLiteIndexAddOrUpdateManifest(
+        WINGET_SQLITE_INDEX_HANDLE index,
+        WINGET_STRING manifestPath,
+        WINGET_STRING relativePath,
+        BOOL* indexModified) try
+    {
+        THROW_HR_IF(E_INVALIDARG, !index);
+        THROW_HR_IF(E_INVALIDARG, !manifestPath);
+        THROW_HR_IF(E_INVALIDARG, !relativePath);
+
+        bool result = reinterpret_cast<SQLiteIndex*>(index)->AddOrUpdateManifest(manifestPath, relativePath);
+        if (indexModified)
+        {
+            *indexModified = (result ? TRUE : FALSE);
+        }
+
+        return S_OK;
+    }
+    CATCH_RETURN()
+
     WINGET_UTIL_API WinGetSQLiteIndexRemoveManifest(
         WINGET_SQLITE_INDEX_HANDLE index, 
         WINGET_STRING manifestPath,

src/WinGetUtil/Source.def

@@ -7,6 +7,7 @@ EXPORTS
     WinGetSQLiteIndexClose
     WinGetSQLiteIndexAddManifest
     WinGetSQLiteIndexUpdateManifest
+    WinGetSQLiteIndexAddOrUpdateManifest
     WinGetSQLiteIndexRemoveManifest
     WinGetSQLiteIndexPrepareForPackaging
     WinGetSQLiteIndexCheckConsistency

src/WinGetUtil/WinGetUtil.h

@@ -158,6 +158,14 @@ extern "C"
         WINGET_STRING relativePath,
         BOOL* indexModified);
 
+    // Adds or Updates the manifest with matching { Id, Version, Channel } in the index.
+    // The return value indicates whether the index was modified by the function.
+    WINGET_UTIL_API WinGetSQLiteIndexAddOrUpdateManifest(
+        WINGET_SQLITE_INDEX_HANDLE index,
+        WINGET_STRING manifestPath,
+        WINGET_STRING relativePath,
+        BOOL* indexModified);
+
     // Removes the manifest with matching { Id, Version, Channel } from the index.
     // Path is currently ignored.
     WINGET_UTIL_API WinGetSQLiteIndexRemoveManifest(