feat: updateDependencies uses parsed data not contents

packages/snyk-fix/src/plugins/python/handlers/pip-requirements/extract-version-provenance.ts

@@ -2,14 +2,14 @@ import * as path from 'path';
 import * as debugLib from 'debug';
 
 import {
+  ParsedRequirements,
   parseRequirementsFile,
-  Requirement,
 } from './update-dependencies/requirements-file-parser';
 import { Workspace } from '../../../../types';
 import { containsRequireDirective } from './contains-require-directive';
 
 interface PythonProvenance {
-  [fileName: string]: Requirement[];
+  [fileName: string]: ParsedRequirements;
 }
 
 const debug = debugLib('snyk-fix:python:extract-version-provenance');

packages/snyk-fix/src/plugins/python/handlers/pip-requirements/index.ts

@@ -10,6 +10,8 @@ import { updateDependencies } from './update-dependencies';
 import { MissingRemediationDataError } from '../../../../lib/errors/missing-remediation-data';
 import { MissingFileNameError } from '../../../../lib/errors/missing-file-name';
 import { partitionByFixable } from './is-supported';
+import { NoFixesCouldBeAppliedError } from '../../../../lib/errors/no-fixes-applied';
+import { parseRequirementsFile } from './update-dependencies/requirements-file-parser';
 
 const debug = debugLib('snyk-fix:python:requirements.txt');
 
@@ -52,11 +54,19 @@ export async function fixIndividualRequirementsTxt(
     throw new MissingFileNameError();
   }
   const requirementsTxt = await entity.workspace.readFile(fileName);
+  const requirementsData = parseRequirementsFile(requirementsTxt);
+
   // TODO: allow handlers per fix type (later also strategies or combine with strategies)
   const { updatedManifest, changes } = updateDependencies(
-    requirementsTxt,
+    requirementsData,
     remediationData.pin,
   );
+
+  // TODO: do this with the changes now that we only return new
+  if (updatedManifest === requirementsTxt) {
+    debug('Manifest has not changed!');
+    throw new NoFixesCouldBeAppliedError();
+  }
   if (!options.dryRun) {
     debug('Writing changes to file');
     await entity.workspace.writeFile(fileName, updatedManifest);

packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/index.ts

@@ -1,10 +1,9 @@
 import * as debugLib from 'debug';
 
-import { NoFixesCouldBeAppliedError } from '../../../../../lib/errors/no-fixes-applied';
 import { DependencyPins, FixChangesSummary } from '../../../../../types';
 import { generatePins } from './generate-pins';
 import { applyUpgrades } from './apply-upgrades';
-import { parseRequirementsFile } from './requirements-file-parser';
+import { ParsedRequirements } from './requirements-file-parser';
 import { generateUpgrades } from './generate-upgrades';
 import { FailedToParseManifest } from '../../../../../lib/errors/failed-to-parse-manifest';
 
@@ -18,15 +17,14 @@ const debug = debugLib('snyk-fix:python:update-dependencies');
  * `requirements.txt` must be in the manifests.
  */
 export function updateDependencies(
-  requirementsTxt: string,
+  parsedRequirementsData: ParsedRequirements,
   updates: DependencyPins,
 ): { updatedManifest: string; changes: FixChangesSummary[] } {
   const {
-    requirements: parsedRequirementsData,
+    requirements,
     endsWithNewLine: shouldEndWithNewLine,
-  } = parseRequirementsFile(requirementsTxt);
-
-  if (!parsedRequirementsData.length) {
+  } = parsedRequirementsData;
+  if (!requirements.length) {
     debug(
       'Error: Expected to receive parsed manifest data. Is manifest empty?',
     );
@@ -35,19 +33,19 @@ export function updateDependencies(
   debug('Finished parsing manifest');
 
   const { updatedRequirements, changes: upgradedChanges } = generateUpgrades(
-    parsedRequirementsData,
+    requirements,
     updates,
   );
   debug('Finished generating upgrades to apply');
 
   const { pinnedRequirements, changes: pinChanges } = generatePins(
-    parsedRequirementsData,
+    requirements,
     updates,
   );
   debug('Finished generating pins to apply');
 
   let updatedManifest = [
-    ...applyUpgrades(parsedRequirementsData, updatedRequirements),
+    ...applyUpgrades(requirements, updatedRequirements),
     ...pinnedRequirements,
   ].join('\n');
 
@@ -58,12 +56,6 @@ export function updateDependencies(
   }
   debug('Finished applying changes to manifest');
 
-  // TODO: do this with the changes now that we only return new
-  if (updatedManifest === requirementsTxt) {
-    debug('Manifest has not changed!');
-    throw new NoFixesCouldBeAppliedError();
-  }
-
   return {
     updatedManifest,
     changes: [...pinChanges, ...upgradedChanges],

packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/requirements-file-parser.ts

@@ -19,12 +19,13 @@ export interface Requirement {
  * such as name, version, etc.
  * @param requirementsFile A requirements.txt file as a string
  */
-export function parseRequirementsFile(
-  requirementsFile: string,
-): {
+export interface ParsedRequirements {
   requirements: Requirement[];
   endsWithNewLine: boolean;
-} {
+}
+export function parseRequirementsFile(
+  requirementsFile: string,
+): ParsedRequirements {
   const endsWithNewLine = requirementsFile.endsWith('\n');
   const lines = requirementsFile.replace(/\n$/, '').split('\n');
   const requirements: Requirement[] = [];

packages/snyk-fix/test/unit/plugins/python/handlers/update-dependencies/update-dependencies.spec.ts

@@ -1,6 +1,7 @@
 import { readFileSync } from 'fs';
 import * as path from 'path';
 import { updateDependencies } from '../../../../../../src/plugins/python/handlers/pip-requirements/update-dependencies';
+import { parseRequirementsFile } from '../../../../../../src/plugins/python/handlers/pip-requirements/update-dependencies/requirements-file-parser';
 
 describe('remediation', () => {
   it('does not add extra new lines', () => {
@@ -24,7 +25,8 @@ describe('remediation', () => {
     const expectedManifest =
       'Django==2.0.1\ntransitive>=1.1.1 # not directly required, pinned by Snyk to avoid a vulnerability';
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes.map((c) => c.userMessage).sort()).toEqual(
       [
         'Upgraded Django from 1.6.1 to 2.0.1',
@@ -56,7 +58,8 @@ describe('remediation', () => {
     const expectedManifest =
       'Django==2.0.1\ntransitive>=1.1.1 # not directly required, pinned by Snyk to avoid a vulnerability\n';
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes.map((c) => c.userMessage).sort()).toEqual(
       [
         'Upgraded Django from 1.6.1 to 2.0.1',
@@ -87,7 +90,8 @@ describe('remediation', () => {
     const expectedManifest =
       '\n#some comment\n\nDjango==2.0.1\ntransitive>=1.1.1 # not directly required, pinned by Snyk to avoid a vulnerability\n';
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes.map((c) => c.userMessage).sort()).toEqual(
       [
         'Upgraded Django from 1.6.1 to 2.0.1',
@@ -110,8 +114,8 @@ describe('remediation', () => {
     const manifestContents = 'django==1.6.1\n';
 
     const expectedManifest = 'django==2.0.1\n';
-
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes[0].userMessage).toEqual(
       'Upgraded django from 1.6.1 to 2.0.1',
     );
@@ -132,7 +136,8 @@ describe('remediation', () => {
 
     const expectedManifest = 'Django==2.0.1\n';
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes[0].userMessage).toEqual(
       'Upgraded Django from 1.6.1 to 2.0.1',
     );
@@ -153,7 +158,8 @@ describe('remediation', () => {
 
     const expectedManifest = 'foo==55.66.7\n';
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes[0].userMessage).toEqual(
       'Upgraded foo from 12.123.14 to 55.66.7',
     );
@@ -174,7 +180,8 @@ describe('remediation', () => {
 
     const expectedManifest = 'django==2.0.1 # this is a comment\n';
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes[0].userMessage).toEqual(
       'Upgraded django from 1.6.1 to 2.0.1',
     );
@@ -201,7 +208,8 @@ describe('remediation', () => {
 
     const expectedManifest = 'django>=2.0.1\nclick>7.1';
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes.map((c) => c.userMessage)).toEqual([
       'Upgraded django from 1.6.1 to 2.0.1',
       'Upgraded click from 7.0 to 7.1',
@@ -236,7 +244,8 @@ describe('remediation', () => {
       'utf8',
     );
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes.map((c) => c.userMessage).sort()).toEqual(
       [
         'Upgraded Django from 1.6.1 to 2.0.1',
@@ -267,7 +276,8 @@ describe('remediation', () => {
       'utf8',
     );
 
-    const result = updateDependencies(manifestContents, upgrades);
+    const requirements = parseRequirementsFile(manifestContents);
+    const result = updateDependencies(requirements, upgrades);
     expect(result.changes[0].userMessage).toEqual(
       'Upgraded click from 7.0 to 7.1',
     );
@@ -286,10 +296,14 @@ describe('remediation', () => {
       ),
       'utf8',
     );
+    const requirements = parseRequirementsFile(manifestContents);
+
     try {
-      updateDependencies(manifestContents, upgrades);
+      updateDependencies(requirements, upgrades);
     } catch (e) {
-      expect(e.message).toEqual('No fixes could be applied. Please contact support@snyk.io');
+      expect(e.message).toEqual(
+        'No fixes could be applied. Please contact support@snyk.io',
+      );
     }
   });
 });