Refactored integrity generation and check (#2818)

* proper integrity check: first commit

* moved the rest code to integrity-check and integrated with install and check commands

* more fixes

* removed todo

* removed hashes from integrity error message

* typos fixes

* replaced todo with a proper comment

* added a test for bailout

* changed integrity hash location logic a bit

Author: bestander

__tests__/commands/install/integration.js

@@ -748,6 +748,27 @@ test.concurrent('install should write and read integrity file based on lockfile
   });
 });
 
+test.concurrent('install should not continue if integrity check passes', (): Promise<void> => {
+  return runInstall({}, 'lockfile-stability', async (config, reporter) => {
+
+    await fs.writeFile(path.join(config.cwd, 'node_modules', 'yarn.test'), 'YARN TEST');
+
+    // install should bail out with integrity check and not remove extraneous file
+    let reinstall = new Install({}, config, reporter, await Lockfile.fromDirectory(config.cwd));
+    await reinstall.init();
+
+    expect(await fs.exists(path.join(config.cwd, 'node_modules', 'yarn.test')));
+
+    await fs.unlink(path.join(config.cwd, 'node_modules', 'yarn.test'));
+
+    reinstall = new Install({}, config, reporter, await Lockfile.fromDirectory(config.cwd));
+    await reinstall.init();
+
+    expect(!await fs.exists(path.join(config.cwd, 'node_modules', 'yarn.test')));
+
+  });
+});
+
 test.concurrent('install should not rewrite lockfile with no substantial changes', (): Promise<void> => {
   const fixture = 'lockfile-no-rewrites';
 

__tests__/commands/install/unit.js

@@ -10,24 +10,6 @@ const path = require('path');
 
 const fixturesLoc = path.join(__dirname, '..', '..', 'fixtures', 'install');
 
-test.concurrent('integrity hash respects flat and production flags', async () => {
-  const cwd = path.join(fixturesLoc, 'noop');
-  const reporter = new NoopReporter();
-  const config = await Config.create({cwd}, reporter);
-
-  const lockfile = new Lockfile();
-
-  const install = new Install({}, config, reporter, lockfile);
-
-  const install2 = new Install({flat: true}, config, reporter, lockfile);
-  expect(install2.generateIntegrityHash('foo', [])).not.toEqual(install.generateIntegrityHash('foo', []));
-
-  const config2 = await Config.create({cwd, production: true}, reporter);
-  const install3 = new Install({}, config2, reporter, lockfile);
-  expect(install3.generateIntegrityHash('foo', [])).not.toEqual(install.generateIntegrityHash('foo', []));
-  expect(install3.generateIntegrityHash('foo', [])).not.toEqual(install2.generateIntegrityHash('foo', []));
-});
-
 test.concurrent('flat arg is inherited from root manifest', async (): Promise<void> => {
   const cwd = path.join(fixturesLoc, 'top-level-flat-parameter');
   const reporter = new NoopReporter();

src/cli/commands/check.js

@@ -1,11 +1,13 @@
 /* @flow */
 
-import type {Reporter} from '../../reporters/index.js';
 import type Config from '../../config.js';
 import {MessageError} from '../../errors.js';
-import {Install} from './install.js';
+import InstallationIntegrityChecker from '../../integrity-checker.js';
+import lockStringify from '../../lockfile/stringify.js';
 import Lockfile from '../../lockfile/wrapper.js';
+import type {Reporter} from '../../reporters/index.js';
 import * as fs from '../../util/fs.js';
+import {Install} from './install.js';
 
 const semver = require('semver');
 const path = require('path');
@@ -142,29 +144,26 @@ async function integrityHashCheck(
     reporter.error(reporter.lang(msg, ...vars));
     errCount++;
   }
+  const integrityChecker = new InstallationIntegrityChecker(config);
 
   const lockfile = await Lockfile.fromDirectory(config.cwd);
   const install = new Install(flags, config, reporter, lockfile);
 
   // get patterns that are installed when running `yarn install`
+  // TODO hydrate takes longer then install command bailout https://github.com/yarnpkg/yarn/issues/2514
   const {patterns: rawPatterns} = await install.hydrate(true);
   const patterns = await install.flatten(rawPatterns);
+  const lockSource = lockStringify(lockfile.getLockfile(install.resolver.patterns));
 
-  // check if patterns exist in lockfile
-  for (const pattern of patterns) {
-    if (!lockfile.getLocked(pattern)) {
-      reportError('lockfileNotContainPatter', pattern);
-    }
+  const match = await integrityChecker.check(patterns, lockfile, lockSource, flags);
+  for (const pattern of match.missingPatterns) {
+    reportError('lockfileNotContainPattern', pattern);
   }
-
-  const integrityLoc = await install.getIntegrityHashLocation();
-  if (integrityLoc && await fs.exists(integrityLoc)) {
-    const match = await install.matchesIntegrityHash(patterns);
-    if (match.matches === false) {
-      reportError('integrityHashesDontMatch', match.expected, match.actual);
-    }
-  } else {
-    reportError('noIntegirtyHashFile');
+  if (match.integrityFileMissing) {
+    reportError('noIntegrityHashFile');
+  }
+  if (!match.integrityHashMatches) {
+    reportError('integrityHashesDontMatch');
   }
 
   if (errCount > 0) {
@@ -219,7 +218,7 @@ export async function run(
   // check if patterns exist in lockfile
   for (const pattern of patterns) {
     if (!lockfile.getLocked(pattern)) {
-      reportError('lockfileNotContainPatter', pattern);
+      reportError('lockfileNotContainPattern', pattern);
     }
   }
 

src/cli/commands/install.js

@@ -6,8 +6,8 @@ import type {Manifest, DependencyRequestPatterns} from '../../types.js';
 import type Config from '../../config.js';
 import type {RegistryNames} from '../../registries/index.js';
 import normalizeManifest from '../../util/normalize-manifest/index.js';
-import {registryNames} from '../../registries/index.js';
 import {MessageError} from '../../errors.js';
+import InstallationIntegrityChecker from '../../integrity-checker.js';
 import Lockfile from '../../lockfile/wrapper.js';
 import lockStringify from '../../lockfile/stringify.js';
 import PackageFetcher from '../../package-fetcher.js';
@@ -20,9 +20,7 @@ import {registries} from '../../registries/index.js';
 import {clean} from './clean.js';
 import * as constants from '../../constants.js';
 import * as fs from '../../util/fs.js';
-import * as crypto from '../../util/crypto.js';
 import map from '../../util/map.js';
-import {sortAlpha} from '../../util/misc.js';
 
 const invariant = require('invariant');
 const semver = require('semver');
@@ -41,13 +39,6 @@ export type InstallCwdRequest = {
   manifest: Object,
 };
 
-export type IntegrityMatch = {
-  actual: string,
-  expected: string,
-  loc: string,
-  matches: boolean,
-};
-
 type Flags = {
   // install
   har: boolean,
@@ -179,6 +170,7 @@ export class Install {
 
     this.resolver = new PackageResolver(config, lockfile);
     this.fetcher = new PackageFetcher(config, this.resolver);
+    this.integrityChecker = new InstallationIntegrityChecker(config);
     this.compatibility = new PackageCompatibility(config, this.resolver, this.flags.ignoreEngines);
     this.linker = new PackageLinker(config, this.resolver);
     this.scripts = new PackageInstallScripts(config, this.resolver, this.flags.force);
@@ -197,6 +189,7 @@ export class Install {
   compatibility: PackageCompatibility;
   fetcher: PackageFetcher;
   rootPatternsToOrigin: { [pattern: string]: string };
+  integrityChecker: InstallationIntegrityChecker;
 
   /**
    * Create a list of dependency requests from the current directories manifests.
@@ -301,22 +294,23 @@ export class Install {
   async bailout(
     patterns: Array<string>,
   ): Promise<boolean> {
-    if (this.flags.frozenLockfile && !this.lockFileInSync(patterns)) {
-      throw new MessageError(this.reporter.lang('frozenLockfileError'));
-    }
     if (this.flags.skipIntegrityCheck || this.flags.force) {
       return false;
     }
+    const lockSource = lockStringify(this.lockfile.getLockfile(this.resolver.patterns));
+    const match = await this.integrityChecker.check(patterns, this.lockfile, lockSource, this.flags);
+    if (this.flags.frozenLockfile && match.missingPatterns.length > 0) {
+      throw new MessageError(this.reporter.lang('frozenLockfileError'));
+    }
 
-    const match = await this.matchesIntegrityHash(patterns);
     const haveLockfile = await fs.exists(path.join(this.config.cwd, constants.LOCKFILE_FILENAME));
 
-    if (match.matches && haveLockfile) {
+    if (match.integrityHashMatches && haveLockfile) {
       this.reporter.success(this.reporter.lang('upToDate'));
       return true;
     }
 
-    if (!patterns.length && !match.expected) {
+    if (!patterns.length && !match.integrityFileMissing) {
       this.reporter.success(this.reporter.lang('nothingToInstall'));
       await this.createEmptyManifestFolders();
       await this.saveLockfileAndIntegrity(patterns);
@@ -398,8 +392,7 @@ export class Install {
 
     steps.push(async (curr: number, total: number) => {
       // remove integrity hash to make this operation atomic
-      const loc = await this.getIntegrityHashLocation();
-      await fs.unlink(loc);
+      await this.integrityChecker.removeIntegrityFile();
       this.reporter.step(curr, total, this.reporter.lang('linkingDependencies'), emoji.get('link'));
       await this.linker.init(patterns, this.flags.linkDuplicates);
     });
@@ -556,41 +549,26 @@ export class Install {
     return flattenedPatterns;
   }
 
-  /**
-   * Check if the loaded lockfile has all the included patterns
-   */
-
-  lockFileInSync(patterns: Array<string>): boolean {
-    let inSync = true;
-    for (const pattern of patterns) {
-      if (!this.lockfile.getLocked(pattern)) {
-        inSync = false;
-        break;
-      }
-    }
-    return inSync;
-  }
-
   /**
    * Save updated integrity and lockfiles.
    */
 
   async saveLockfileAndIntegrity(patterns: Array<string>): Promise<void> {
-    // stringify current lockfile
-    const lockSource = lockStringify(this.lockfile.getLockfile(this.resolver.patterns));
-
-    // write integrity hash
-    await this.writeIntegrityHash(lockSource, patterns);
-
     // --no-lockfile or --pure-lockfile flag
     if (this.flags.lockfile === false || this.flags.pureLockfile) {
       return;
     }
 
-    const inSync = this.lockFileInSync(patterns);
+    // stringify current lockfile
+    const lockSource = lockStringify(this.lockfile.getLockfile(this.resolver.patterns));
 
-    // remove is followed by install with force on which we rewrite lockfile
-    if (inSync && patterns.length && !this.flags.force) {
+    // write integrity hash
+    await this.integrityChecker.save(patterns, lockSource, this.flags, this.resolver.usedRegistries);
+
+    const lockFileHasAllPatterns = patterns.filter((p) => !this.lockfile.getLocked(p)).length === 0;
+
+    // remove command is followed by install with force, lockfile will be rewritten in any case then
+    if (lockFileHasAllPatterns && patterns.length && !this.flags.force) {
       return;
     }
 
@@ -607,111 +585,6 @@ export class Install {
     this.reporter.success(this.reporter.lang('savedLockfile'));
   }
 
-  /**
-   * Check if the integrity hash of this installation matches one on disk.
-   */
-
-  async matchesIntegrityHash(patterns: Array<string>): Promise<IntegrityMatch> {
-    const loc = await this.getIntegrityHashLocation();
-    if (!await fs.exists(loc)) {
-      return {
-        actual: '',
-        expected: '',
-        loc,
-        matches: false,
-      };
-    }
-
-    const lockSource = lockStringify(this.lockfile.getLockfile(this.resolver.patterns));
-    const actual = this.generateIntegrityHash(lockSource, patterns);
-    const expected = (await fs.readFile(loc)).trim();
-
-    return {
-      actual,
-      expected,
-      loc,
-      matches: actual === expected,
-    };
-  }
-
-  /**
-   * Get the location of an existing integrity hash. If none exists then return the location where we should
-   * write a new one.
-   */
-
-  async getIntegrityHashLocation(): Promise<string> {
-    // build up possible folders
-    const possibleFolders = [];
-    if (this.config.modulesFolder) {
-      possibleFolders.push(this.config.modulesFolder);
-    }
-
-    // get a list of registry names to check existence in
-    let checkRegistryNames = this.resolver.usedRegistries;
-    if (!checkRegistryNames.length) {
-      // we haven't used any registries yet
-      checkRegistryNames = registryNames;
-    }
-
-    // ensure we only write to a registry folder that was used
-    for (const name of checkRegistryNames) {
-      const loc = path.join(this.config.cwd, this.config.registries[name].folder);
-      possibleFolders.push(loc);
-    }
-
-    // if we already have an integrity hash in one of these folders then use it's location otherwise use the
-    // first folder
-    const possibles = possibleFolders.map((folder): string => path.join(folder, constants.INTEGRITY_FILENAME));
-    let loc = possibles[0];
-    for (const possibleLoc of possibles) {
-      if (await fs.exists(possibleLoc)) {
-        loc = possibleLoc;
-        break;
-      }
-    }
-    return loc;
-  }
-  /**
-   * Write the integrity hash of the current install to disk.
-   */
-
-  async writeIntegrityHash(lockSource: string, patterns: Array<string>): Promise<void> {
-    const loc = await this.getIntegrityHashLocation();
-    invariant(loc, 'expected integrity hash location');
-    await fs.mkdirp(path.dirname(loc));
-    await fs.writeFile(loc, this.generateIntegrityHash(lockSource, patterns));
-  }
-
-  /**
-   * Generate integrity hash of input lockfile.
-   */
-
-  generateIntegrityHash(lockfile: string, patterns: Array<string>): string {
-    const opts = [lockfile];
-
-    opts.push(`patterns:${patterns.sort(sortAlpha).join(',')}`);
-
-    if (this.flags.flat) {
-      opts.push('flat');
-    }
-
-    if (this.config.production) {
-      opts.push('production');
-    }
-
-    const linkedModules = this.config.linkedModules;
-    if (linkedModules.length) {
-      opts.push(`linked:${linkedModules.join(',')}`);
-    }
-
-    const mirror = this.config.getOfflineMirrorPath();
-    if (mirror != null) {
-      opts.push(`mirror:${mirror}`);
-    }
-
-    return crypto.hash(opts.join('-'), 'sha256');
-  }
-
   /**
    * Load the dependency graph of the current install. Only does package resolving and wont write to the cwd.
    */

src/cli/index.js

@@ -56,7 +56,7 @@ commander.option('--har', 'save HAR output of network traffic');
 commander.option('--ignore-platform', 'ignore platform checks');
 commander.option('--ignore-engines', 'ignore engines check');
 commander.option('--ignore-optional', 'ignore optional dependencies');
-commander.option('--force', 'install and build scripts even if they were built before, overwrite lockfile');
+commander.option('--force', 'install and build packages even if they were built before, overwrite lockfile');
 commander.option('--skip-integrity-check', 'run install without checking if node_modules is installed');
 commander.option('--no-bin-links', "don't generate bin links when setting up packages");
 commander.option('--flat', 'only allow one version of a package');