Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Invalidate the cache if cspell version has changed. #2580

Merged
merged 3 commits into from Mar 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/cspell/src/app.ts
@@ -1,6 +1,5 @@
import type { Command } from 'commander';
import { program } from 'commander';
import * as path from 'path';
import { satisfies as semverSatisfies } from 'semver';
import { commandCheck } from './commandCheck';
import { commandLink } from './commandLink';
Expand All @@ -9,7 +8,7 @@ import { commandSuggestion } from './commandSuggestion';
import { commandTrace } from './commandTrace';
import { ApplicationError } from './util/errors';
// eslint-disable-next-line @typescript-eslint/no-var-requires
const npmPackage = require(path.join(__dirname, '..', 'package.json'));
const npmPackage = require('../package.json');

export { LinterCliOptions as Options } from './options';
export { CheckFailed } from './util/errors';
Expand Down
5 changes: 4 additions & 1 deletion packages/cspell/src/lint/lint.ts
Expand Up @@ -28,6 +28,9 @@ import { getTimeMeasurer } from '../util/timer';
import * as util from '../util/util';
import { LintRequest } from './LintRequest';
import chalk = require('chalk');
// eslint-disable-next-line @typescript-eslint/no-var-requires
const npmPackage = require('../../package.json');
const version = npmPackage.version;

export async function runLint(cfg: LintRequest): Promise<RunResult> {
let { reporter } = cfg;
Expand Down Expand Up @@ -253,7 +256,7 @@ export async function runLint(cfg: LintRequest): Promise<RunResult> {
const { root } = cfg;

try {
const cacheSettings = await calcCacheSettings(configInfo.config, cfg.options, root);
const cacheSettings = await calcCacheSettings(configInfo.config, { ...cfg.options, version }, root);
const files = await determineFilesToCheck(configInfo, cfg, reporter, globInfo);

return await processFiles(files, configInfo, cacheSettings);
Expand Down
2 changes: 1 addition & 1 deletion packages/cspell/src/options.ts
@@ -1,6 +1,6 @@
import type { CacheOptions } from './util/cache';

export interface LinterOptions extends BaseOptions, CacheOptions {
export interface LinterOptions extends BaseOptions, Omit<CacheOptions, 'version'> {
/**
* Display verbose information
*/
Expand Down
5 changes: 5 additions & 0 deletions packages/cspell/src/util/cache/CacheOptions.ts
@@ -1,6 +1,11 @@
import type { CacheStrategy } from '@cspell/cspell-types';

export interface CacheOptions {
/**
* The version of `cspell` that made the cache entry.
* Cache entries must match the `major.minor` version.
*/
version: string;
/**
* Store the info about processed files in order to only operate on the changed ones.
*/
Expand Down
3 changes: 2 additions & 1 deletion packages/cspell/src/util/cache/DiskCache.test.ts
Expand Up @@ -39,7 +39,7 @@ describe('DiskCache', () => {
};

beforeEach(() => {
diskCache = new DiskCache('.foobar', false);
diskCache = new DiskCache('.foobar', false, 'version');
fileEntryCache = mockCreateFileEntryCache.mock.results[0].value;
});

Expand Down Expand Up @@ -173,6 +173,7 @@ function entry(result: CachedFileResult, dependencies: string[] = [], size = 100
data: {
r: result,
d: dependencies,
v: 'version',
},
},
};
Expand Down
15 changes: 13 additions & 2 deletions packages/cspell/src/util/cache/DiskCache.ts
Expand Up @@ -16,6 +16,8 @@ interface CachedData {
r: CachedFileResult;
/** dependencies */
d: string[];
/** meta version */
v: string;
}

interface CSpellCachedMetaData {
Expand All @@ -32,7 +34,7 @@ export class DiskCache implements CSpellLintResultCache {
private changedDependencies: Set<string> = new Set();
private knownDependencies: Set<string> = new Set();

constructor(cacheFileLocation: string, useCheckSum: boolean) {
constructor(cacheFileLocation: string, useCheckSum: boolean, readonly version: string) {
this.fileEntryCache = fileEntryCache.createFromFile(resolvePath(cacheFileLocation), useCheckSum);
}

Expand All @@ -41,13 +43,21 @@ export class DiskCache implements CSpellLintResultCache {
const meta = fileDescriptor.meta as CSpellCacheMeta;
const data = meta?.data;
const result = data?.r;
const versionMatches = this.version === data?.v;

// Cached lint results are valid if and only if:
// 1. The file is present in the filesystem
// 2. The file has not changed since the time it was previously linted
// 3. The CSpell configuration has not changed since the time the file was previously linted
// If any of these are not true, we will not reuse the lint results.
if (fileDescriptor.notFound || fileDescriptor.changed || !meta || !result || !this.checkDependencies(data.d)) {
if (
fileDescriptor.notFound ||
fileDescriptor.changed ||
!meta ||
!result ||
!versionMatches ||
!this.checkDependencies(data.d)
) {
return undefined;
}

Expand Down Expand Up @@ -77,6 +87,7 @@ export class DiskCache implements CSpellLintResultCache {
const data: CachedData = {
r: result,
d: dependsUponFiles,
v: this.version,
};

meta.data = data;
Expand Down
39 changes: 32 additions & 7 deletions packages/cspell/src/util/cache/createCache.test.ts
@@ -1,15 +1,17 @@
import { calcCacheSettings, createCache } from './createCache';
import { DiskCache } from './DiskCache';
import { CacheOptions } from './CacheOptions';
import * as path from 'path';
import { resolve as r } from 'path';
import { CreateCacheSettings } from '.';
import { CacheOptions } from './CacheOptions';
import { calcCacheSettings, createCache, __testing__ } from './createCache';
import { DiskCache } from './DiskCache';
import { DummyCache } from './DummyCache';

jest.mock('./DiskCache');

const mockedDiskCache = jest.mocked(DiskCache);

const version = '5.20.0-alpha.192';

const U = undefined;
const T = true;
const F = false;
Expand All @@ -30,8 +32,11 @@ describe('Validate calcCacheSettings', () => {
${{}} | ${{ cacheStrategy: 'content' }} | ${'.'} | ${cco(F, U, 'content')} | ${'override default strategy'}
${{}} | ${co(T, U, 'content')} | ${'.'} | ${cco(T, U, 'content')} | ${'override strategy'}
${cs(U, U, 'content')} | ${co(T, U, 'metadata')} | ${'.'} | ${cco(T, U, 'metadata')} | ${'override config strategy'}
${cs(T, U, 'content')} | ${{}} | ${'.'} | ${cco(T, U, 'content')} | ${'override default strategy'}
${cs(T, U, 'content')} | ${{ version }} | ${'.'} | ${cco(T, U, 'content')} | ${'override default strategy'}
`('calcCacheSettings $comment - $config $options $root', async ({ config, options, root, expected }) => {
if (!options.version) {
options.version = version;
}
expect(await calcCacheSettings({ cache: config }, options, root)).toEqual(expected);
});
});
Expand All @@ -52,6 +57,26 @@ describe('Validate createCache', () => {
});
});

describe('validate normalizeVersion', () => {
test.each`
version | expected
${'5.8'} | ${'5.8'}
${'5.8.30'} | ${'5.8'}
${'5.8.30-alpha.2'} | ${'5.8'}
${'5.28.30-alpha.2'} | ${'5.28'}
${'6.0.30-alpha.2'} | ${'6.0'}
`('normalizeVersion $version', ({ version, expected }) => {
expect(__testing__.normalizeVersion(version)).toBe(expected + __testing__.versionSuffix);
});
test.each`
version
${'5'}
${''}
`('normalizeVersion bad "$version"', ({ version }) => {
expect(() => __testing__.normalizeVersion(version)).toThrow();
});
});

/**
* CreateCacheSettings
*/
Expand All @@ -63,13 +88,13 @@ function cco(
if (cacheLocation) {
cacheLocation = path.resolve(process.cwd(), cacheLocation);
}
return { useCache, cacheLocation, cacheStrategy };
return { useCache, cacheLocation, cacheStrategy, version };
}

function cs(useCache?: boolean, cacheLocation?: string, cacheStrategy?: CacheOptions['cacheStrategy']) {
return { useCache, cacheLocation, cacheStrategy };
return { useCache, cacheLocation, cacheStrategy, version };
}

function co(cache?: boolean, cacheLocation?: string, cacheStrategy?: CacheOptions['cacheStrategy']) {
return { cache, cacheLocation, cacheStrategy };
return { cache, cacheLocation, cacheStrategy, version };
}
35 changes: 30 additions & 5 deletions packages/cspell/src/util/cache/createCache.ts
@@ -1,24 +1,33 @@
import { CacheSettings, CSpellSettings } from '@cspell/cspell-types';
import assert from 'assert';
import { stat } from 'fs-extra';
import path from 'path';
import { CacheOptions } from '.';
import { isError } from '../errors';
import { CSpellLintResultCache } from './CSpellLintResultCache';
import { DiskCache } from './DiskCache';
import { DummyCache } from './DummyCache';
import { stat } from 'fs-extra';
import { isError } from '../errors';

// cspell:word cspellcache
export const DEFAULT_CACHE_LOCATION = '.cspellcache';

export type CreateCacheSettings = Required<CacheSettings>;
export interface CreateCacheSettings extends Required<CacheSettings> {
/**
* cspell version used to validate cache entries.
*/
version: string;
}

const versionSuffix = '';

/**
* Creates CSpellLintResultCache (disk cache if caching is enabled in config or dummy otherwise)
*/

export function createCache(options: CreateCacheSettings): CSpellLintResultCache {
const { useCache, cacheLocation, cacheStrategy } = options;
return useCache ? new DiskCache(path.resolve(cacheLocation), cacheStrategy === 'content') : new DummyCache();
return useCache
? new DiskCache(path.resolve(cacheLocation), cacheStrategy === 'content', normalizeVersion(options.version))
: new DummyCache();
}

export async function calcCacheSettings(
Expand All @@ -37,6 +46,7 @@ export async function calcCacheSettings(
useCache,
cacheLocation,
cacheStrategy,
version: cacheOptions.version,
};
}

Expand All @@ -52,3 +62,18 @@ async function resolveCacheLocation(cacheLocation: string): Promise<string> {
throw err;
}
}

/**
* Normalizes the version and return only `major.minor + versionSuffix`
* @param version The cspell semantic version.
*/
function normalizeVersion(version: string): string {
const parts = version.split('.').slice(0, 2);
assert(parts.length === 2);
return parts.join('.') + versionSuffix;
}

export const __testing__ = {
normalizeVersion,
versionSuffix,
};