From d1393b058134724fd1299605c463f9c0526dd82c Mon Sep 17 00:00:00 2001 From: vsavkin Date: Wed, 1 Jul 2015 16:06:15 -0700 Subject: [PATCH] fix(di): injecting null causes a cyclic dependency --- modules/angular2/src/di/injector.ts | 97 ++--- modules/angular2/test/di/injector_spec.ts | 417 ++++++++++++---------- 2 files changed, 285 insertions(+), 229 deletions(-) diff --git a/modules/angular2/src/di/injector.ts b/modules/angular2/src/di/injector.ts index c337d7581a..b20417278b 100644 --- a/modules/angular2/src/di/injector.ts +++ b/modules/angular2/src/di/injector.ts @@ -197,16 +197,16 @@ export interface InjectorStrategy { } export class InjectorInlineStrategy implements InjectorStrategy { - obj0: any = null; - obj1: any = null; - obj2: any = null; - obj3: any = null; - obj4: any = null; - obj5: any = null; - obj6: any = null; - obj7: any = null; - obj8: any = null; - obj9: any = null; + obj0: any = undefinedValue; + obj1: any = undefinedValue; + obj2: any = undefinedValue; + obj3: any = undefinedValue; + obj4: any = undefinedValue; + obj5: any = undefinedValue; + obj6: any = undefinedValue; + obj7: any = undefinedValue; + obj8: any = undefinedValue; + obj9: any = undefinedValue; constructor(public injector: Injector, public protoStrategy: ProtoInjectorInlineStrategy) {} @@ -217,16 +217,26 @@ export class InjectorInlineStrategy implements InjectorStrategy { inj._constructionCounter = 0; - if (isPresent(p.keyId0) && isBlank(this.obj0)) this.obj0 = inj._new(p.binding0, p.visibility0); - if (isPresent(p.keyId1) && isBlank(this.obj1)) this.obj1 = inj._new(p.binding1, p.visibility1); - if (isPresent(p.keyId2) && isBlank(this.obj2)) this.obj2 = inj._new(p.binding2, p.visibility2); - if (isPresent(p.keyId3) && isBlank(this.obj3)) this.obj3 = inj._new(p.binding3, p.visibility3); - if (isPresent(p.keyId4) && isBlank(this.obj4)) this.obj4 = inj._new(p.binding4, p.visibility4); - if (isPresent(p.keyId5) && isBlank(this.obj5)) this.obj5 = inj._new(p.binding5, p.visibility5); - if (isPresent(p.keyId6) && isBlank(this.obj6)) this.obj6 = inj._new(p.binding6, p.visibility6); - if (isPresent(p.keyId7) && isBlank(this.obj7)) this.obj7 = inj._new(p.binding7, p.visibility7); - if (isPresent(p.keyId8) && isBlank(this.obj8)) this.obj8 = inj._new(p.binding8, p.visibility8); - if (isPresent(p.keyId9) && isBlank(this.obj9)) this.obj9 = inj._new(p.binding9, p.visibility9); + if (isPresent(p.keyId0) && this.obj0 === undefinedValue) + this.obj0 = inj._new(p.binding0, p.visibility0); + if (isPresent(p.keyId1) && this.obj1 === undefinedValue) + this.obj1 = inj._new(p.binding1, p.visibility1); + if (isPresent(p.keyId2) && this.obj2 === undefinedValue) + this.obj2 = inj._new(p.binding2, p.visibility2); + if (isPresent(p.keyId3) && this.obj3 === undefinedValue) + this.obj3 = inj._new(p.binding3, p.visibility3); + if (isPresent(p.keyId4) && this.obj4 === undefinedValue) + this.obj4 = inj._new(p.binding4, p.visibility4); + if (isPresent(p.keyId5) && this.obj5 === undefinedValue) + this.obj5 = inj._new(p.binding5, p.visibility5); + if (isPresent(p.keyId6) && this.obj6 === undefinedValue) + this.obj6 = inj._new(p.binding6, p.visibility6); + if (isPresent(p.keyId7) && this.obj7 === undefinedValue) + this.obj7 = inj._new(p.binding7, p.visibility7); + if (isPresent(p.keyId8) && this.obj8 === undefinedValue) + this.obj8 = inj._new(p.binding8, p.visibility8); + if (isPresent(p.keyId9) && this.obj9 === undefinedValue) + this.obj9 = inj._new(p.binding9, p.visibility9); } attach(parent: Injector, isBoundary: boolean): void { @@ -236,16 +246,16 @@ export class InjectorInlineStrategy implements InjectorStrategy { } dehydrate() { - this.obj0 = null; - this.obj1 = null; - this.obj2 = null; - this.obj3 = null; - this.obj4 = null; - this.obj5 = null; - this.obj6 = null; - this.obj7 = null; - this.obj8 = null; - this.obj9 = null; + this.obj0 = undefinedValue; + this.obj1 = undefinedValue; + this.obj2 = undefinedValue; + this.obj3 = undefinedValue; + this.obj4 = undefinedValue; + this.obj5 = undefinedValue; + this.obj6 = undefinedValue; + this.obj7 = undefinedValue; + this.obj8 = undefinedValue; + this.obj9 = undefinedValue; } getObjByKeyId(keyId: number, visibility: number): any { @@ -253,61 +263,61 @@ export class InjectorInlineStrategy implements InjectorStrategy { var inj = this.injector; if (p.keyId0 === keyId && (p.visibility0 & visibility) > 0) { - if (isBlank(this.obj0)) { + if (this.obj0 === undefinedValue) { this.obj0 = inj._new(p.binding0, p.visibility0); } return this.obj0; } if (p.keyId1 === keyId && (p.visibility1 & visibility) > 0) { - if (isBlank(this.obj1)) { + if (this.obj1 === undefinedValue) { this.obj1 = inj._new(p.binding1, p.visibility1); } return this.obj1; } if (p.keyId2 === keyId && (p.visibility2 & visibility) > 0) { - if (isBlank(this.obj2)) { + if (this.obj2 === undefinedValue) { this.obj2 = inj._new(p.binding2, p.visibility2); } return this.obj2; } if (p.keyId3 === keyId && (p.visibility3 & visibility) > 0) { - if (isBlank(this.obj3)) { + if (this.obj3 === undefinedValue) { this.obj3 = inj._new(p.binding3, p.visibility3); } return this.obj3; } if (p.keyId4 === keyId && (p.visibility4 & visibility) > 0) { - if (isBlank(this.obj4)) { + if (this.obj4 === undefinedValue) { this.obj4 = inj._new(p.binding4, p.visibility4); } return this.obj4; } if (p.keyId5 === keyId && (p.visibility5 & visibility) > 0) { - if (isBlank(this.obj5)) { + if (this.obj5 === undefinedValue) { this.obj5 = inj._new(p.binding5, p.visibility5); } return this.obj5; } if (p.keyId6 === keyId && (p.visibility6 & visibility) > 0) { - if (isBlank(this.obj6)) { + if (this.obj6 === undefinedValue) { this.obj6 = inj._new(p.binding6, p.visibility6); } return this.obj6; } if (p.keyId7 === keyId && (p.visibility7 & visibility) > 0) { - if (isBlank(this.obj7)) { + if (this.obj7 === undefinedValue) { this.obj7 = inj._new(p.binding7, p.visibility7); } return this.obj7; } if (p.keyId8 === keyId && (p.visibility8 & visibility) > 0) { - if (isBlank(this.obj8)) { + if (this.obj8 === undefinedValue) { this.obj8 = inj._new(p.binding8, p.visibility8); } return this.obj8; } if (p.keyId9 === keyId && (p.visibility9 & visibility) > 0) { - if (isBlank(this.obj9)) { + if (this.obj9 === undefinedValue) { this.obj9 = inj._new(p.binding9, p.visibility9); } return this.obj9; @@ -339,12 +349,13 @@ export class InjectorDynamicStrategy implements InjectorStrategy { constructor(public protoStrategy: ProtoInjectorDynamicStrategy, public injector: Injector) { this.objs = ListWrapper.createFixedSize(protoStrategy.bindings.length); + ListWrapper.fill(this.objs, undefinedValue); } hydrate(): void { var p = this.protoStrategy; for (var i = 0; i < p.keyIds.length; i++) { - if (isPresent(p.keyIds[i]) && isBlank(this.objs[i])) { + if (isPresent(p.keyIds[i]) && this.objs[i] === undefinedValue) { this.objs[i] = this.injector._new(p.bindings[i], p.visibilities[i]); } } @@ -356,14 +367,14 @@ export class InjectorDynamicStrategy implements InjectorStrategy { inj._isBoundary = isBoundary; } - dehydrate(): void { ListWrapper.fill(this.objs, null); } + dehydrate(): void { ListWrapper.fill(this.objs, undefinedValue); } getObjByKeyId(keyId: number, visibility: number): any { var p = this.protoStrategy; for (var i = 0; i < p.keyIds.length; i++) { if (p.keyIds[i] === keyId && (p.visibilities[i] & visibility) > 0) { - if (isBlank(this.objs[i])) { + if (this.objs[i] === undefinedValue) { this.objs[i] = this.injector._new(p.bindings[i], p.visibilities[i]); } diff --git a/modules/angular2/test/di/injector_spec.ts b/modules/angular2/test/di/injector_spec.ts index 4921b6f312..0035e15730 100644 --- a/modules/angular2/test/di/injector_spec.ts +++ b/modules/angular2/test/di/injector_spec.ts @@ -9,6 +9,9 @@ import { DependencyAnnotation, Injectable } from 'angular2/di'; + +import {InjectorInlineStrategy, InjectorDynamicStrategy} from 'angular2/src/di/injector'; + import {Optional, Inject} from 'angular2/src/di/decorators'; import * as ann from 'angular2/src/di/annotations_impl'; @@ -82,196 +85,238 @@ class NoAnnotations { } export function main() { - describe('injector', () => { + var dynamicBindings = [ + bind('binding0') + .toValue(1), + bind('binding1').toValue(1), + bind('binding2').toValue(1), + bind('binding3').toValue(1), + bind('binding4').toValue(1), + bind('binding5').toValue(1), + bind('binding6').toValue(1), + bind('binding7').toValue(1), + bind('binding8').toValue(1), + bind('binding9').toValue(1), + bind('binding10').toValue(1) + ]; - it('should instantiate a class without dependencies', () => { - var injector = Injector.resolveAndCreate([Engine]); - var engine = injector.get(Engine); + [{strategy: 'inline', bindings: [], strategyClass: InjectorInlineStrategy}, + { + strategy: 'dynamic', + bindings: dynamicBindings, + strategyClass: InjectorDynamicStrategy + }].forEach((context) => { - expect(engine).toBeAnInstanceOf(Engine); + function createInjector(bindings: any[]) { + return Injector.resolveAndCreate(bindings.concat(context['bindings'])); + } + + describe(`injector ${context['strategy']}`, () => { + it("should use the right strategy", () => { + var injector = createInjector([]); + expect(injector.internalStrategy).toBeAnInstanceOf(context['strategyClass']); + }); + + it('should instantiate a class without dependencies', () => { + var injector = createInjector([Engine]); + var engine = injector.get(Engine); + + expect(engine).toBeAnInstanceOf(Engine); + }); + + it('should resolve dependencies based on type information', () => { + var injector = createInjector([Engine, Car]); + var car = injector.get(Car); + + expect(car).toBeAnInstanceOf(Car); + expect(car.engine).toBeAnInstanceOf(Engine); + }); + + it('should resolve dependencies based on @Inject annotation', () => { + var injector = createInjector([TurboEngine, Engine, CarWithInject]); + var car = injector.get(CarWithInject); + + expect(car).toBeAnInstanceOf(CarWithInject); + expect(car.engine).toBeAnInstanceOf(TurboEngine); + }); + + it('should throw when no type and not @Inject', () => { + expect(() => createInjector([NoAnnotations])) + .toThrowError('Cannot resolve all parameters for NoAnnotations(?). ' + + 'Make sure they all have valid type or annotations.'); + }); + + it('should cache instances', () => { + var injector = createInjector([Engine]); + + var e1 = injector.get(Engine); + var e2 = injector.get(Engine); + + expect(e1).toBe(e2); + }); + + it('should bind to a value', () => { + var injector = createInjector([bind(Engine).toValue("fake engine")]); + + var engine = injector.get(Engine); + expect(engine).toEqual("fake engine"); + }); + + it('should bind to a factory', () => { + function sportsCarFactory(e) { return new SportsCar(e); } + + var injector = createInjector([Engine, bind(Car).toFactory(sportsCarFactory, [Engine])]); + + var car = injector.get(Car); + expect(car).toBeAnInstanceOf(SportsCar); + expect(car.engine).toBeAnInstanceOf(Engine); + }); + + it('should supporting binding to null', () => { + var injector = createInjector([bind(Engine).toValue(null)]); + + for (var i = 0; i < 20; ++i) { + injector.get(Engine); + } + + var engine = injector.get(Engine); + expect(engine).toBeNull(); + }); + + it('should bind to an alias', () => { + var injector = createInjector( + [Engine, bind(SportsCar).toClass(SportsCar), bind(Car).toAlias(SportsCar)]); + + var car = injector.get(Car); + var sportsCar = injector.get(SportsCar); + expect(car).toBeAnInstanceOf(SportsCar); + expect(car).toBe(sportsCar); + }); + + it('should throw when the aliased binding does not exist', () => { + var injector = createInjector([bind('car').toAlias(SportsCar)]); + var e = `No provider for ${stringify(SportsCar)}! (car -> ${stringify(SportsCar)})`; + expect(() => injector.get('car')).toThrowError(e); + }); + + it('should throw with a meaningful message when the aliased binding is blank', () => { + expect(() => bind('car').toAlias(null)).toThrowError('Can not alias car to a blank value!'); + }); + + it('should handle forwardRef in toAlias', () => { + var injector = createInjector([ + bind('originalEngine') + .toClass(forwardRef(() => Engine)), + bind('aliasedEngine').toAlias(forwardRef(() => 'originalEngine')) + ]); + expect(injector.get('aliasedEngine')).toBeAnInstanceOf(Engine); + }); + + it('should support overriding factory dependencies', () => { + var injector = + createInjector([Engine, bind(Car).toFactory((e) => new SportsCar(e), [Engine])]); + + var car = injector.get(Car); + expect(car).toBeAnInstanceOf(SportsCar); + expect(car.engine).toBeAnInstanceOf(Engine); + }); + + it('should support optional dependencies', () => { + var injector = createInjector([CarWithOptionalEngine]); + + var car = injector.get(CarWithOptionalEngine); + expect(car.engine).toEqual(null); + }); + + it("should flatten passed-in bindings", () => { + var injector = createInjector([[[Engine, Car]]]); + + var car = injector.get(Car); + expect(car).toBeAnInstanceOf(Car); + }); + + it("should use the last binding when there are multiple bindings for same token", () => { + var injector = + createInjector([bind(Engine).toClass(Engine), bind(Engine).toClass(TurboEngine)]); + + expect(injector.get(Engine)).toBeAnInstanceOf(TurboEngine); + }); + + it('should use non-type tokens', () => { + var injector = createInjector([bind('token').toValue('value')]); + + expect(injector.get('token')).toEqual('value'); + }); + + it('should throw when given invalid bindings', () => { + expect(() => createInjector(["blah"])) + .toThrowError( + 'Invalid binding - only instances of Binding and Type are allowed, got: blah'); + expect(() => createInjector([bind("blah")])) + .toThrowError('Invalid binding - only instances of Binding and Type are allowed, ' + + 'got: blah'); + }); + + it('should provide itself', () => { + var parent = createInjector([]); + var child = parent.resolveAndCreateChild([]); + + expect(child.get(Injector)).toBe(child); + }); + + it('should throw when no provider defined', () => { + var injector = createInjector([]); + expect(() => injector.get('NonExisting')).toThrowError('No provider for NonExisting!'); + }); + + it('should show the full path when no provider', () => { + var injector = createInjector([CarWithDashboard, Engine, Dashboard]); + expect(() => injector.get(CarWithDashboard)) + .toThrowError( + `No provider for DashboardSoftware! (${stringify(CarWithDashboard)} -> ${stringify(Dashboard)} -> DashboardSoftware)`); + }); + + it('should throw when trying to instantiate a cyclic dependency', () => { + var injector = createInjector([Car, bind(Engine).toClass(CyclicEngine)]); + + expect(() => injector.get(Car)) + .toThrowError( + `Cannot instantiate cyclic dependency! (${stringify(Car)} -> ${stringify(Engine)} -> ${stringify(Car)})`); + }); + + it('should show the full path when error happens in a constructor', () => { + var injector = createInjector([Car, bind(Engine).toClass(BrokenEngine)]); + + try { + injector.get(Car); + throw "Must throw"; + } catch (e) { + expect(e.message) + .toContain(`Error during instantiation of Engine! (${stringify(Car)} -> Engine)`); + expect(e.originalException instanceof BaseException).toBeTruthy(); + expect(e.causeKey.token).toEqual(Engine); + } + }); + + it('should instantiate an object after a failed attempt', () => { + var isBroken = true; + + var injector = createInjector( + [Car, bind(Engine).toFactory(() => isBroken ? new BrokenEngine() : new Engine())]); + + expect(() => injector.get(Car)).toThrowError(new RegExp("Error")); + + isBroken = false; + + expect(injector.get(Car)).toBeAnInstanceOf(Car); + }); + + it('should support null values', () => { + var injector = createInjector([bind('null').toValue(null)]); + expect(injector.get('null')).toBe(null); + }); }); - it('should resolve dependencies based on type information', () => { - var injector = Injector.resolveAndCreate([Engine, Car]); - var car = injector.get(Car); - - expect(car).toBeAnInstanceOf(Car); - expect(car.engine).toBeAnInstanceOf(Engine); - }); - - it('should resolve dependencies based on @Inject annotation', () => { - var injector = Injector.resolveAndCreate([TurboEngine, Engine, CarWithInject]); - var car = injector.get(CarWithInject); - - expect(car).toBeAnInstanceOf(CarWithInject); - expect(car.engine).toBeAnInstanceOf(TurboEngine); - }); - - it('should throw when no type and not @Inject', () => { - expect(() => Injector.resolveAndCreate([NoAnnotations])) - .toThrowError('Cannot resolve all parameters for NoAnnotations(?). ' + - 'Make sure they all have valid type or annotations.'); - }); - - it('should cache instances', () => { - var injector = Injector.resolveAndCreate([Engine]); - - var e1 = injector.get(Engine); - var e2 = injector.get(Engine); - - expect(e1).toBe(e2); - }); - - it('should bind to a value', () => { - var injector = Injector.resolveAndCreate([bind(Engine).toValue("fake engine")]); - - var engine = injector.get(Engine); - expect(engine).toEqual("fake engine"); - }); - - it('should bind to a factory', () => { - function sportsCarFactory(e) { return new SportsCar(e); } - - var injector = - Injector.resolveAndCreate([Engine, bind(Car).toFactory(sportsCarFactory, [Engine])]); - - var car = injector.get(Car); - expect(car).toBeAnInstanceOf(SportsCar); - expect(car.engine).toBeAnInstanceOf(Engine); - }); - - it('should bind to an alias', () => { - var injector = Injector.resolveAndCreate( - [Engine, bind(SportsCar).toClass(SportsCar), bind(Car).toAlias(SportsCar)]); - - var car = injector.get(Car); - var sportsCar = injector.get(SportsCar); - expect(car).toBeAnInstanceOf(SportsCar); - expect(car).toBe(sportsCar); - }); - - it('should throw when the aliased binding does not exist', () => { - var injector = Injector.resolveAndCreate([bind('car').toAlias(SportsCar)]); - var e = `No provider for ${stringify(SportsCar)}! (car -> ${stringify(SportsCar)})`; - expect(() => injector.get('car')).toThrowError(e); - }); - - it('should throw with a meaningful message when the aliased binding is blank', () => { - expect(() => bind('car').toAlias(null)).toThrowError('Can not alias car to a blank value!'); - }); - - it('should handle forwardRef in toAlias', () => { - var injector = Injector.resolveAndCreate([ - bind('originalEngine') - .toClass(forwardRef(() => Engine)), - bind('aliasedEngine').toAlias(forwardRef(() => 'originalEngine')) - ]); - expect(injector.get('aliasedEngine')).toBeAnInstanceOf(Engine); - }); - - it('should support overriding factory dependencies', () => { - var injector = Injector.resolveAndCreate( - [Engine, bind(Car).toFactory((e) => new SportsCar(e), [Engine])]); - - var car = injector.get(Car); - expect(car).toBeAnInstanceOf(SportsCar); - expect(car.engine).toBeAnInstanceOf(Engine); - }); - - it('should support optional dependencies', () => { - var injector = Injector.resolveAndCreate([CarWithOptionalEngine]); - - var car = injector.get(CarWithOptionalEngine); - expect(car.engine).toEqual(null); - }); - - it("should flatten passed-in bindings", () => { - var injector = Injector.resolveAndCreate([[[Engine, Car]]]); - - var car = injector.get(Car); - expect(car).toBeAnInstanceOf(Car); - }); - - it("should use the last binding when there are multiple bindings for same token", () => { - var injector = Injector.resolveAndCreate( - [bind(Engine).toClass(Engine), bind(Engine).toClass(TurboEngine)]); - - expect(injector.get(Engine)).toBeAnInstanceOf(TurboEngine); - }); - - it('should use non-type tokens', () => { - var injector = Injector.resolveAndCreate([bind('token').toValue('value')]); - - expect(injector.get('token')).toEqual('value'); - }); - - it('should throw when given invalid bindings', () => { - expect(() => Injector.resolveAndCreate(["blah"])) - .toThrowError( - 'Invalid binding - only instances of Binding and Type are allowed, got: blah'); - expect(() => Injector.resolveAndCreate([bind("blah")])) - .toThrowError('Invalid binding - only instances of Binding and Type are allowed, ' + - 'got: blah'); - }); - - it('should provide itself', () => { - var parent = Injector.resolveAndCreate([]); - var child = parent.resolveAndCreateChild([]); - - expect(child.get(Injector)).toBe(child); - }); - - it('should throw when no provider defined', () => { - var injector = Injector.resolveAndCreate([]); - expect(() => injector.get('NonExisting')).toThrowError('No provider for NonExisting!'); - }); - - it('should show the full path when no provider', () => { - var injector = Injector.resolveAndCreate([CarWithDashboard, Engine, Dashboard]); - expect(() => injector.get(CarWithDashboard)) - .toThrowError( - `No provider for DashboardSoftware! (${stringify(CarWithDashboard)} -> ${stringify(Dashboard)} -> DashboardSoftware)`); - }); - - it('should throw when trying to instantiate a cyclic dependency', () => { - var injector = Injector.resolveAndCreate([Car, bind(Engine).toClass(CyclicEngine)]); - - expect(() => injector.get(Car)) - .toThrowError( - `Cannot instantiate cyclic dependency! (${stringify(Car)} -> ${stringify(Engine)} -> ${stringify(Car)})`); - }); - - it('should show the full path when error happens in a constructor', () => { - var injector = Injector.resolveAndCreate([Car, bind(Engine).toClass(BrokenEngine)]); - - try { - injector.get(Car); - throw "Must throw"; - } catch (e) { - expect(e.message) - .toContain(`Error during instantiation of Engine! (${stringify(Car)} -> Engine)`); - expect(e.originalException instanceof BaseException).toBeTruthy(); - expect(e.causeKey.token).toEqual(Engine); - } - }); - - it('should instantiate an object after a failed attempt', () => { - var isBroken = true; - - var injector = Injector.resolveAndCreate( - [Car, bind(Engine).toFactory(() => isBroken ? new BrokenEngine() : new Engine())]); - - expect(() => injector.get(Car)).toThrowError(new RegExp("Error")); - - isBroken = false; - - expect(injector.get(Car)).toBeAnInstanceOf(Car); - }); - - it('should support null values', () => { - var injector = Injector.resolveAndCreate([bind('null').toValue(null)]); - expect(injector.get('null')).toBe(null); - }); describe("child", () => { it('should load instances from parent injector', () => {