(#3461,#3487) Prevent dependency resolution from downgrading packages #3486

corbob · 2024-07-16T00:51:03Z

Description Of Changes

Prevent dependency resolution from downgrading packages when --allow-downgrade is not specified.
Prevent packages from installing if a dependent package fails installation.

Motivation and Context

Chocolatey 2.3.0 incorrectly allows a package to be downgraded to resolve a dependency when it has not been specified to allow downgrades.
Chocolatey 2.3.0 installs a package even if one or more of the packages it depends on fails to install resulting in a broken state.

Testing

Run tests through TeamCity/Test Kitchen
Run all integration tests with ./build.bat --testExecutionType=all --shouldRunOpenCover=false
Run a spattering of manual tests.

Operating Systems Testing

Windows Server 2019/2016
Windows 10

Change Types Made

Bug fix (non-breaking change).
Feature / Enhancement (non-breaking change).
Breaking change (fix or feature that could cause existing functionality to change).
Documentation changes.
PowerShell code changes.

Change Checklist

Requires a change to the documentation.
Documentation has been updated.
Tests to cover my changes, have been added.
All new and existing tests passed?
PowerShell code changes: PowerShell v3 compatibility checked?

Related Issue

corbob · 2024-07-16T00:51:53Z

This PR is in draft as I still need to run more tests as well as add more tests. And apparently bring the branch in line with the develop branch.

corbob · 2024-08-15T18:05:36Z

This PR is currently based upon #3500, while this will make a review of it prior to #3500 being merged a little awkward, the fix from #3500 is required for some of the pester tests to complete successfully.

tests/pester-tests/commands/choco-upgrade.Tests.ps1

gep13 · 2024-08-22T14:34:47Z

@corbob I am going to rebase this PR onto the head of develop, since my PR has now been merged, which will make reviewing this PR easier.

gep13

I have left a few comments for review.

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

tests/packages/failingdependency/tools/chocolateyinstall.ps1

gep13 · 2024-08-22T14:48:49Z

tests/packages/upgradedowngradesdependency/Readme.md

@@ -0,0 +1,6 @@
+These packages can be used to test the installation or upgrading of packages that have require an existing package to downgrade.


This sentence doesn't make much sense to me. Can you re-word?

tests/packages/upgradedowngradesdependency/Readme.md

gep13 · 2024-08-22T14:53:13Z

tests/pester-tests/commands/choco-install.Tests.ps1

+        }
+    }
+
+    Context 'Installing a package with argument (<Argument>) should (<AllowsDowngrade>) downgrade an existing package dependency.' -Tag Downgrade, StopOnFirstPackageFailure -ForEach @(


Should this test name mention that it includes usage of the StopOnFirstPackageFailure feature?

It definitely should. I copied it from the other tests, and didn't update the name of it apparently.

Naming is hard, I've tried to encompass what the test is doing...

gep13 · 2024-08-22T15:04:04Z

tests/packages/dependencyfailure/dependencyfailure.nuspec

+    <summary>__REPLACE__</summary>
+    <description>__REPLACE__MarkDown_Okay </description>


Should there be words in here to say what this package is for?

gep13 · 2024-08-22T15:04:29Z

tests/packages/failingdependency/failingdependency.nuspec

+    <summary>__REPLACE__</summary>
+    <description>__REPLACE__MarkDown_Okay </description>


Should there be words here to say what this package is for?

gep13 · 2024-08-22T15:04:44Z

tests/packages/hasfailingnesteddependency/1.0.0/hasfailingnesteddependency.nuspec

+        <description>__REPLACE__</description>
+        <summary>__REPLACE__</summary>


Same question again.

gep13 · 2024-08-22T15:04:57Z

tests/packages/hasnesteddependency/1.0.0/hasnesteddependency.nuspec

+        <description>__REPLACE__</description>
+        <summary>__REPLACE__</summary>


corbob · 2024-08-26T16:27:08Z

@gep13 I'll add it here instead of in the conversations... The var x and var nullResult I think were taken from my re-use of this code that is currently line 1187 in the PR:

    var nullResult = packageResultsToReturn.GetOrAdd(packageName, new PackageResult(installedPackage.PackageMetadata, pathResolver.GetInstallPath(installedPackage.PackageMetadata.Id)));
    nullResult.Messages.Add(new ResultMessage(ResultType.Error, logMessage));

I think, I did the var x as while debugging I wanted to investiage the object to see if there was anything I could do with it in lieu of what I was.

The var nullResult was a direct copy of the existing code, but as discussed earlier today, it is better to be clear about what the object is for, so I'll either come up with a better name, add a comment, or perhaps not even use a variable...

When a package dependency fails to install, we will now fail the package as well.

Some of these package already existed on our internal repository, while some of them have been added for the tests being added.

We have added some tests for the install all command. This adds a new package source to the testing environment so that we can run the tests locally and in Test Kitchen.

Add a number of tests for dependency scenarios where we did not have tests before.

In the NugetService we were using a nullResult variable to allow us to add messages to the result. We do not need this variable if we're only adding a single message. This commit removes the unnecessary variables.

…ncies

…tests

This updates the `test-chocolateypath` package to have a description and removes the unneeded comments. This is related to a GitLab MR that noted these things when bringing this package into the internal repository. This reverts commit ffd9aab4b40dc8b47b84de5f3ce91ea22fda7a6d.

AdmiringWorm · 2024-09-19T10:36:52Z

tests/packages/hasnesteddependency/1.0.0/hasnesteddependency.nuspec

+        <description>__REPLACE__</description>
+        <summary>__REPLACE__</summary>


Looks like this was missed to be updated with information about what this package is about.

Can we get the description and summary updated to say what the package is for?

AdmiringWorm · 2024-09-19T10:39:07Z

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

+                        packageResultsToReturn
+                            .GetOrAdd(
+                                packageDependencyInfo.Id,
+                                new PackageResult(packageDependencyInfo.Id, packageDependencyInfo.Version.ToStringSafe(), string.Empty)


Any reason you are using ToStringSafe instead of ToNormalizedVersionChecked here?

Or is that method not available here?

AdmiringWorm · 2024-09-19T10:40:29Z

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

@@ -793,6 +800,26 @@ Version was specified as '{0}'. It is possible that version

                foreach (SourcePackageDependencyInfo packageDependencyInfo in resolvedPackages)
                {
+                    // Don't attempt to action this package if dependencies failed.
+                    if (packageDependencyInfo != null && packageResultsToReturn.Any(r => r.Value.Success != true && packageDependencyInfo.Dependencies.Any(d => d.Id == r.Value.Identity.Id)))


Can we make this dependency any check be case-insensitive?

Just using == here makes it case-sensitive.

AdmiringWorm · 2024-09-19T10:42:18Z

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

@@ -1559,6 +1608,25 @@ public virtual ConcurrentDictionary<string, PackageResult> Upgrade(ChocolateyCon
                                break;
                            }

+                            // Don't attempt to action this package if dependencies failed.
+                            if (packageResultsToReturn.Any(r => r.Value.Success != true && packageDependencyInfo.Dependencies.Any(d => d.Id == r.Value.Identity.Id)))


Same as earlier, can we make this dependency any check case-insensitive?

AdmiringWorm · 2024-09-19T10:42:46Z

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

+                                var logMessage = StringResources.ErrorMessages.DependencyFailedToInstall.FormatWith(packageDependencyInfo.Id, packageDependencyInfo.Version);
+                                packageResultsToReturn.GetOrAdd(
+                                        packageDependencyInfo.Id, 
+                                        new PackageResult(packageDependencyInfo.Id, packageDependencyInfo.Version.ToStringSafe(), string.Empty)


Can we use ToNormalizedVersionChecked?

corbob force-pushed the 3461-downgrades branch from 521b26b to c3c71b6 Compare July 18, 2024 19:56

corbob force-pushed the 3461-downgrades branch 2 times, most recently from 64f2ded to 1731d3e Compare August 12, 2024 23:41

corbob force-pushed the 3461-downgrades branch 2 times, most recently from 37574fb to 7e225aa Compare August 15, 2024 17:47

corbob changed the title ~~(#3461) Prevent dependency resolution from downgrading packages~~ (#3461,#3487) Prevent dependency resolution from downgrading packages Aug 15, 2024

corbob commented Aug 15, 2024

View reviewed changes

tests/pester-tests/commands/choco-upgrade.Tests.ps1 Outdated Show resolved Hide resolved

corbob force-pushed the 3461-downgrades branch 2 times, most recently from a4cf193 to 8b82e88 Compare August 15, 2024 20:53

gep13 self-requested a review August 16, 2024 15:24

gep13 force-pushed the 3461-downgrades branch from 8b82e88 to 695754d Compare August 22, 2024 14:38

gep13 reviewed Aug 22, 2024

View reviewed changes

gep13 requested changes Aug 26, 2024

View reviewed changes

corbob added 5 commits August 26, 2024 09:55

(maint) Convert common string to constant

7dcd1cd

(chocolatey#3461) Prevent downgrade of dependency

6383e0b

(chocolatey#3487) Prevent package success on dependency fail

d4e876a

When a package dependency fails to install, we will now fail the package as well.

(chocolatey#3461) Add downgrade install pester test

15553ee

(chocolatey#3461,chocolatey#3487) Add test packages for added tests

a2caa3f

Some of these package already existed on our internal repository, while some of them have been added for the tests being added.

corbob force-pushed the 3461-downgrades branch from 695754d to 950ce5d Compare August 26, 2024 16:57

corbob marked this pull request as ready for review August 27, 2024 14:49

corbob added 6 commits August 30, 2024 15:02

(chocolatey#3461,chocolatey#3487) Add an all packages source for tests

1c15990

We have added some tests for the install all command. This adds a new package source to the testing environment so that we can run the tests locally and in Test Kitchen.

(chocolatey#3461,chocolatey#3487) Add pester tests for dependencies

a4d128c

Add a number of tests for dependency scenarios where we did not have tests before.

(maint) Remove unnecessary variable usage

8cc4087

In the NugetService we were using a nullResult variable to allow us to add messages to the result. We do not need this variable if we're only adding a single message. This commit removes the unnecessary variables.

fixup! (chocolatey#3461,chocolatey#3487) Add pester tests for depende…

814d5de

…ncies

fixup! (chocolatey#3461,chocolatey#3487) Add test packages for added …

3a93ad4

…tests

corbob force-pushed the 3461-downgrades branch from e1a6373 to 8841937 Compare August 30, 2024 22:33

AdmiringWorm requested changes Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#3461,#3487) Prevent dependency resolution from downgrading packages #3486

(#3461,#3487) Prevent dependency resolution from downgrading packages #3486

corbob commented Jul 16, 2024 •

edited

Loading

corbob commented Jul 16, 2024

corbob commented Aug 15, 2024

gep13 commented Aug 22, 2024

gep13 left a comment

gep13 Aug 22, 2024

gep13 Aug 22, 2024

corbob Aug 26, 2024 •

edited

Loading

gep13 Aug 22, 2024

gep13 Aug 22, 2024

gep13 Aug 22, 2024

gep13 Aug 22, 2024

corbob commented Aug 26, 2024

AdmiringWorm Sep 19, 2024

AdmiringWorm Sep 19, 2024

AdmiringWorm Sep 19, 2024

AdmiringWorm Sep 19, 2024

AdmiringWorm Sep 19, 2024

		@@ -0,0 +1,6 @@
		These packages can be used to test the installation or upgrading of packages that have require an existing package to downgrade.

		<summary>__REPLACE__</summary>
		<description>__REPLACE__MarkDown_Okay </description>

		<description>__REPLACE__</description>
		<summary>__REPLACE__</summary>

(#3461,#3487) Prevent dependency resolution from downgrading packages #3486

Are you sure you want to change the base?

(#3461,#3487) Prevent dependency resolution from downgrading packages #3486

Conversation

corbob commented Jul 16, 2024 • edited Loading

Description Of Changes

Motivation and Context

Testing

Operating Systems Testing

Change Types Made

Change Checklist

Related Issue

corbob commented Jul 16, 2024

corbob commented Aug 15, 2024

gep13 commented Aug 22, 2024

gep13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corbob Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corbob commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corbob commented Jul 16, 2024 •

edited

Loading

corbob Aug 26, 2024 •

edited

Loading