jaredwray / cacheable

a robust, scalable, and maintained set of caching packages
https://cacheable.org
MIT License
1.63k stars 165 forks source link

regression: file-entry-cache fails to update `meta.data` after the entry has been created. #901

Closed Jason3S closed 1 week ago

Jason3S commented 1 week ago

I use the meta.data to store calculation results and file dependencies. This seems to have broken between versions 9 and 10.

Once a cache entry has been created on disk, updating the meta.data will not get saved to the cache.

Here is a diff to the unit tests that show the failure:

diff --git a/packages/file-entry-cache/test/index.test.ts b/packages/file-entry-cache/test/index.test.ts
index 13769ea..9273dfa 100644
--- a/packages/file-entry-cache/test/index.test.ts
+++ b/packages/file-entry-cache/test/index.test.ts
@@ -3,7 +3,7 @@ import path from 'node:path';
 import {
    describe, test, expect, beforeAll, afterAll, beforeEach, afterEach,
 } from 'vitest';
-import defaultFileEntryCache, {FileEntryCache, type FileEntryCacheOptions} from '../src/index.js';
+import defaultFileEntryCache, {createFromFile, FileEntryCache, type FileEntryCacheOptions} from '../src/index.js';

 describe('file-entry-cache with options', () => {
    test('should initialize', () => {
@@ -421,6 +421,40 @@ describe('reconcile()', () => {
        fs.rmSync(path.resolve(`./${fileCacheName}`), {recursive: true, force: true});
    });

+   test('create / reconcile / update / read', () => {
+       const shared = {shared: 'shared'};
+       const cachePath = `${fileCacheName}`;
+
+       function calculateData(name: string) {
+           return {testingFooVariable: '11', name, shared};
+       }
+
+       function prepCache() {
+           const fileEntryCache = createFromFile(`./${cachePath}/test1.cache`, true, `./${fileCacheName}`);
+           fileEntryCache.getFileDescriptor('test1.txt');
+           fileEntryCache.getFileDescriptor('test2.txt');
+           fileEntryCache.getFileDescriptor('test3.txt');
+           fileEntryCache.reconcile();
+       }
+
+       function addMetaData() {
+           const fileEntryCache = createFromFile(`./${cachePath}/test1.cache`, true, `./${fileCacheName}`);
+           fileEntryCache.getFileDescriptor('test1.txt').meta.data = calculateData('test1.txt');
+           fileEntryCache.getFileDescriptor('test2.txt').meta.data = calculateData('test2.txt');
+           fileEntryCache.getFileDescriptor('test3.txt').meta.data = calculateData('test3.txt');
+           fileEntryCache.reconcile();
+       }
+
+       prepCache();
+       addMetaData();
+       const fileEntryCache = createFromFile(`./${cachePath}/test1.cache`, true, `./${fileCacheName}`);
+       expect(fileEntryCache.getFileDescriptor('test1.txt').meta.data).toEqual(calculateData('test1.txt'));
+       expect(fileEntryCache.getFileDescriptor('test2.txt').meta.data).toEqual(calculateData('test2.txt'));
+       expect(fileEntryCache.getFileDescriptor('test3.txt').meta.data).toEqual(calculateData('test3.txt'));
+
+       fs.rmSync(path.resolve(`./${cachePath}`), {recursive: true, force: true});
+   });
+
    test('should reconcile with deleted files', () => {
        const options: FileEntryCacheOptions = {
            currentWorkingDirectory: `./${fileCacheName}`,
jaredwray commented 1 week ago

@Jason3S thanks. Looking at this now

jaredwray commented 1 week ago

@Jason3S - this should be resolved and I added a unit test to validate the fix.

    test('should save the meta data after the first call and loading data', () => {
        const shared = {shared: 'shared'};
        const data = {testingFooVariable: '11', name: 'test1.txt', shared};
        const fileEntryCache = new FileEntryCache({useCheckSum: true});
        const testFile1 = path.resolve('./.cacheGFD/test1.txt');
        const fileDescriptor = fileEntryCache.getFileDescriptor(testFile1);
        fileDescriptor.meta.data = data;
        expect(fileDescriptor).toBeDefined();
        fileEntryCache.reconcile();

        // Add the meta data to the cache
        const fileEntryCache2 = createFromFile(fileEntryCache.cache.cacheFilePath, true);
        const fileDescriptor2 = fileEntryCache2.getFileDescriptor(testFile1);
        const data2 = {testingFooVariable: '22', name: 'test1.txt', shared};
        fileDescriptor2.meta.data = data2;
        fileEntryCache2.reconcile();

        // Load the meta data from the cache
        const fileEntryCache3 = createFromFile(fileEntryCache.cache.cacheFilePath, true);
        const fileDescriptor3 = fileEntryCache3.getFileDescriptor(testFile1);
        expect(fileDescriptor3).toBeDefined();
        expect(fileDescriptor3.meta.data).toEqual(data2);

        // Verify that the data shows changed
        const fileDescriptor4 = fileEntryCache3.getFileDescriptor(testFile1);
        expect(fileDescriptor4).toBeDefined();
        expect(fileDescriptor4.meta.data).toEqual(data2);
        expect(fileDescriptor4.changed).toEqual(true);
    });

It will save the state and also I fixed a bug where we didnt recognize when meta.data state was changed.

This is now released file-entry-cache@10.0.3

Jason3S commented 1 week ago

Thank you!

Jason3S commented 1 week ago

@jaredwray,

Thank you for the quick update.

I tried it, but it is not working as expected.

  1. The cache file size is 10x larger than before. My meta.data entries can have shared nested meta.data objects with other entries. This used to get optimized so that shared objects were stored only once.
  2. It looks like changed is always getting set to true if there is meta.data.
jaredwray commented 1 week ago

@Jason3S - I will revert the issue you bring up on #2 as you are correct that when looking at the previous code it did not detect meta change.

On the #1 we have used the flatted which does store circular references as much as possible but might struggle with the way we are handling it now. I am going to not pursue that as it seems to be acceptable by other projects using this.

jaredwray commented 1 week ago

https://github.com/jaredwray/cacheable/pull/906 and file-entry-cache@10.0.4 has been published.