diff --git a/.changeset/four-bugs-mate.md b/.changeset/four-bugs-mate.md new file mode 100644 index 0000000000..10c4fffd16 --- /dev/null +++ b/.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 diff --git a/packages/medusa/src/services/__fixtures__/tax-provider.ts b/packages/medusa/src/services/__fixtures__/tax-provider.ts new file mode 100644 index 0000000000..5e437feaa3 --- /dev/null +++ b/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 } +} diff --git a/packages/medusa/src/services/__tests__/tax-provider.js b/packages/medusa/src/services/__tests__/tax-provider.js index 960da08888..2dab0afda6 100644 --- a/packages/medusa/src/services/__tests__/tax-provider.js +++ b/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", () => { + afterEach(() => { + jest.clearAllMocks() + }) + describe("retrieveProvider", () => { - const container = { - manager: MockManager, - taxRateService: {}, - systemTaxService: "system", - tp_test: "good", - } - - const providerService = new TaxProviderService(container) + 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 = { - manager: MockManager, - lineItemTaxLineRepository: MockRepository({ create: (d) => d }), - systemTaxService: { - getTaxLines: mockCalculateLineItemTaxes, - }, - } + const container = createContainer({}, defaultContainer) + container.register("systemTaxService", asValue({ + 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 = { - manager: MockManager, - taxRateService: { - listByProduct: jest.fn(), + const container = createContainer({}, defaultContainer) + container.register("taxRateService", asValue({ + withTransaction: function () { + return this }, - } + listByProduct: jest.fn(() => Promise.resolve([])), + })) test("success", async () => { - container.taxRateService = { - withTransaction: function () { - return this - }, - listByProduct: jest.fn(() => Promise.resolve([])), - } - - const providerService = new TaxProviderService(container) - providerService.getCacheEntry = jest.fn(() => null) - providerService.setCache = jest.fn() + const providerService = container.resolve("taxProviderService") 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) - expect(providerService.getCacheEntry).toHaveBeenCalledWith( - "prod_id", - "reg_id" - ) + const cacheService = container.resolve("cacheService") + const cacheKey = getCacheKey("prod_id", "reg_id") - expect(providerService.setCache).toHaveBeenCalledTimes(1) - expect(providerService.setCache).toHaveBeenCalledWith( - "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 ) }) test("success - without product rates", async () => { - container.taxRateService = { - withTransaction: function () { - return this - }, - listByProduct: jest.fn(() => Promise.resolve([])), - } - - const providerService = new TaxProviderService(container) - providerService.getCacheEntry = jest.fn(() => null) - providerService.setCache = jest.fn() + const providerService = container.resolve("taxProviderService") 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) - expect(providerService.setCache).toHaveBeenCalledWith( - "prod_id", - "reg_id", + const cacheService = container.resolve("cacheService") + const cacheKey = getCacheKey("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) - providerService.getCacheEntry = jest.fn(() => null) - providerService.setCache = jest.fn() + const providerService = container.resolve("taxProviderService") 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) - expect(providerService.setCache).toHaveBeenCalledWith( - "prod_id", - "reg_id", + const cacheService = container.resolve("cacheService") + const cacheKey = getCacheKey("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 = { - manager: MockManager, - productTaxRateService: {}, - systemTaxService: "system", - tp_test: "good", - } - - const providerService = new TaxProviderService(container) + const providerService = defaultContainer.resolve("taxProviderService") 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 } -} diff --git a/packages/medusa/src/services/tax-provider.ts b/packages/medusa/src/services/tax-provider.ts index 935bc1c076..f7f3adea79 100644 --- a/packages/medusa/src/services/tax-provider.ts +++ b/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 { @@ -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 { - const cacheHit = await this.getCacheEntry(optionId, regionDetails.id) + const cacheKey = this.getCacheKey(optionId, regionDetails.id) + const cacheHit = await this.cacheService_.get(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 { - const cacheHit = await this.getCacheEntry(productId, region.id) + const cacheKey = this.getCacheKey(productId, region.id) + const cacheHit = await this.cacheService_.get(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 { - return `txrtcache:${productId}:${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 { - 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 { - 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 + private getCacheKey(id: string, regionId: string): string { + return `txrtcache:${id}:${regionId}` } async registerInstalledProviders(providers: string[]): Promise { diff --git a/packages/medusa/src/strategies/price-selection.ts b/packages/medusa/src/strategies/price-selection.ts index 94a4fc6e20..490e83f24b 100644 --- a/packages/medusa/src/strategies/price-selection.ts +++ b/packages/medusa/src/strategies/price-selection.ts @@ -50,7 +50,6 @@ class PriceSelectionStrategy extends AbstractPriceSelectionStrategy { variant_id: string, context: PriceSelectionContext ): Promise { - // TODO: Refactor using the cache decorators when it will be finished const cacheKey = this.getCacheKey(variant_id, context) const cached = await this.cacheService_ .get(cacheKey)