chocolatey / chocolatey/choco

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

Open
#2,914 1 comment 0 reactions 0 assignees View on GitHub
0 - Triaging WorkItem
Dominant language
C#
Stars
11.5k
Forks
960
PR merge metrics
No merged PRs in 30d

Description

### 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:

https://github.com/chocolatey/choco/blob/04666760efb08385b59199992fbe007b9bfb36d1/src/chocolatey/infrastructure.app/services/NugetService.cs#L600-L622

https://github.com/chocolatey/choco/blob/04666760efb08385b59199992fbe007b9bfb36d1/src/chocolatey/infrastructure.app/services/NugetService.cs#L378-L436

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.

https://github.com/chocolatey/choco/blob/04666760efb08385b59199992fbe007b9bfb36d1/src/chocolatey/infrastructure.app/services/NugetService.cs#L442-L554

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

Contributor guide

Open the contributing guide

Research direction

Start in src/chocolatey/infrastructure.app/services/NugetService.cs, comparing upgrade_run around lines 600-622 with install_run around lines 378-436 and its foreach logic around lines 442-554. Trace GetPackageManager() and nuspec/nupkg handling, then define the shared behavior and whether multiple file paths should be accepted; done means the paths are less coupled without changing the intended install or upgrade behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
cli
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.