From 4d68d4d87d5e9c0e11bde693fd1d2c508dfe76fb Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 9 Mar 2021 15:31:54 +0100 Subject: [PATCH] core-api: identify ApiRefs based on ID instead of reference Co-authored-by: Juan Lulkin Signed-off-by: Patrik Oldsberg --- .../apis/system/ApiFactoryRegistry.test.ts | 21 +++ .../src/apis/system/ApiFactoryRegistry.ts | 14 +- .../src/apis/system/ApiRegistry.test.ts | 11 +- .../core-api/src/apis/system/ApiRegistry.ts | 14 +- .../src/apis/system/ApiResolver.test.ts | 153 ++++++++++++++---- .../core-api/src/apis/system/ApiResolver.ts | 10 +- 6 files changed, 172 insertions(+), 51 deletions(-) diff --git a/packages/core-api/src/apis/system/ApiFactoryRegistry.test.ts b/packages/core-api/src/apis/system/ApiFactoryRegistry.test.ts index 9f44adaef3..a51ed03fea 100644 --- a/packages/core-api/src/apis/system/ApiFactoryRegistry.test.ts +++ b/packages/core-api/src/apis/system/ApiFactoryRegistry.test.ts @@ -67,4 +67,25 @@ describe('ApiFactoryRegistry', () => { expect(registry.get(cRef)).toBe(cFactory); expect(registry.getAllApis()).toEqual(new Set([aRef, bRef, cRef])); }); + + it('should identify ApiRefs by id but still return the correct factory ref when listing all apis', () => { + const ref1 = createApiRef({ id: 'a', description: 'ref1' }); + const ref2 = createApiRef({ id: 'a', description: 'ref2' }); + + const factory1 = { api: ref1, deps: {}, factory: () => 3 }; + const factory2 = { api: ref2, deps: {}, factory: () => 3 }; + + const registry = new ApiFactoryRegistry(); + expect(registry.register('default', factory1)).toBe(true); + expect(registry.register('default', factory2)).toBe(false); + expect(registry.get(ref1)).toEqual(factory1); + expect(registry.get(ref2)).toEqual(factory1); + expect(registry.getAllApis()).toEqual(new Set([ref1])); + + expect(registry.register('app', factory2)).toBe(true); + expect(registry.get(ref1)).toEqual(factory2); + expect(registry.get(ref2)).toEqual(factory2); + expect(registry.getAllApis()).toEqual(new Set([ref2])); + expect(registry.getAllApis()).not.toEqual(new Set([ref1])); + }); }); diff --git a/packages/core-api/src/apis/system/ApiFactoryRegistry.ts b/packages/core-api/src/apis/system/ApiFactoryRegistry.ts index 9d9b058eac..c5a76ee1d4 100644 --- a/packages/core-api/src/apis/system/ApiFactoryRegistry.ts +++ b/packages/core-api/src/apis/system/ApiFactoryRegistry.ts @@ -46,7 +46,7 @@ type FactoryTuple = { * higher priority scopes override ones with lower priority. */ export class ApiFactoryRegistry implements ApiFactoryHolder { - private readonly factories = new Map(); + private readonly factories = new Map(); /** * Register a new API factory. Returns true if the factory was added @@ -60,19 +60,19 @@ export class ApiFactoryRegistry implements ApiFactoryHolder { factory: ApiFactory, ) { const priority = ScopePriority[scope]; - const existing = this.factories.get(factory.api); + const existing = this.factories.get(factory.api.id); if (existing && existing.priority >= priority) { return false; } - this.factories.set(factory.api, { priority, factory }); + this.factories.set(factory.api.id, { priority, factory }); return true; } get( api: ApiRef, ): ApiFactory | undefined { - const tuple = this.factories.get(api); + const tuple = this.factories.get(api.id); if (!tuple) { return undefined; } @@ -80,6 +80,10 @@ export class ApiFactoryRegistry implements ApiFactoryHolder { } getAllApis(): Set { - return new Set(this.factories.keys()); + const refs = new Set(); + for (const { factory } of this.factories.values()) { + refs.add(factory.api); + } + return refs; } } diff --git a/packages/core-api/src/apis/system/ApiRegistry.test.ts b/packages/core-api/src/apis/system/ApiRegistry.test.ts index a859d51004..93dd6dc085 100644 --- a/packages/core-api/src/apis/system/ApiRegistry.test.ts +++ b/packages/core-api/src/apis/system/ApiRegistry.test.ts @@ -18,8 +18,9 @@ import { ApiRegistry } from './ApiRegistry'; import { createApiRef } from './ApiRef'; describe('ApiRegistry', () => { - const x1Ref = createApiRef({ id: 'x', description: '' }); - const x2Ref = createApiRef({ id: 'x', description: '' }); + const x1Ref = createApiRef({ id: 'x1', description: '' }); + const x1DuplicateRef = createApiRef({ id: 'x1', description: '' }); + const x2Ref = createApiRef({ id: 'x2', description: '' }); it('should be created', () => { const registry = ApiRegistry.from([]); @@ -32,12 +33,14 @@ describe('ApiRegistry', () => { [x2Ref, 'y'], ]); expect(registry.get(x1Ref)).toBe(3); + expect(registry.get(x1DuplicateRef)).toBe(3); expect(registry.get(x2Ref)).toBe('y'); }); it('should be built', () => { const registry = ApiRegistry.builder().build(); expect(registry.get(x1Ref)).toBe(undefined); + expect(registry.get(x1DuplicateRef)).toBe(undefined); }); it('should be built with APIs', () => { @@ -47,6 +50,7 @@ describe('ApiRegistry', () => { const registry = builder.build(); expect(registry.get(x1Ref)).toBe(3); + expect(registry.get(x1DuplicateRef)).toBe(3); expect(registry.get(x2Ref)).toBe('y'); }); @@ -55,6 +59,7 @@ describe('ApiRegistry', () => { const reg2 = reg1.with(x2Ref, 'y'); const reg3 = reg2.with(x2Ref, 'z'); const reg4 = reg3.with(x1Ref, 2); + const reg5 = reg3.with(x1DuplicateRef, 4); expect(reg1.get(x1Ref)).toBe(3); expect(reg1.get(x2Ref)).toBe(undefined); @@ -64,5 +69,7 @@ describe('ApiRegistry', () => { expect(reg3.get(x2Ref)).toBe('z'); expect(reg4.get(x1Ref)).toBe(2); expect(reg4.get(x2Ref)).toBe('z'); + expect(reg5.get(x1Ref)).toBe(4); + expect(reg5.get(x2Ref)).toBe('z'); }); }); diff --git a/packages/core-api/src/apis/system/ApiRegistry.ts b/packages/core-api/src/apis/system/ApiRegistry.ts index b105633c51..01101b2b62 100644 --- a/packages/core-api/src/apis/system/ApiRegistry.ts +++ b/packages/core-api/src/apis/system/ApiRegistry.ts @@ -19,10 +19,10 @@ import { ApiRef, ApiHolder } from './types'; type ApiImpl = readonly [ApiRef, T]; class ApiRegistryBuilder { - private apis: ApiImpl[] = []; + private apis: [string, unknown][] = []; add(api: ApiRef, impl: I): I { - this.apis.push([api, impl]); + this.apis.push([api.id, impl]); return impl; } @@ -38,7 +38,7 @@ export class ApiRegistry implements ApiHolder { } static from(apis: ApiImpl[]) { - return new ApiRegistry(new Map(apis)); + return new ApiRegistry(new Map(apis.map(([api, impl]) => [api.id, impl]))); } /** @@ -48,10 +48,10 @@ export class ApiRegistry implements ApiHolder { * @param impl Implementation of the API to add */ static with(api: ApiRef, impl: T): ApiRegistry { - return new ApiRegistry(new Map([[api, impl]])); + return new ApiRegistry(new Map([[api.id, impl]])); } - constructor(private readonly apis: Map, unknown>) {} + constructor(private readonly apis: Map) {} /** * Returns a new ApiRegistry with the provided API added to the existing ones. @@ -60,10 +60,10 @@ export class ApiRegistry implements ApiHolder { * @param impl Implementation of the API to add */ with(api: ApiRef, impl: T): ApiRegistry { - return new ApiRegistry(new Map([...this.apis, [api, impl]])); + return new ApiRegistry(new Map([...this.apis, [api.id, impl]])); } get(api: ApiRef): T | undefined { - return this.apis.get(api) as T | undefined; + return this.apis.get(api.id) as T | undefined; } } diff --git a/packages/core-api/src/apis/system/ApiResolver.test.ts b/packages/core-api/src/apis/system/ApiResolver.test.ts index 3cd40f289e..7a46d2db3b 100644 --- a/packages/core-api/src/apis/system/ApiResolver.test.ts +++ b/packages/core-api/src/apis/system/ApiResolver.test.ts @@ -19,8 +19,14 @@ import { createApiRef } from './ApiRef'; import { ApiFactoryRegistry } from './ApiFactoryRegistry'; const aRef = createApiRef({ id: 'a', description: '' }); +const otherARef = createApiRef({ id: 'a', description: 'other' }); const bRef = createApiRef({ id: 'b', description: '' }); +const otherBRef = createApiRef({ id: 'b', description: 'other' }); const cRef = createApiRef<{ x: string }>({ id: 'c', description: '' }); +const otherCRef = createApiRef<{ x: string }>({ + id: 'c', + description: 'other', +}); function createRegistry() { const registry = new ApiFactoryRegistry(); @@ -36,28 +42,18 @@ function createRegistry() { }); registry.register('default', { api: cRef, - deps: { b: bRef }, + deps: { b: otherBRef }, factory: ({ b }) => ({ x: 'x', b }), }); return registry; } -function createLongCyclicRegistry() { +function createSelfCyclicRegistry() { const registry = new ApiFactoryRegistry(); registry.register('default', { api: aRef, - deps: { b: bRef }, - factory: () => 1, - }); - registry.register('default', { - api: bRef, - deps: { c: cRef }, - factory: () => 'b', - }); - registry.register('default', { - api: cRef, deps: { a: aRef }, - factory: () => ({ x: 'x' }), + factory: () => 1, }); return registry; } @@ -66,7 +62,37 @@ function createShortCyclicRegistry() { const registry = new ApiFactoryRegistry(); registry.register('default', { api: aRef, + deps: { b: bRef }, + factory: () => 1, + }); + registry.register('default', { + api: bRef, deps: { a: aRef }, + factory: () => 'x', + }); + return registry; +} + +function createShortCyclicRegistryWithOther() { + const registry = new ApiFactoryRegistry(); + registry.register('default', { + api: aRef, + deps: { b: bRef }, + factory: () => 1, + }); + registry.register('default', { + api: otherBRef, + deps: { a: otherARef }, + factory: () => 'x', + }); + return registry; +} + +function createLongCyclicRegistry() { + const registry = new ApiFactoryRegistry(); + registry.register('default', { + api: aRef, + deps: { b: otherBRef }, factory: () => 1, }); registry.register('default', { @@ -76,7 +102,7 @@ function createShortCyclicRegistry() { }); registry.register('default', { api: cRef, - deps: { b: bRef }, + deps: { a: aRef }, factory: () => ({ x: 'x' }), }); return registry; @@ -87,15 +113,51 @@ describe('ApiResolver', () => { const resolver = new ApiResolver(new ApiFactoryRegistry()); expect(resolver.get(aRef)).toBe(undefined); expect(resolver.get(bRef)).toBe(undefined); + expect(resolver.get(otherBRef)).toBe(undefined); expect(resolver.get(cRef)).toBe(undefined); }); it('should instantiate APIs', () => { const resolver = new ApiResolver(createRegistry()); expect(resolver.get(aRef)).toBe(1); + expect(resolver.get(otherARef)).toBe(1); expect(resolver.get(bRef)).toBe('b'); + expect(resolver.get(otherBRef)).toBe('b'); expect(resolver.get(cRef)).toEqual({ x: 'x', b: 'b' }); - expect(resolver.get(cRef)).toBe(resolver.get(cRef)); + expect(resolver.get(cRef)).toBe(resolver.get(otherCRef)); + }); + + it('should detect self dependency cycles', () => { + const resolver = new ApiResolver(createSelfCyclicRegistry()); + expect(() => resolver.get(aRef)).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + }); + + it('should detect short dependency cycles', () => { + const resolver = new ApiResolver(createShortCyclicRegistry()); + expect(() => resolver.get(aRef)).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + expect(() => resolver.get(bRef)).toThrow( + 'Circular dependency of api factory for apiRef{b}', + ); + }); + + it('should detect short dependency cycles with other refs', () => { + const resolver = new ApiResolver(createShortCyclicRegistryWithOther()); + expect(() => resolver.get(aRef)).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + expect(() => resolver.get(bRef)).toThrow( + 'Circular dependency of api factory for apiRef{b}', + ); + expect(() => resolver.get(otherARef)).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + expect(() => resolver.get(otherBRef)).toThrow( + 'Circular dependency of api factory for apiRef{b}', + ); }); it('should detect long dependency cycles', () => { @@ -110,17 +172,7 @@ describe('ApiResolver', () => { expect(() => resolver.get(bRef)).toThrow( 'Circular dependency of api factory for apiRef{b}', ); - expect(() => resolver.get(cRef)).toThrow( - 'Circular dependency of api factory for apiRef{c}', - ); - }); - - it('should detect short dependency cycles', () => { - const resolver = new ApiResolver(createShortCyclicRegistry()); - expect(() => resolver.get(aRef)).toThrow( - 'Circular dependency of api factory for apiRef{a}', - ); - expect(() => resolver.get(bRef)).toThrow( + expect(() => resolver.get(otherBRef)).toThrow( 'Circular dependency of api factory for apiRef{b}', ); expect(() => resolver.get(cRef)).toThrow( @@ -130,22 +182,54 @@ describe('ApiResolver', () => { it('should validate a factory holder', () => { expect(() => { - ApiResolver.validateFactories(createRegistry(), [aRef, bRef, cRef]); + ApiResolver.validateFactories(createRegistry(), [ + aRef, + bRef, + otherBRef, + cRef, + ]); }).not.toThrow(); }); + it('should find self cycles with validation', () => { + const self = createSelfCyclicRegistry(); + expect(() => ApiResolver.validateFactories(self, [aRef])).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + expect(() => ApiResolver.validateFactories(self, [otherARef])).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + }); + it('should find dependency cycles with validation', () => { const short = createShortCyclicRegistry(); - expect(() => - ApiResolver.validateFactories(short, short.getAllApis()), - ).toThrow('Circular dependency of api factory for apiRef{a}'); + expect(() => ApiResolver.validateFactories(short, [aRef])).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + expect(() => ApiResolver.validateFactories(short, [otherARef])).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); expect(() => ApiResolver.validateFactories(short, [bRef])).toThrow( 'Circular dependency of api factory for apiRef{b}', ); - expect(() => ApiResolver.validateFactories(short, [cRef])).toThrow( - 'Circular dependency of api factory for apiRef{c}', + expect(() => ApiResolver.validateFactories(short, [otherBRef])).toThrow( + 'Circular dependency of api factory for apiRef{b}', ); + const shortOther = createShortCyclicRegistryWithOther(); + expect(() => ApiResolver.validateFactories(shortOther, [aRef])).toThrow( + 'Circular dependency of api factory for apiRef{a}', + ); + expect(() => + ApiResolver.validateFactories(shortOther, [otherARef]), + ).toThrow('Circular dependency of api factory for apiRef{a}'); + expect(() => ApiResolver.validateFactories(shortOther, [bRef])).toThrow( + 'Circular dependency of api factory for apiRef{b}', + ); + expect(() => + ApiResolver.validateFactories(shortOther, [otherBRef]), + ).toThrow('Circular dependency of api factory for apiRef{b}'); + const long = createLongCyclicRegistry(); expect(() => ApiResolver.validateFactories(long, long.getAllApis()), @@ -153,6 +237,9 @@ describe('ApiResolver', () => { expect(() => ApiResolver.validateFactories(long, [bRef])).toThrow( 'Circular dependency of api factory for apiRef{b}', ); + expect(() => ApiResolver.validateFactories(long, [otherBRef])).toThrow( + 'Circular dependency of api factory for apiRef{b}', + ); expect(() => ApiResolver.validateFactories(long, [cRef])).toThrow( 'Circular dependency of api factory for apiRef{c}', ); @@ -173,5 +260,7 @@ describe('ApiResolver', () => { expect(factory).toHaveBeenCalledTimes(1); expect(resolver.get(aRef)).toBe(2); expect(factory).toHaveBeenCalledTimes(1); + expect(resolver.get(otherARef)).toBe(2); + expect(factory).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/core-api/src/apis/system/ApiResolver.ts b/packages/core-api/src/apis/system/ApiResolver.ts index 0dfeaeedd0..9738e09622 100644 --- a/packages/core-api/src/apis/system/ApiResolver.ts +++ b/packages/core-api/src/apis/system/ApiResolver.ts @@ -23,8 +23,6 @@ import { } from './types'; export class ApiResolver implements ApiHolder { - private readonly apis = new Map(); - /** * Validate factories by making sure that each of the apis can be created * without hitting any circular dependencies. @@ -45,7 +43,7 @@ export class ApiResolver implements ApiHolder { } for (const dep of Object.values(factory.deps)) { - if (dep === api) { + if (dep.id === api.id) { throw new Error(`Circular dependency of api factory for ${api}`); } if (!allDeps.has(dep)) { @@ -57,6 +55,8 @@ export class ApiResolver implements ApiHolder { } } + private readonly apis = new Map(); + constructor(private readonly factories: ApiFactoryHolder) {} get(ref: ApiRef): T | undefined { @@ -64,7 +64,7 @@ export class ApiResolver implements ApiHolder { } private load(ref: ApiRef, loading: AnyApiRef[] = []): T | undefined { - const impl = this.apis.get(ref); + const impl = this.apis.get(ref.id); if (impl) { return impl as T; } @@ -80,7 +80,7 @@ export class ApiResolver implements ApiHolder { const deps = this.loadDeps(ref, factory.deps, [...loading, factory.api]); const api = factory.factory(deps); - this.apis.set(ref, api); + this.apis.set(ref.id, api); return api as T; }