NugetService: Upgrade and Install methods are coupled in confusing and error-prone ways

[vexx32]

What You Are Seeing?

This was noticed while I was looking at #2884.

The code paths involved here are in NugetService, where both upgrade_run and install_run do some of the same work:

choco/src/chocolatey/infrastructure.app/services/NugetService.cs

Lines 600 to 622 in 0466676

	public virtual ConcurrentDictionary<string, PackageResult> upgrade_run(ChocolateyConfiguration config, Action<PackageResult> continueAction, bool performAction, Action<PackageResult> beforeUpgradeAction = null)
	{
	_fileSystem.create_directory_if_not_exists(ApplicationParameters.PackagesLocation);
	var packageInstalls = new ConcurrentDictionary<string, PackageResult>(StringComparer.InvariantCultureIgnoreCase);
	SemanticVersion version = !string.IsNullOrWhiteSpace(config.Version) ? new SemanticVersion(config.Version) : null;
	if (config.Force) config.AllowDowngrade = true;
	var packageManager = NugetCommon.GetPackageManager(
	config,
	_nugetLogger,
	_packageDownloader,
	installSuccessAction: (e) =>
	{
	var pkg = e.Package;
	var packageResult = packageInstalls.GetOrAdd(pkg.Id.to_lower(), new PackageResult(pkg, e.InstallPath));
	packageResult.InstallLocation = e.InstallPath;
	packageResult.Messages.Add(new ResultMessage(ResultType.Debug, ApplicationParameters.Messages.ContinueChocolateyAction));
	if (continueAction != null) continueAction.Invoke(packageResult);
	},
	uninstallSuccessAction: null,
	addUninstallHandler: false);

choco/src/chocolatey/infrastructure.app/services/NugetService.cs

Lines 378 to 436 in 0466676

	public virtual ConcurrentDictionary<string, PackageResult> install_run(ChocolateyConfiguration config, Action<PackageResult> continueAction)
	{
	_fileSystem.create_directory_if_not_exists(ApplicationParameters.PackagesLocation);
	var packageInstalls = new ConcurrentDictionary<string, PackageResult>(StringComparer.InvariantCultureIgnoreCase);
	//todo: #23 handle all
	SemanticVersion version = !string.IsNullOrWhiteSpace(config.Version) ? new SemanticVersion(config.Version) : null;
	if (config.Force) config.AllowDowngrade = true;
	IList<string> packageNames = config.PackageNames.Split(new[] { ApplicationParameters.PackageNamesSeparator }, StringSplitOptions.RemoveEmptyEntries).or_empty_list_if_null().ToList();
	if (packageNames.Count == 1)
	{
	var packageName = packageNames.DefaultIfEmpty(string.Empty).FirstOrDefault();
	if (packageName.EndsWith(Constants.PackageExtension) || packageName.EndsWith(Constants.ManifestExtension))
	{
	this.Log().Debug("Updating source and package name to handle *.nupkg or *.nuspec file.");
	packageNames.Clear();
	config.Sources = _fileSystem.get_directory_name(_fileSystem.get_full_path(packageName));
	if (packageName.EndsWith(Constants.ManifestExtension))
	{
	packageNames.Add(_fileSystem.get_file_name_without_extension(packageName));
	this.Log().Debug("Building nuspec file prior to install.");
	config.Input = packageName;
	// build package
	pack_run(config);
	}
	else
	{
	var packageFile = new OptimizedZipPackage(_fileSystem.get_full_path(packageName));
	version = packageFile.Version;
	packageNames.Add(packageFile.Id);
	}
	}
	}
	// this is when someone points the source directly at a nupkg
	// e.g. -source c:\somelocation\somewhere\packagename.nupkg
	if (config.Sources.to_string().EndsWith(Constants.PackageExtension))
	{
	config.Sources = _fileSystem.get_directory_name(_fileSystem.get_full_path(config.Sources));
	}
	var packageManager = NugetCommon.GetPackageManager(
	config, _nugetLogger, _packageDownloader,
	installSuccessAction: (e) =>
	{
	var pkg = e.Package;
	var packageResult = packageInstalls.GetOrAdd(pkg.Id.to_lower(), new PackageResult(pkg, e.InstallPath));
	packageResult.InstallLocation = e.InstallPath;
	packageResult.Messages.Add(new ResultMessage(ResultType.Debug, ApplicationParameters.Messages.ContinueChocolateyAction));
	if (continueAction != null) continueAction.Invoke(packageResult);
	},
	uninstallSuccessAction: null,
	addUninstallHandler: true);

There is some code here that is duplicated, and this is kind of weird and potentially problematic in that it very tightly couples upgrade_run to install_run while also doing a lot of the same things before calling into install_run for every package being upgraded. This unintuitive coupling in part caused #2884.

Interestingly, it looks as though this might result in unintended consequences, such as the code handling for passing in a nuspec or nupkg file path to choco install only being intended to work for a single file at a time, whereas upgrade is calling this code path for each package being upgraded, so (I haven't tested, but) it's possible that choco upgrade might unintentionally accept and work with multiple paths to nupkg or nuspec files despite choco install apparently being designed to handle just one.

What is Expected?

These code paths should be less entangled, and should ideally avoid double work such as having to call GetPackageManager() multiple times (when calling choco upgrade, this method is called n+1 times in this code path — n being the number of packages being upgraded).

I think it should also be possible to take much of the code from the foreach loop in install_run (linked/embedded below) and move it to a separate method that both install_run and upgrade_run can call into, to avoid upgrade_run needing to directly call install_run and incur the double-work here.

choco/src/chocolatey/infrastructure.app/services/NugetService.cs

Lines 442 to 554 in 0466676

	// We need to ensure we are using a clean configuration file
	// before we start reading it.
	config.reset_config();
	//todo: #2577 get smarter about realizing multiple versions have been installed before and allowing that
	IPackage installedPackage = packageManager.LocalRepository.FindPackage(packageName);
	if (Platform.get_platform() != PlatformType.Windows && !packageName.EndsWith(".template"))
	{
	string logMessage = "{0} is not a supported package on non-Windows systems.{1}Only template packages are currently supported.".format_with(packageName, Environment.NewLine);
	this.Log().Warn(ChocolateyLoggers.Important, logMessage);
	}
	if (installedPackage != null && (version == null || version == installedPackage.Version) && !config.Force)
	{
	string logMessage = "{0} v{1} already installed.{2} Use --force to reinstall, specify a version to install, or try upgrade.".format_with(installedPackage.Id, installedPackage.Version, Environment.NewLine);
	var nullResult = packageInstalls.GetOrAdd(packageName, new PackageResult(installedPackage, _fileSystem.combine_paths(ApplicationParameters.PackagesLocation, installedPackage.Id)));
	nullResult.Messages.Add(new ResultMessage(ResultType.Warn, logMessage));
	nullResult.Messages.Add(new ResultMessage(ResultType.Inconclusive, logMessage));
	this.Log().Warn(ChocolateyLoggers.Important, logMessage);
	continue;
	}
	if (installedPackage != null && (version == null || version == installedPackage.Version) && config.Force)
	{
	this.Log().Warn(ChocolateyLoggers.Important, () => @"{0} v{1} already installed. Forcing reinstall of version '{1}'.
	Please use upgrade if you meant to upgrade to a new version.".format_with(installedPackage.Id, installedPackage.Version));
	version = installedPackage.Version;
	}
	if (installedPackage != null && version != null && version < installedPackage.Version && !config.AllowMultipleVersions && !config.AllowDowngrade)
	{
	string logMessage = "A newer version of {0} (v{1}) is already installed.{2} Use --allow-downgrade or --force to attempt to install older versions, or use --side-by-side to allow multiple versions.".format_with(installedPackage.Id, installedPackage.Version, Environment.NewLine);
	var nullResult = packageInstalls.GetOrAdd(packageName, new PackageResult(installedPackage, _fileSystem.combine_paths(ApplicationParameters.PackagesLocation, installedPackage.Id)));
	nullResult.Messages.Add(new ResultMessage(ResultType.Error, logMessage));
	this.Log().Error(ChocolateyLoggers.Important, logMessage);
	continue;
	}
	IPackage availablePackage = NugetList.find_package(packageName, version, config, packageManager.SourceRepository);
	if (availablePackage == null)
	{
	var logMessage = @"{0} not installed. The package was not found with the source(s) listed.
	Source(s): '{1}'
	NOTE: When you specify explicit sources, it overrides default sources.
	If the package version is a prerelease and you didn't specify `--pre`,
	the package may not be found.{2}{3}".format_with(packageName, config.Sources, string.IsNullOrWhiteSpace(config.Version) ? String.Empty :
	@"
	Version was specified as '{0}'. It is possible that version
	does not exist for '{1}' at the source specified.".format_with(config.Version.to_string(), packageName),
	@"
	Please see https://docs.chocolatey.org/en-us/troubleshooting for more
	assistance.");
	this.Log().Error(ChocolateyLoggers.Important, logMessage);
	var noPkgResult = packageInstalls.GetOrAdd(packageName, new PackageResult(packageName, version.to_string(), null));
	noPkgResult.Messages.Add(new ResultMessage(ResultType.Error, logMessage));
	continue;
	}
	if (installedPackage != null && (installedPackage.Version == availablePackage.Version) && config.Force)
	{
	var forcedResult = packageInstalls.GetOrAdd(packageName, new PackageResult(availablePackage, _fileSystem.combine_paths(ApplicationParameters.PackagesLocation, availablePackage.Id)));
	forcedResult.Messages.Add(new ResultMessage(ResultType.Note, "Backing up and removing old version"));
	remove_rollback_directory_if_exists(packageName);
	backup_existing_version(config, installedPackage, _packageInfoService.get_package_information(installedPackage));
	try
	{
	packageManager.UninstallPackage(installedPackage, forceRemove: config.Force, removeDependencies: config.ForceDependencies);
	if (!forcedResult.InstallLocation.is_equal_to(ApplicationParameters.PackagesLocation))
	{
	_fileSystem.delete_directory_if_exists(forcedResult.InstallLocation, recursive: true);
	}
	remove_cache_for_package(config, installedPackage);
	}
	catch (Exception ex)
	{
	string logMessage = "{0}:{1} {2}".format_with("Unable to remove existing package prior to forced reinstall", Environment.NewLine, ex.Message);
	this.Log().Warn(logMessage);
	forcedResult.Messages.Add(new ResultMessage(ResultType.Inconclusive, logMessage));
	}
	}
	try
	{
	using (packageManager.SourceRepository.StartOperation(
	RepositoryOperationNames.Install,
	packageName,
	version == null ? null : version.ToString()))
	{
	packageManager.InstallPackage(availablePackage, ignoreDependencies: config.IgnoreDependencies, allowPrereleaseVersions: config.Prerelease);
	//packageManager.InstallPackage(packageName, version, configuration.IgnoreDependencies, configuration.Prerelease);
	remove_nuget_cache_for_package(availablePackage);
	}
	}
	catch (Exception ex)
	{
	var message = ex.Message;
	var webException = ex as System.Net.WebException;
	if (webException != null)
	{
	var response = webException.Response as HttpWebResponse;
	if (response != null && !string.IsNullOrWhiteSpace(response.StatusDescription)) message += " {0}".format_with(response.StatusDescription);
	}
	var logMessage = "{0} not installed. An error occurred during installation:{1} {2}".format_with(packageName, Environment.NewLine, message);
	this.Log().Error(ChocolateyLoggers.Important, logMessage);
	var errorResult = packageInstalls.GetOrAdd(packageName, new PackageResult(packageName, version.to_string(), null));
	errorResult.Messages.Add(new ResultMessage(ResultType.Error, logMessage));
	if (errorResult.ExitCode == 0) errorResult.ExitCode = 1;
	if (continueAction != null) continueAction.Invoke(errorResult);
	}

We should also evaluate:

1. Does the logic handling for nuspec/nupkg files in install_run needs to be properly and explicitly shared to upgrade_run?
2. Does the current implementation have unintended possible uses via choco upgrade and if so do we want:
   1. To allow the usage of multiple nuspec/nupkg file paths for both choco install and choco upgrade, or
   2. To prevent the possibility of using choco upgrade in this way.

Splitting out some of this duplicated logic into shared methods should also help us reduce the cognitive weight of these very large methods and make this area of the code easier to maintain in future.

How Did You Get This To Happen? (Steps to Reproduce)

N/A, found while investigating #2884

System Details

N/A

Output Log

N/A

[TheCakeIsNaOH]

I completely agree that this area needs work.
This area had quite a few major changes with #2740, although the overall issue of coupling remains.

Splitting out some of this duplicated logic into shared methods should also help us reduce the cognitive weight of these very large methods and make this area of the code easier to maintain in future.

Related/dupe of #2575