Skip to content

Commit

Permalink
Refactor argument validation in AddPackageCatalogAsync & RemovePackag…
Browse files Browse the repository at this point in the history
…eCatalogAsync to throw errors on invalid arguments, following API design guidelines.

- Updated `AddPackageCatalogAsync` and `RemovePackageCatalogAsync`
in `PackageManager.cpp` to perform argument validation at the beginning of the methods, ensuring early error detection and potential performance improvements. At this stage, we avoid constructing a result object for error codes and simply use a THROW statement to generate an exception for the caller.

- Refactored `AddPackageCatalogWithInvalidOptions` and `RemovePackageCatalogWithInvalidOptions` in
`PackageCatalogInterop.cs` to be synchronous and directly test for exceptions using `Assert.ThrowsAsync<ArgumentException>`.
  • Loading branch information
Madhusudhan-MSFT committed Oct 23, 2024
1 parent f92b947 commit 10f92be
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 17 deletions.
14 changes: 6 additions & 8 deletions src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,14 @@ public async Task AddRemovePackageCatalog()
/// <summary>
/// Add package catalog with invalid options.
/// </summary>
/// <returns>representing the asynchronous unit test.</returns>
[Test]
public async Task AddPackageCatalogWithInvalidOptions()
public void AddPackageCatalogWithInvalidOptions()
{
// Add package catalog with null options.
await this.AddAndValidatePackageCatalogAsync(null, AddPackageCatalogStatus.InvalidOptions, Constants.ErrorCode.E_INVALIDARG);
Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.AddPackageCatalogAsync(null));

// Add package catalog with empty options.
await this.AddAndValidatePackageCatalogAsync(this.TestFactory.CreateAddPackageCatalogOptions(), AddPackageCatalogStatus.InvalidOptions, Constants.ErrorCode.E_INVALIDARG);
Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.AddPackageCatalogAsync(this.TestFactory.CreateAddPackageCatalogOptions()));
}

/// <summary>
Expand Down Expand Up @@ -232,15 +231,14 @@ public async Task AddRemovePackageCatalogWithPreserveDataFiledSet()
/// <summary>
/// Remove package catalog with invalid options.
/// </summary>
/// <returns> representing the asynchronous unit test.</returns>
[Test]
public async Task RemovePackageCatalogWithInvalidOptions()
public void RemovePackageCatalogWithInvalidOptions()
{
// Remove package catalog with null options.
await this.RemoveAndValidatePackageCatalogAsync(null, RemovePackageCatalogStatus.InvalidOptions, Constants.ErrorCode.E_INVALIDARG);
Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.RemovePackageCatalogAsync(null));

// Remove package catalog with empty options.
await this.RemoveAndValidatePackageCatalogAsync(this.TestFactory.CreateRemovePackageCatalogOptions(), RemovePackageCatalogStatus.InvalidOptions, Constants.ErrorCode.E_INVALIDARG);
Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.RemovePackageCatalogAsync(this.TestFactory.CreateRemovePackageCatalogOptions()));
}

/// <summary>
Expand Down
18 changes: 9 additions & 9 deletions src/Microsoft.Management.Deployment/PackageManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1312,14 +1312,14 @@ namespace winrt::Microsoft::Management::Deployment::implementation
{
LogStartupIfApplicable();

// options must be set.
THROW_HR_IF_NULL(E_INVALIDARG, options);
THROW_HR_IF(E_INVALIDARG, options.Name().empty());
THROW_HR_IF(E_INVALIDARG, options.SourceUri().empty());

HRESULT terminationHR = S_OK;
try {

// options must be set.
THROW_HR_IF_NULL(E_INVALIDARG, options);
THROW_HR_IF(E_INVALIDARG, options.Name().empty());
THROW_HR_IF(E_INVALIDARG, options.SourceUri().empty());

// Check if running as admin/system.
// [NOTE:] For OutOfProc calls, the Windows Package Manager Service executes in the context initiated by the caller process,
// so the same admin/system validation check is applicable for both InProc and OutOfProc calls.
Expand Down Expand Up @@ -1355,13 +1355,13 @@ namespace winrt::Microsoft::Management::Deployment::implementation
{
LogStartupIfApplicable();

// options must be set.
THROW_HR_IF_NULL(E_INVALIDARG, options);
THROW_HR_IF(E_INVALIDARG, options.Name().empty());

HRESULT terminationHR = S_OK;
try {

// options must be set.
THROW_HR_IF_NULL(E_INVALIDARG, options);
THROW_HR_IF(E_INVALIDARG, options.Name().empty());

// Check if running as admin/system.
// [NOTE:] For OutOfProc calls, the Windows Package Manager Service executes in the context initiated by the caller process,
// so the same admin/system validation check is applicable for both InProc and OutOfProc calls.
Expand Down

0 comments on commit 10f92be

Please sign in to comment.