From b5eae14b7fdda52a920f75091660a6fcca62d523 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Thu, 24 Oct 2024 18:12:33 -0700 Subject: [PATCH] [PR Feedback] Refactor error handling and optimize data structures - Updated data structures for `progressWeights` to account for edge case. - Refined progress reporting logic to handle edge cases and ensure accurate tracking. - Replaced `std::wstring` with `hstring` in Add/Removal headers get a perf. benefits. - Updated exception types in tests to reflect actual exceptions thrown. - Removed unnecessary headers. --- .../Interop/PackageCatalogInterop.cs | 4 +- .../Public/AppInstallerProgress.h | 1 - .../AddPackageCatalogOptions.h | 8 +-- .../Converters.cpp | 2 - .../PackageCatalogProgress.cpp | 55 +++++++++++-------- .../PackageCatalogProgress.h | 4 +- .../PackageManager.cpp | 29 ++-------- .../PackageManager.idl | 14 +++++ .../RemovePackageCatalogOptions.h | 2 +- 9 files changed, 61 insertions(+), 58 deletions(-) diff --git a/src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs b/src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs index 23e133fb00..c38cb36f6b 100644 --- a/src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs +++ b/src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs @@ -77,7 +77,7 @@ public async Task AddRemovePackageCatalog() public void AddPackageCatalogWithInvalidOptions() { // Add package catalog with null options. - Assert.ThrowsAsync(async () => await this.packageManager.AddPackageCatalogAsync(null)); + Assert.ThrowsAsync(async () => await this.packageManager.AddPackageCatalogAsync(null)); // Add package catalog with empty options. Assert.ThrowsAsync(async () => await this.packageManager.AddPackageCatalogAsync(this.TestFactory.CreateAddPackageCatalogOptions())); @@ -235,7 +235,7 @@ public async Task AddRemovePackageCatalogWithPreserveDataFiledSet() public void RemovePackageCatalogWithInvalidOptions() { // Remove package catalog with null options. - Assert.ThrowsAsync(async () => await this.packageManager.RemovePackageCatalogAsync(null)); + Assert.ThrowsAsync(async () => await this.packageManager.RemovePackageCatalogAsync(null)); // Remove package catalog with empty options. Assert.ThrowsAsync(async () => await this.packageManager.RemovePackageCatalogAsync(this.TestFactory.CreateRemovePackageCatalogOptions())); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerProgress.h b/src/AppInstallerCommonCore/Public/AppInstallerProgress.h index 5f9a08dcc2..7d7460b137 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerProgress.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerProgress.h @@ -5,7 +5,6 @@ #include #include #include -#include namespace AppInstaller { diff --git a/src/Microsoft.Management.Deployment/AddPackageCatalogOptions.h b/src/Microsoft.Management.Deployment/AddPackageCatalogOptions.h index 91ec268cad..d7a32b774e 100644 --- a/src/Microsoft.Management.Deployment/AddPackageCatalogOptions.h +++ b/src/Microsoft.Management.Deployment/AddPackageCatalogOptions.h @@ -31,11 +31,11 @@ namespace winrt::Microsoft::Management::Deployment::implementation #if !defined(INCLUDE_ONLY_INTERFACE_METHODS) private: - std::wstring m_name = L""; - std::wstring m_sourceUri = L""; - std::wstring m_type = L""; + hstring m_name = L""; + hstring m_sourceUri = L""; + hstring m_type = L""; winrt::Microsoft::Management::Deployment::PackageCatalogTrustLevel m_trustLevel = winrt::Microsoft::Management::Deployment::PackageCatalogTrustLevel::None; - std::wstring m_customHeader = L""; + hstring m_customHeader = L""; bool m_explicit = false; #endif }; diff --git a/src/Microsoft.Management.Deployment/Converters.cpp b/src/Microsoft.Management.Deployment/Converters.cpp index 5412dea8f9..08338dd306 100644 --- a/src/Microsoft.Management.Deployment/Converters.cpp +++ b/src/Microsoft.Management.Deployment/Converters.cpp @@ -510,8 +510,6 @@ namespace winrt::Microsoft::Management::Deployment::implementation case APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS: case APPINSTALLER_CLI_ERROR_SOURCE_ARG_ALREADY_EXISTS: return AddPackageCatalogStatus::InvalidOptions; - case APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED: - return AddPackageCatalogStatus::CatalogError; default: return HandleCommonCatalogOperationStatus(hresult); } diff --git a/src/Microsoft.Management.Deployment/PackageCatalogProgress.cpp b/src/Microsoft.Management.Deployment/PackageCatalogProgress.cpp index 7539a7593f..e82f1d7fdb 100644 --- a/src/Microsoft.Management.Deployment/PackageCatalogProgress.cpp +++ b/src/Microsoft.Management.Deployment/PackageCatalogProgress.cpp @@ -17,21 +17,21 @@ namespace winrt::Microsoft::Management::Deployment if (sourceType.empty() || Utility::CaseInsensitiveEquals( Repository::Microsoft::PredefinedInstalledSourceFactory::Type(), sourceType)) { - std::unordered_map progressWeights; + std::vector> progressWeights; - // There is no download operation for remove operation, so remove the download progress. + // There is no download operation for remove operation, so use only percentage based progress to account for uninstall. if (removeOperation) { // it is percentage based progress. - progressWeights.insert_or_assign(AppInstaller::ProgressType::Bytes, 0); + progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Percent, 1.0)); } else { // Add/Update operation has two progress types: // 1. Bytes for downloading index and // 2. Percent for index installation. - progressWeights.insert_or_assign(AppInstaller::ProgressType::Bytes, 0.7); - progressWeights.insert_or_assign(AppInstaller::ProgressType::Percent, 0.3); + progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Bytes, 0.7)); + progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Percent, 0.3)); } return std::make_shared(progressWeights, progressReporter); @@ -52,16 +52,12 @@ namespace winrt::Microsoft::Management::Deployment } } - void CompletionOnlyProgressSink::OnProgress(uint64_t current, uint64_t maximum, AppInstaller::ProgressType type) + void CompletionOnlyProgressSink::OnProgress(uint64_t /*current*/, uint64_t /*maximum*/, AppInstaller::ProgressType /*type*/) { - UNREFERENCED_PARAMETER(current); - UNREFERENCED_PARAMETER(maximum); - UNREFERENCED_PARAMETER(type); } - void CompletionOnlyProgressSink::SetProgressMessage(std::string_view message) + void CompletionOnlyProgressSink::SetProgressMessage(std::string_view /*message*/) { - UNREFERENCED_PARAMETER(message); } void CompletionOnlyProgressSink::BeginProgress() @@ -69,13 +65,12 @@ namespace winrt::Microsoft::Management::Deployment m_progressReporter(0); } - void CompletionOnlyProgressSink::EndProgress(bool hideProgressWhenDone) + void CompletionOnlyProgressSink::EndProgress(bool /*hideProgressWhenDone*/) { - UNREFERENCED_PARAMETER(hideProgressWhenDone); m_progressReporter(100); } - PreIndexedPackageCatalogProgressSink::PreIndexedPackageCatalogProgressSink(std::unordered_map progressWeights, std::function progressReporter) : + PreIndexedPackageCatalogProgressSink::PreIndexedPackageCatalogProgressSink(std::vector> progressWeights, std::function progressReporter) : m_progressWeights(progressWeights), m_progressReporter(progressReporter) { if (!m_progressReporter) @@ -86,7 +81,7 @@ namespace winrt::Microsoft::Management::Deployment // If no weights are provided, default to percent. if (m_progressWeights.empty()) { - m_progressWeights.insert_or_assign(AppInstaller::ProgressType::Percent, 1.0); + m_progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Percent, 1.0)); } // Calculate the total weight. @@ -117,30 +112,46 @@ namespace winrt::Microsoft::Management::Deployment m_progressValues[type] = progress; double totalProgress = 0.0; + double totalWeight = 0.0; // Calculate the total progress. for (const auto& [progressType, weight] : m_progressWeights) { + double progressValue = m_progressValues[progressType]; + + // [NOTE:] Sequential execution assumption & Handling incomplete progress reports : + // This progress calculation assumes that each operation is executed sequentially, meaning the download must be complete before + // the installation begins.If the download fails, the installation will not proceed.However, there may be cases where the previous + // operation completes successfully, but its onprogress callback does not report 100% completion(e.g., the last progress report for + // the download was at 90%, but the download is complete, and the installation has started).This can result in the total progress not + // reaching 100% after the last operation completes due to the gap in the previous operation's progress report.To handle this, consider + // the progress for the last operation as complete by assigning its full weight while computing progress for the following operation. + // For example, while computing progress for the installation, consider the download operation complete even if it did not report progress + // exactly at 100%. + if (progressValue != 0) + { + totalProgress = totalWeight; + } + // Adjust the total progress value based on the weight. - totalProgress += m_progressValues[progressType] * weight; + totalWeight += weight; + totalProgress += progressValue * weight; } m_progressReporter(totalProgress * 100); } - void PreIndexedPackageCatalogProgressSink::SetProgressMessage(std::string_view message) + void PreIndexedPackageCatalogProgressSink::SetProgressMessage(std::string_view /*message*/) { - UNREFERENCED_PARAMETER(message); } void PreIndexedPackageCatalogProgressSink::BeginProgress() { - OnProgress(0, 1, AppInstaller::ProgressType::None); + m_progressReporter(0); } - void PreIndexedPackageCatalogProgressSink::EndProgress(bool hideProgressWhenDone) + void PreIndexedPackageCatalogProgressSink::EndProgress(bool /*hideProgressWhenDone*/) { - UNREFERENCED_PARAMETER(hideProgressWhenDone); - OnProgress(1, 1, AppInstaller::ProgressType::None); + m_progressReporter(100); } } diff --git a/src/Microsoft.Management.Deployment/PackageCatalogProgress.h b/src/Microsoft.Management.Deployment/PackageCatalogProgress.h index f6dfb5c396..4f175d054e 100644 --- a/src/Microsoft.Management.Deployment/PackageCatalogProgress.h +++ b/src/Microsoft.Management.Deployment/PackageCatalogProgress.h @@ -52,7 +52,7 @@ namespace winrt::Microsoft::Management::Deployment /// /// ProgressType weight map. /// Callback function that reports progress to caller. - PreIndexedPackageCatalogProgressSink(std::unordered_map progressWeights, std::function progressReporter); + PreIndexedPackageCatalogProgressSink(std::vector> progressWeights, std::function progressReporter); /// /// Reports combined progress to caller when configured for multiple progress types. @@ -66,7 +66,7 @@ namespace winrt::Microsoft::Management::Deployment void EndProgress(bool hideProgressWhenDone) override; private: - std::unordered_map m_progressWeights; + std::vector> m_progressWeights; std::function m_progressReporter; std::unordered_map m_progressValues; }; diff --git a/src/Microsoft.Management.Deployment/PackageManager.cpp b/src/Microsoft.Management.Deployment/PackageManager.cpp index ac6fa06a17..e0b3c7c87d 100644 --- a/src/Microsoft.Management.Deployment/PackageManager.cpp +++ b/src/Microsoft.Management.Deployment/PackageManager.cpp @@ -114,29 +114,11 @@ namespace winrt::Microsoft::Management::Deployment::implementation source.SetCustomHeader(customHeader); } - try - { - auto sourceInfo = source.GetInformation(); + auto sourceInfo = source.GetInformation(); - if (sourceInfo.Authentication.Type == ::AppInstaller::Authentication::AuthenticationType::Unknown) - { - throw winrt::hresult_error(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED); - } - } - catch (const winrt::hresult_error& hre) - { - if (hre.code() == APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED) - { - THROW_HR(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED); - } - else - { - THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED); - } - } - catch (...) // Catch all exceptions + if (sourceInfo.Authentication.Type == ::AppInstaller::Authentication::AuthenticationType::Unknown) { - THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED); + THROW_HR(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED); } return source; @@ -1314,7 +1296,7 @@ namespace winrt::Microsoft::Management::Deployment::implementation LogStartupIfApplicable(); // options must be set. - THROW_HR_IF_NULL(E_INVALIDARG, options); + THROW_HR_IF_NULL(E_POINTER, options); THROW_HR_IF(E_INVALIDARG, options.Name().empty()); THROW_HR_IF(E_INVALIDARG, options.SourceUri().empty()); @@ -1352,7 +1334,7 @@ namespace winrt::Microsoft::Management::Deployment::implementation LogStartupIfApplicable(); // options must be set. - THROW_HR_IF_NULL(E_INVALIDARG, options); + THROW_HR_IF_NULL(E_POINTER, options); THROW_HR_IF(E_INVALIDARG, options.Name().empty()); HRESULT terminationHR = S_OK; @@ -1379,7 +1361,6 @@ namespace winrt::Microsoft::Management::Deployment::implementation if (options.PreserveData()) { THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NAME_DOES_NOT_EXIST, !sourceToRemove.DropSource(matchingSource.value().Name)); - packageCatalogProgressSink->OnProgress(100, 100, AppInstaller::ProgressType::Percent); } else { diff --git a/src/Microsoft.Management.Deployment/PackageManager.idl b/src/Microsoft.Management.Deployment/PackageManager.idl index 302e66fda5..268fde4327 100644 --- a/src/Microsoft.Management.Deployment/PackageManager.idl +++ b/src/Microsoft.Management.Deployment/PackageManager.idl @@ -940,6 +940,11 @@ namespace Microsoft.Management.Deployment [contract(Microsoft.Management.Deployment.WindowsPackageManagerContract, 12)] { /// Updates the package catalog. + /// The progress value, represented as a double, indicates the percentage of operation completion. + /// For "Microsoft.PreIndexed.Package" Package Catalogs, it reflects the progress of downloading and + /// installing the index. When there is no meaningful progress to report for other Package Catalogs, + /// the progress will be set to 0% at the start and will be updated to 100% upon the operation's completion. + /// The progress range is from 0 to 100. Windows.Foundation.IAsyncOperationWithProgress RefreshPackageCatalogAsync(); } } @@ -1495,9 +1500,18 @@ namespace Microsoft.Management.Deployment [contract(Microsoft.Management.Deployment.WindowsPackageManagerContract, 12)] { /// Add a catalog to the Windows Package Catalogs. + /// The progress value, represented as a double, indicates the percentage of operation completion. + /// For "Microsoft.PreIndexed.Package" Package Catalogs, it reflects the progress of downloading and + /// installing the index. When there is no meaningful progress to report for other Package Catalogs, + /// the progress will be set to 0% at the start and will be updated to 100% upon the operation's completion. + /// The progress range is from 0 to 100. Windows.Foundation.IAsyncOperationWithProgress AddPackageCatalogAsync(AddPackageCatalogOptions options); /// Unregisters a Package Catalog from the Windows Package Catalogs and eliminates the system artifacts based on the provided options. + /// The progress value, represented as a double, indicates the percentage of operation completion.For "Microsoft.PreIndexed.Package" + /// Package Catalogs, it reflects the progress of uninstalling the index. When there is no meaningful progress to report for + /// other Package Catalogs, the progress will be set to 0% at the start and will be updated to 100% upon the operation's completion. + /// The progress range is from 0 to 100. Windows.Foundation.IAsyncOperationWithProgress RemovePackageCatalogAsync(RemovePackageCatalogOptions options); } diff --git a/src/Microsoft.Management.Deployment/RemovePackageCatalogOptions.h b/src/Microsoft.Management.Deployment/RemovePackageCatalogOptions.h index ba312a70fa..583043d6f1 100644 --- a/src/Microsoft.Management.Deployment/RemovePackageCatalogOptions.h +++ b/src/Microsoft.Management.Deployment/RemovePackageCatalogOptions.h @@ -19,7 +19,7 @@ namespace winrt::Microsoft::Management::Deployment::implementation #if !defined(INCLUDE_ONLY_INTERFACE_METHODS) private: - std::wstring m_name = L""; + hstring m_name = L""; bool m_preserveData = false; #endif };