From d820026ae2d7eb1fbb7dfede92c5d584f6a430d4 Mon Sep 17 00:00:00 2001 From: ghe Date: Tue, 14 Sep 2021 12:58:01 +0100 Subject: [PATCH] fix: move throwable upgrades calculation back into try/catch Move throwable code into try/catch to make sure we continue fixing + spinner is not stuck Update a test to show that if some dependencies fail under --sequentially-fix we can still update other dependencies. --- .../update-dependencies/index.ts | 12 ++--- .../poetry/update-dependencies/index.ts | 9 ++-- .../poetry/update-dependencies.spec.ts | 51 ++++++++++++++----- .../handlers/poetry/generate-upgrades.spec.ts | 2 +- src/cli/args.ts | 1 + src/cli/commands/fix/index.ts | 4 +- src/lib/types.ts | 1 + 7 files changed, 56 insertions(+), 24 deletions(-) diff --git a/packages/snyk-fix/src/plugins/python/handlers/pipenv-pipfile/update-dependencies/index.ts b/packages/snyk-fix/src/plugins/python/handlers/pipenv-pipfile/update-dependencies/index.ts index a4e3e72d251..5bb037503a1 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/pipenv-pipfile/update-dependencies/index.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/pipenv-pipfile/update-dependencies/index.ts @@ -31,14 +31,14 @@ async function fixAll( failed: [], skipped: [], }; - const { upgrades } = await generateUpgrades(entity); - if (!upgrades.length) { - throw new NoFixesCouldBeAppliedError( - 'Failed to calculate package updates to apply', - ); - } const changes: FixChangesSummary[] = []; try { + const { upgrades } = await generateUpgrades(entity); + if (!upgrades.length) { + throw new NoFixesCouldBeAppliedError( + 'Failed to calculate package updates to apply', + ); + } // TODO: for better support we need to: // 1. parse the manifest and extract original requirements, version spec etc // 2. swap out only the version and retain original spec diff --git a/packages/snyk-fix/src/plugins/python/handlers/poetry/update-dependencies/index.ts b/packages/snyk-fix/src/plugins/python/handlers/poetry/update-dependencies/index.ts index 4a4dd54b08f..717f10454a6 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/poetry/update-dependencies/index.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/poetry/update-dependencies/index.ts @@ -3,12 +3,10 @@ import * as debugLib from 'debug'; import { PluginFixResponse } from '../../../../types'; import { EntityToFix, - FixChangesError, FixChangesSummary, FixOptions, } from '../../../../../types'; import { NoFixesCouldBeAppliedError } from '../../../../../lib/errors/no-fixes-applied'; -import { isSuccessfulChange } from '../../attempted-changes-summary'; import { generateUpgrades } from './generate-upgrades'; import { poetryAdd } from './poetry-add'; import { ensureHasUpdates } from '../../ensure-has-updates'; @@ -95,8 +93,12 @@ async function fixSequentially( // 2. swap out only the version and retain original spec // 3. re-lock the lockfile const changes: FixChangesSummary[] = []; - try { + if (![...upgrades, ...devUpgrades].length) { + throw new NoFixesCouldBeAppliedError( + 'Failed to calculate package updates to apply', + ); + } // update prod dependencies first if (upgrades.length) { for (const upgrade of upgrades) { @@ -113,7 +115,6 @@ async function fixSequentially( ); } } - ensureHasUpdates(changes); handlerResult.succeeded.push({ original: entity, diff --git a/packages/snyk-fix/test/acceptance/plugins/python/handlers/poetry/update-dependencies.spec.ts b/packages/snyk-fix/test/acceptance/plugins/python/handlers/poetry/update-dependencies.spec.ts index f1589f72192..c359fc17fee 100644 --- a/packages/snyk-fix/test/acceptance/plugins/python/handlers/poetry/update-dependencies.spec.ts +++ b/packages/snyk-fix/test/acceptance/plugins/python/handlers/poetry/update-dependencies.spec.ts @@ -623,7 +623,7 @@ describe('fix Poetry Python projects fix sequentially', () => { }); it('error is bubbled up', async () => { - jest.spyOn(poetryFix, 'poetryAdd').mockResolvedValue({ + jest.spyOn(poetryFix, 'poetryAdd').mockResolvedValueOnce({ exitCode: 1, stdout: '', stderr: `Resolving dependencies... (1.7s) @@ -633,7 +633,14 @@ describe('fix Poetry Python projects fix sequentially', () => { Because package-A (2.6) depends on package-B (>=1.19) and package-C (2.2.1) depends on package-B (>=1.16.0,<1.19.0), package-D (2.6) is incompatible with package-C (2.2.1). So, because package-Z depends on both package-C (2.2.1) and package-D (2.6), version solving failed.`, - command: 'poetry install six==2.0.1 transitive==1.1.1', + command: 'poetry install six==2.0.1', + duration: 123, + }); + jest.spyOn(poetryFix, 'poetryAdd').mockResolvedValueOnce({ + exitCode: 0, + stdout: '', + stderr: '', + command: 'poetry install transitive==1.1.1', duration: 123, }); @@ -680,25 +687,45 @@ describe('fix Poetry Python projects fix sequentially', () => { exceptions: {}, results: { python: { - failed: [ + failed: [], + skipped: [], + succeeded: [ { original: entityToFix, - error: expect.objectContaining({ - name: 'NoFixesCouldBeAppliedError', - }), - tip: 'Try running `poetry install six==2.0.1 transitive==1.1.1`', + changes: [ + { + from: 'six@1.1.6', + issueIds: ['VULN-six'], + reason: `SolverProblemError + + Because package-A (2.6) depends on package-B (>=1.19) + and package-C (2.2.1) depends on package-B (>=1.16.0,<1.19.0), package-D (2.6) is incompatible with package-C (2.2.1). + So, because package-Z depends on both package-C (2.2.1) and package-D (2.6), version solving failed`, + success: false, + tip: 'Try running `poetry install six==2.0.1`', + to: 'six@2.0.1', + userMessage: 'Failed to upgrade six from 1.1.6 to 2.0.1', + }, + { + from: 'transitive@1.0.0', + issueIds: [], + success: true, + to: 'transitive@1.1.1', + userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', + }, + ], }, ], - skipped: [], - succeeded: [], }, }, }); - expect(result.fixSummary).toContain('✖ SolverProblemError'); + expect(result.fixSummary).toContain('SolverProblemError'); expect(result.fixSummary).toContain( - 'Tip: Try running `poetry install six==2.0.1 transitive==1.1.1`', + 'Tip: Try running `poetry install six==2.0.1`', + ); + expect(result.fixSummary).toContain( + 'Pinned transitive from 1.0.0 to 1.1.1', ); - expect(result.fixSummary).toContain('✖ No successful fixes'); expect(poetryFixStub).toHaveBeenCalledTimes(2); expect(poetryFixStub).toHaveBeenCalledWith( pathLib.resolve(workspacesPath, 'simple'), diff --git a/packages/snyk-fix/test/unit/plugins/python/handlers/poetry/generate-upgrades.spec.ts b/packages/snyk-fix/test/unit/plugins/python/handlers/poetry/generate-upgrades.spec.ts index 5f994b4396a..7b6062381d6 100644 --- a/packages/snyk-fix/test/unit/plugins/python/handlers/poetry/generate-upgrades.spec.ts +++ b/packages/snyk-fix/test/unit/plugins/python/handlers/poetry/generate-upgrades.spec.ts @@ -1,4 +1,4 @@ -import { generateUpgrades } from '../../../../../../src/plugins/python/handlers/poetry/update-dependencies'; +import { generateUpgrades } from '../../../../../../src/plugins/python/handlers/poetry/update-dependencies/generate-upgrades'; import { generateEntityToFix } from '../../../../../helpers/generate-entity-to-fix'; describe('generateUpgrades', () => { it('returns empty if no upgrades could be generated', async () => { diff --git a/src/cli/args.ts b/src/cli/args.ts index bf942a9c519..a695ddbd9ea 100644 --- a/src/cli/args.ts +++ b/src/cli/args.ts @@ -218,6 +218,7 @@ export function args(rawArgv: string[]): Args { 'integration-version', 'prune-repeated-subdependencies', 'dry-run', + 'update-sequentially', ]; for (const dashedArg of argumentsToTransform) { if (argv[dashedArg]) { diff --git a/src/cli/commands/fix/index.ts b/src/cli/commands/fix/index.ts index 613e6ecfac2..2df4144f12a 100644 --- a/src/cli/commands/fix/index.ts +++ b/src/cli/commands/fix/index.ts @@ -25,6 +25,7 @@ const snykFixFeatureFlag = 'cliSnykFix'; interface FixOptions { dryRun?: boolean; quiet?: boolean; + updateSequentially?: boolean; } export default async function fix(...args: MethodArgs): Promise { const { options: rawOptions, paths } = await processCommandArgs( @@ -44,12 +45,13 @@ export default async function fix(...args: MethodArgs): Promise { const vulnerableResults = results.filter( (res) => Object.keys(res.testResult.issues).length, ); - const { dryRun, quiet } = options; + const { dryRun, quiet, updateSequentially: sequentialFix } = options; const { fixSummary, meta, results: resultsByPlugin } = await snykFix.fix( results, { dryRun, quiet, + sequentialFix, }, ); diff --git a/src/lib/types.ts b/src/lib/types.ts index 976abf711d7..cb235240284 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -163,6 +163,7 @@ export type SupportedUserReachableFacingCliArgs = | 'detection-depth' | 'docker' | 'dry-run' + | 'update-sequentially' | 'fail-on' | 'file' | 'gradle-sub-project'