fix(core): Improvements to Redis cache plugin (#3303)

packages/core/e2e/cache-service-default.e2e-spec.ts

@@ -15,7 +15,9 @@ import {
     invalidatesByMultipleTags,
     invalidatesBySingleTag,
     invalidatesManyByMultipleTags,
+    invalidTTLsShouldNotSetKey,
     setsAKey,
+    setsAKeyWithSubSecondTtl,
     setsAKeyWithTtl,
     setsArrayOfObjects,
     setsArrayValue,
@@ -66,6 +68,8 @@ describe('CacheService with DefaultCachePlugin (sql)', () => {
 
     it('sets a key with ttl', () => setsAKeyWithTtl(cacheService, ttlProvider));
 
+    it('sets a key with sub-second ttl', () => setsAKeyWithSubSecondTtl(cacheService, ttlProvider));
+
     it('evicts the oldest key when cache is full', () => evictsTheOldestKeyWhenCacheIsFull(cacheService));
 
     it('invalidates by single tag', () => invalidatesBySingleTag(cacheService));
@@ -75,4 +79,6 @@ describe('CacheService with DefaultCachePlugin (sql)', () => {
     it('invalidates many by multiple tags', () => invalidatesManyByMultipleTags(cacheService));
 
     it('invalidates a large number of keys by tag', () => invalidatesALargeNumberOfKeysByTag(cacheService));
+
+    it('invalid ttls should not set key', () => invalidTTLsShouldNotSetKey(cacheService));
 });

packages/core/e2e/cache-service-in-memory.e2e-spec.ts

@@ -16,7 +16,9 @@ import {
     invalidatesByMultipleTags,
     invalidatesBySingleTag,
     invalidatesManyByMultipleTags,
+    invalidTTLsShouldNotSetKey,
     setsAKey,
+    setsAKeyWithSubSecondTtl,
     setsAKeyWithTtl,
     setsArrayOfObjects,
     setsArrayValue,
@@ -67,6 +69,8 @@ describe('CacheService in-memory', () => {
 
     it('sets a key with ttl', () => setsAKeyWithTtl(cacheService, ttlProvider));
 
+    it('sets a key with sub-second ttl', () => setsAKeyWithSubSecondTtl(cacheService, ttlProvider));
+
     it('evicts the oldest key when cache is full', () => evictsTheOldestKeyWhenCacheIsFull(cacheService));
 
     it('invalidates by single tag', () => invalidatesBySingleTag(cacheService));
@@ -76,4 +80,6 @@ describe('CacheService in-memory', () => {
     it('invalidates many by multiple tags', () => invalidatesManyByMultipleTags(cacheService));
 
     it('invalidates a large number of keys by tag', () => invalidatesALargeNumberOfKeysByTag(cacheService));
+
+    it('invalid ttls should not set key', () => invalidTTLsShouldNotSetKey(cacheService));
 });

packages/core/e2e/cache-service-redis.e2e-spec.ts

@@ -1,7 +1,7 @@
-import { CacheService, mergeConfig, RedisCachePlugin } from '@vendure/core';
+import { CacheService, Logger, mergeConfig, RedisCachePlugin } from '@vendure/core';
 import { createTestEnvironment } from '@vendure/testing';
 import path from 'path';
-import { afterAll, beforeAll, describe, it } from 'vitest';
+import { vi, afterAll, beforeAll, describe, it, expect } from 'vitest';
 
 import { initialData } from '../../../e2e-common/e2e-initial-data';
 import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config';
@@ -13,7 +13,10 @@ import {
     invalidatesByMultipleTags,
     invalidatesBySingleTag,
     invalidatesManyByMultipleTags,
+    invalidTTLsShouldNotSetKey,
     setsAKey,
+    setsAKeyWithSubSecondTtl,
+    setsAKeyWithTtl,
     setsArrayOfObjects,
     setsArrayValue,
     setsObjectValue,
@@ -61,6 +64,30 @@ describe('CacheService with RedisCachePlugin', () => {
 
     it('deletes a key', () => deletesAKey(cacheService));
 
+    it('sets a key with ttl', async () => {
+        await cacheService.set('test-key1', 'test-value', { ttl: 1000 });
+        const result = await cacheService.get('test-key1');
+        expect(result).toBe('test-value');
+
+        await new Promise(resolve => setTimeout(resolve, 1500));
+
+        const result2 = await cacheService.get('test-key1');
+
+        expect(result2).toBeUndefined();
+    });
+
+    it('sets a key with sub-second ttl', async () => {
+        await cacheService.set('test-key2', 'test-value', { ttl: 900 });
+        const result = await cacheService.get('test-key2');
+        expect(result).toBe('test-value');
+
+        await new Promise(resolve => setTimeout(resolve, 1500));
+
+        const result2 = await cacheService.get('test-key2');
+
+        expect(result2).toBeUndefined();
+    });
+
     it('invalidates by single tag', () => invalidatesBySingleTag(cacheService));
 
     it('invalidates by multiple tags', () => invalidatesByMultipleTags(cacheService));
@@ -68,4 +95,6 @@ describe('CacheService with RedisCachePlugin', () => {
     it('invalidates many by multiple tags', () => invalidatesManyByMultipleTags(cacheService));
 
     it('invalidates a large number of keys by tag', () => invalidatesALargeNumberOfKeysByTag(cacheService));
+
+    it('invalid ttls should not set key', () => invalidTTLsShouldNotSetKey(cacheService));
 });

packages/core/e2e/fixtures/cache-service-shared-tests.ts

@@ -1,5 +1,5 @@
-import { CacheService } from '@vendure/core';
-import { expect } from 'vitest';
+import { CacheService, Logger } from '@vendure/core';
+import { expect, vi } from 'vitest';
 
 import { TestingCacheTtlProvider } from '../../src/cache/cache-ttl-provider';
 
@@ -59,6 +59,22 @@ export async function setsAKeyWithTtl(cacheService: CacheService, ttlProvider: T
     expect(result2).toBeUndefined();
 }
 
+export async function setsAKeyWithSubSecondTtl(
+    cacheService: CacheService,
+    ttlProvider: TestingCacheTtlProvider,
+) {
+    ttlProvider.setTime(new Date().getTime());
+    await cacheService.set('test-key', 'test-value', { ttl: 900 });
+    const result = await cacheService.get('test-key');
+    expect(result).toBe('test-value');
+
+    ttlProvider.incrementTime(2000);
+
+    const result2 = await cacheService.get('test-key');
+
+    expect(result2).toBeUndefined();
+}
+
 export async function evictsTheOldestKeyWhenCacheIsFull(cacheService: CacheService) {
     await cacheService.set('key1', 'value1');
     await cacheService.set('key2', 'value2');
@@ -127,3 +143,10 @@ export async function invalidatesALargeNumberOfKeysByTag(cacheService: CacheServ
         expect(await cacheService.get(`taggedKey${i}`)).toBeUndefined();
     }
 }
+
+export async function invalidTTLsShouldNotSetKey(cacheService: CacheService) {
+    await cacheService.set('test-key', 'test-value', { ttl: -1500 });
+    const result = await cacheService.get('test-key');
+
+    expect(result).toBeUndefined();
+}

packages/core/src/plugin/redis-cache-plugin/redis-cache-strategy.ts

@@ -63,13 +63,24 @@ export class RedisCacheStrategy implements CacheStrategy {
                     return;
                 }
             }
-            multi.set(namedspacedKey, JSON.stringify(value), 'EX', ttl);
+            if (Math.round(ttl) <= 0) {
+                Logger.error(
+                    `Could not set cache item ${key}: TTL must be greater than 0 seconds`,
+                    loggerCtx,
+                );
+                return;
+            }
+            multi.set(namedspacedKey, JSON.stringify(value), 'EX', Math.round(ttl));
             if (options?.tags) {
                 for (const tag of options.tags) {
                     multi.sadd(this.tagNamespace(tag), namedspacedKey);
                 }
             }
-            await multi.exec();
+            const results = await multi.exec();
+            const resultWithError = results?.find(([err, _]) => err);
+            if (resultWithError) {
+                throw resultWithError[0];
+            }
         } catch (e: any) {
             Logger.error(`Could not set cache item ${key}: ${e.message as string}`, loggerCtx);
         }