chore(medusa): Replace inline Redis cache in favour of the CacheService (#2802)

* chore: Replace all usage of redis for the cache in favour of the cache service

* Create four-bugs-mate.md

* chore: missing new line

.changeset/four-bugs-mate.md

@@ -0,0 +1,5 @@
+---
+"@medusajs/medusa": patch
+---
+
+chore: Replace all usage of redis for the cache in favour of the cache service

packages/medusa/src/services/__fixtures__/tax-provider.ts

@@ -0,0 +1,45 @@
+import { cacheServiceMock } from "../__mocks__/cache";
+import TaxProviderService from "../tax-provider";
+import { EventBusServiceMock } from "../__mocks__/event-bus";
+import { asClass, asValue, createContainer } from "awilix";
+import { MockManager, MockRepository } from "medusa-test-utils"
+
+export function getCacheKey(id, regionId) {
+  return `txrtcache:${id}:${regionId}`
+}
+
+export const defaultContainer = createContainer()
+defaultContainer.register("manager", asValue(MockManager))
+defaultContainer.register("taxRateService", asValue({}))
+defaultContainer.register("systemTaxService", asValue("system"))
+defaultContainer.register("tp_test", asValue("good"))
+defaultContainer.register("cacheService", asValue(cacheServiceMock))
+defaultContainer.register("taxProviderService", asClass(TaxProviderService))
+defaultContainer.register("taxProviderRepository", asValue(MockRepository))
+defaultContainer.register("lineItemTaxLineRepository", asValue(MockRepository({ create: (d) => d })))
+defaultContainer.register("shippingMethodTaxLineRepository", asValue(MockRepository))
+defaultContainer.register("eventBusService", asValue(EventBusServiceMock))
+
+export const getTaxLineFactory = ({ items, region }) => {
+  const cart = {
+    shipping_methods: [],
+    items: items.map((i) => {
+      return {
+        id: i.id,
+        variant: {
+          product_id: i.product_id,
+        },
+      }
+    }),
+  }
+
+  const calculationContext = {
+    region,
+    customer: {},
+    allocation_map: {},
+    shipping_address: {},
+    shipping_methods: [],
+  }
+
+  return { cart, calculationContext }
+}

packages/medusa/src/services/__tests__/tax-provider.js

@@ -1,16 +1,14 @@
-import { MockManager, MockRepository } from "medusa-test-utils"
+import { asValue, createContainer } from "awilix"
 import TaxProviderService from "../tax-provider"
+import { defaultContainer, getCacheKey, getTaxLineFactory } from "../__fixtures__/tax-provider";
 
 describe("TaxProviderService", () => {
-  describe("retrieveProvider", () => {
+  afterEach(() => {
-    const container = {
+    jest.clearAllMocks()
-      manager: MockManager,
+  })
-      taxRateService: {},
-      systemTaxService: "system",
-      tp_test: "good",
-    }
   
-    const providerService = new TaxProviderService(container)
+  describe("retrieveProvider", () => {
+    const providerService = defaultContainer.resolve("taxProviderService")
 
     it("successfully retrieves system provider", () => {
       const provider = providerService.retrieveProvider({
@@ -48,16 +46,13 @@ describe("TaxProviderService", () => {
         item_id: "item_1",
       },
     ])
-    const container = {
+    const container = createContainer({}, defaultContainer)
-      manager: MockManager,
+    container.register("systemTaxService", asValue({
-      lineItemTaxLineRepository: MockRepository({ create: (d) => d }),
+      getTaxLines: mockCalculateLineItemTaxes,
-      systemTaxService: {
+    }))
-        getTaxLines: mockCalculateLineItemTaxes,
-      },
-    }
 
     test("success", async () => {
-      const providerService = new TaxProviderService(container)
+      const providerService = container.resolve("taxProviderService")
       providerService.getRegionRatesForProduct = jest.fn(() => [])
 
       const region = { id: "test", tax_provider_id: null }
@@ -80,10 +75,11 @@ describe("TaxProviderService", () => {
 
       expect(rates).toEqual(expected)
 
-      expect(container.lineItemTaxLineRepository.create).toHaveBeenCalledTimes(
+      const lineItemTaxLineRepository = container.resolve("lineItemTaxLineRepository")
+      expect(lineItemTaxLineRepository.create).toHaveBeenCalledTimes(
         1
       )
-      expect(container.lineItemTaxLineRepository.create).toHaveBeenCalledWith(
+      expect(lineItemTaxLineRepository.create).toHaveBeenCalledWith(
         expected[0]
       )
 
@@ -108,24 +104,16 @@ describe("TaxProviderService", () => {
   })
 
   describe("getRegionRatesForProduct", () => {
-    const container = {
+    const container = createContainer({}, defaultContainer)
-      manager: MockManager,
+    container.register("taxRateService", asValue({
-      taxRateService: {
+      withTransaction: function () {
-        listByProduct: jest.fn(),
+        return this
       },
-    }
+      listByProduct: jest.fn(() => Promise.resolve([])),
+    }))
 
     test("success", async () => {
-      container.taxRateService = {
+      const providerService = container.resolve("taxProviderService")
-        withTransaction: function () {
-          return this
-        },
-        listByProduct: jest.fn(() => Promise.resolve([])),
-      }
-
-      const providerService = new TaxProviderService(container)
-      providerService.getCacheEntry = jest.fn(() => null)
-      providerService.setCache = jest.fn()
 
       const rates = await providerService.getRegionRatesForProduct("prod_id", {
         id: "reg_id",
@@ -142,31 +130,21 @@ describe("TaxProviderService", () => {
       ]
       expect(rates).toEqual(expected)
 
-      expect(providerService.getCacheEntry).toHaveBeenCalledTimes(1)
+      const cacheService = container.resolve("cacheService")
-      expect(providerService.getCacheEntry).toHaveBeenCalledWith(
+      const cacheKey = getCacheKey("prod_id", "reg_id")
-        "prod_id",
-        "reg_id"
-      )
 
-      expect(providerService.setCache).toHaveBeenCalledTimes(1)
+      expect(cacheService.get).toHaveBeenCalledTimes(1)
-      expect(providerService.setCache).toHaveBeenCalledWith(
+      expect(cacheService.get).toHaveBeenCalledWith(cacheKey)
-        "prod_id",
+
-        "reg_id",
+      expect(cacheService.set).toHaveBeenCalledTimes(1)
+      expect(cacheService.set).toHaveBeenCalledWith(
+        cacheKey,
         expected
       )
     })
 
     test("success - without product rates", async () => {
-      container.taxRateService = {
+      const providerService = container.resolve("taxProviderService")
-        withTransaction: function () {
-          return this
-        },
-        listByProduct: jest.fn(() => Promise.resolve([])),
-      }
-
-      const providerService = new TaxProviderService(container)
-      providerService.getCacheEntry = jest.fn(() => null)
-      providerService.setCache = jest.fn()
 
       const rates = await providerService.getRegionRatesForProduct("prod_id", {
         id: "reg_id",
@@ -181,16 +159,24 @@ describe("TaxProviderService", () => {
           code: "default",
         },
       ]
-      expect(container.taxRateService.listByProduct).toHaveBeenCalledTimes(1)
+
-      expect(container.taxRateService.listByProduct).toHaveBeenCalledWith(
+      const taxRateService = container.resolve("taxRateService")
+
+      expect(taxRateService.listByProduct).toHaveBeenCalledTimes(1)
+      expect(taxRateService.listByProduct).toHaveBeenCalledWith(
         "prod_id",
         { region_id: "reg_id" }
       )
 
-      expect(providerService.setCache).toHaveBeenCalledTimes(1)
+      const cacheService = container.resolve("cacheService")
-      expect(providerService.setCache).toHaveBeenCalledWith(
+      const cacheKey = getCacheKey("prod_id", "reg_id")
-        "prod_id",
+
-        "reg_id",
+      expect(cacheService.get).toHaveBeenCalledTimes(1)
+      expect(cacheService.get).toHaveBeenCalledWith(cacheKey)
+
+      expect(cacheService.set).toHaveBeenCalledTimes(1)
+      expect(cacheService.set).toHaveBeenCalledWith(
+        cacheKey,
         expected
       )
 
@@ -198,7 +184,8 @@ describe("TaxProviderService", () => {
     })
 
     test("success - with product rates", async () => {
-      container.taxRateService = {
+      const container = createContainer({}, defaultContainer)
+      container.register("taxRateService", asValue({
         withTransaction: function () {
           return this
         },
@@ -211,11 +198,9 @@ describe("TaxProviderService", () => {
             },
           ])
         ),
-      }
+      }))
 
-      const providerService = new TaxProviderService(container)
+      const providerService = container.resolve("taxProviderService")
-      providerService.getCacheEntry = jest.fn(() => null)
-      providerService.setCache = jest.fn()
 
       const rates = await providerService.getRegionRatesForProduct("prod_id", {
         id: "reg_id",
@@ -230,16 +215,24 @@ describe("TaxProviderService", () => {
           code: "ptr",
         },
       ]
-      expect(container.taxRateService.listByProduct).toHaveBeenCalledTimes(1)
+
-      expect(container.taxRateService.listByProduct).toHaveBeenCalledWith(
+      const taxRateService = container.resolve("taxRateService")
+
+      expect(taxRateService.listByProduct).toHaveBeenCalledTimes(1)
+      expect(taxRateService.listByProduct).toHaveBeenCalledWith(
         "prod_id",
         { region_id: "reg_id" }
       )
 
-      expect(providerService.setCache).toHaveBeenCalledTimes(1)
+      const cacheService = container.resolve("cacheService")
-      expect(providerService.setCache).toHaveBeenCalledWith(
+      const cacheKey = getCacheKey("prod_id", "reg_id")
-        "prod_id",
+
-        "reg_id",
+      expect(cacheService.get).toHaveBeenCalledTimes(1)
+      expect(cacheService.get).toHaveBeenCalledWith(cacheKey)
+
+      expect(cacheService.set).toHaveBeenCalledTimes(1)
+      expect(cacheService.set).toHaveBeenCalledWith(
+        cacheKey,
         expected
       )
 
@@ -248,14 +241,7 @@ describe("TaxProviderService", () => {
   })
 
   describe("getCacheKey", () => {
-    const container = {
+    const providerService = defaultContainer.resolve("taxProviderService")
-      manager: MockManager,
-      productTaxRateService: {},
-      systemTaxService: "system",
-      tp_test: "good",
-    }
-
-    const providerService = new TaxProviderService(container)
 
     test("formats correctly", () => {
       expect(providerService.getCacheKey("product", "region")).toEqual(
@@ -264,27 +250,3 @@ describe("TaxProviderService", () => {
     })
   })
 })
-
-const getTaxLineFactory = ({ items, region }) => {
-  const cart = {
-    shipping_methods: [],
-    items: items.map((i) => {
-      return {
-        id: i.id,
-        variant: {
-          product_id: i.product_id,
-        },
-      }
-    }),
-  }
-
-  const calculationContext = {
-    region,
-    customer: {},
-    allocation_map: {},
-    shipping_address: {},
-    shipping_methods: [],
-  }
-
-  return { cart, calculationContext }
-}

packages/medusa/src/services/tax-provider.ts

@@ -1,7 +1,6 @@
 import { MedusaError } from "medusa-core-utils"
 import { AwilixContainer } from "awilix"
 import { EntityManager, In } from "typeorm"
-import Redis from "ioredis"
 
 import { LineItemTaxLineRepository } from "../repositories/line-item-tax-line"
 import { ShippingMethodTaxLineRepository } from "../repositories/shipping-method-tax-line"
@@ -17,6 +16,7 @@ import {
 } from "../models"
 import { isCart } from "../types/cart"
 import {
+  ICacheService,
   ITaxService,
   ItemTaxCalculationLine,
   TaxCalculationContext,
@@ -28,8 +28,6 @@ import { TaxLinesMaps, TaxServiceRate } from "../types/tax-service"
 import TaxRateService from "./tax-rate"
 import EventBusService from "./event-bus"
 
-const CACHE_TIME = 30 // seconds
-
 type RegionDetails = {
   id: string
   tax_rate: number | null
@@ -43,24 +41,25 @@ class TaxProviderService extends TransactionBaseService {
   protected transactionManager_: EntityManager
 
   protected readonly container_: AwilixContainer
+  protected readonly cacheService_: ICacheService
   protected readonly taxRateService_: TaxRateService
   protected readonly taxLineRepo_: typeof LineItemTaxLineRepository
   protected readonly smTaxLineRepo_: typeof ShippingMethodTaxLineRepository
   protected readonly taxProviderRepo_: typeof TaxProviderRepository
-  protected readonly redis_: Redis.Redis
   protected readonly eventBus_: EventBusService
 
   constructor(container: AwilixContainer) {
     super(container)
 
     this.container_ = container
+    this.cacheService_ = container["cacheService"]
     this.taxLineRepo_ = container["lineItemTaxLineRepository"]
     this.smTaxLineRepo_ = container["shippingMethodTaxLineRepository"]
     this.taxRateService_ = container["taxRateService"]
     this.eventBus_ = container["eventBusService"]
     this.taxProviderRepo_ = container["taxProviderRepository"]
+
     this.manager_ = container["manager"]
-    this.redis_ = container["redisClient"]
   }
 
   async list(): Promise<TaxProvider[]> {
@@ -76,12 +75,16 @@ class TaxProviderService extends TransactionBaseService {
   retrieveProvider(region: Region): ITaxService {
     let provider: ITaxService
     if (region.tax_provider_id) {
-      provider = this.container_[`tp_${region.tax_provider_id}`]
+      try {
+        provider = this.container_[`tp_${region.tax_provider_id}`]
+      } catch (e) {
+        // noop
+      }
     } else {
       provider = this.container_["systemTaxService"]
     }
 
-    if (!provider) {
+    if (!provider!) {
       throw new MedusaError(
         MedusaError.Types.NOT_FOUND,
         `Could not find a tax provider with id: ${region.tax_provider_id}`
@@ -380,7 +383,8 @@ class TaxProviderService extends TransactionBaseService {
     optionId: string,
     regionDetails: RegionDetails
   ): Promise<TaxServiceRate[]> {
-    const cacheHit = await this.getCacheEntry(optionId, regionDetails.id)
+    const cacheKey = this.getCacheKey(optionId, regionDetails.id)
+    const cacheHit = await this.cacheService_.get<TaxServiceRate[]>(cacheKey)
     if (cacheHit) {
       return cacheHit
     }
@@ -410,7 +414,7 @@ class TaxProviderService extends TransactionBaseService {
       ]
     }
 
-    await this.setCache(optionId, regionDetails.id, toReturn)
+    await this.cacheService_.set(cacheKey, toReturn)
 
     return toReturn
   }
@@ -426,7 +430,8 @@ class TaxProviderService extends TransactionBaseService {
     productId: string,
     region: RegionDetails
   ): Promise<TaxServiceRate[]> {
-    const cacheHit = await this.getCacheEntry(productId, region.id)
+    const cacheKey = this.getCacheKey(productId, region.id)
+    const cacheHit = await this.cacheService_.get<TaxServiceRate[]>(cacheKey)
     if (cacheHit) {
       return cacheHit
     }
@@ -458,67 +463,19 @@ class TaxProviderService extends TransactionBaseService {
       ]
     }
 
-    await this.setCache(productId, region.id, toReturn)
+    await this.cacheService_.set(cacheKey, toReturn)
 
     return toReturn
   }
 
   /**
    * The cache key to get cache hits by.
-   * @param productId - the product id to cache
+   * @param id - the entity id to cache
    * @param regionId - the region id to cache
    * @return the cache key to use for the id set
    */
-  private getCacheKey(productId: string, regionId: string): string {
+  private getCacheKey(id: string, regionId: string): string {
-    return `txrtcache:${productId}:${regionId}`
+    return `txrtcache:${id}:${regionId}`
-  }
-
-  /**
-   * Sets the cache results for a set of ids
-   * @param productId - the product id to cache
-   * @param regionId - the region id to cache
-   * @param value - tax rates to cache
-   * @return promise that resolves after the cache has been set
-   */
-  private async setCache(
-    productId: string,
-    regionId: string,
-    value: TaxServiceRate[]
-  ): Promise<null | string> {
-    const cacheKey = this.getCacheKey(productId, regionId)
-    return await this.redis_.set(
-      cacheKey,
-      JSON.stringify(value),
-      "EX",
-      CACHE_TIME
-    )
-  }
-
-  /**
-   * Gets the cache results for a set of ids
-   * @param productId - the product id to cache
-   * @param regionId - the region id to cache
-   * @return the cached result or null
-   */
-  private async getCacheEntry(
-    productId: string,
-    regionId: string
-  ): Promise<TaxServiceRate[] | null> {
-    const cacheKey = this.getCacheKey(productId, regionId)
-
-    try {
-      const cacheHit = await this.redis_.get(cacheKey)
-      if (cacheHit) {
-        // TODO: Validate that cache has correct data
-        const parsedResults = JSON.parse(cacheHit) as TaxServiceRate[]
-        return parsedResults
-      }
-    } catch (_) {
-      // noop - cache parse failed
-      await this.redis_.del(cacheKey)
-    }
-
-    return null
   }
 
   async registerInstalledProviders(providers: string[]): Promise<void> {

packages/medusa/src/strategies/price-selection.ts

@@ -50,7 +50,6 @@ class PriceSelectionStrategy extends AbstractPriceSelectionStrategy {
     variant_id: string,
     context: PriceSelectionContext
   ): Promise<PriceSelectionResult> {
-    // TODO: Refactor using the cache decorators when it will be finished
     const cacheKey = this.getCacheKey(variant_id, context)
     const cached = await this.cacheService_
       .get<PriceSelectionResult>(cacheKey)