Fix tricky trait issue in fb 968

The issue as described in [factory_bot #968][url] is that depending on the order
a parent or child factory is initialized, it will copy over the other's traits
in a way that leads to inconsistent results.

[url]: thoughtbot/factory_bot#968

The fix for us (and hopefully for factory_bot, if I can present this properly)
is to make the "search" for a trait happen every time instead of at compile, and
to stop copying any traits at all into each other. Instead, keep track of the
"parent" (which is effectively a lexical context) inside of each trait, and when
calling "getAttributes" on a trait, have it check up the parent hierarchy for
the existence of a trait with that name.

To handle this, we have to copy over the "parent" of each trait when iterating
over them. This is slightly messy and not my favorite means of handling things,
but because it happens unconditionally, it's okay.

NOTE: You have to delete or move all of the unit tests to run anything, because
typescript is dumb.

lib/definition-proxy.ts

@@ -33,7 +33,8 @@ export class DefinitionProxy {
 
 	execute(): void {
 		if (this.definition.block) {
-			this.definition.block.call(addMethodMissing(this), addMethodMissing(this));
+			const wrappedThis = addMethodMissing(this);
+			this.definition.block.call(wrappedThis, wrappedThis);
 		}
 	}
 
@@ -93,11 +94,8 @@ export class DefinitionProxy {
 	}
 
 	trait(name: string, block?: blockFunction): void {
-		if (block && isFunction(block)) {
-			this.definition.defineTrait(new Trait(name, this.fixtureRiveter, block));
-		} else {
-			throw new Error(`wrong options, bruh: ${name}, ${block}`);
-		}
+		const newTrait = new Trait(name, this.fixtureRiveter, block);
+		this.definition.defineTrait(newTrait);
 	}
 
 	transient(block: blockFunction): void {

lib/definition.ts

@@ -16,7 +16,6 @@ export class Definition {
 	name: string;
 	aliases: string[];
 	attributes: Attribute[];
-	definedTraits: Trait[];
 	baseTraits: string[];
 	additionalTraits: string[];
 	traitsCache?: Record<string, Trait>;
@@ -27,20 +26,23 @@ export class Definition {
 	sequenceHandler: SequenceHandler;
 	declarationHandler: DeclarationHandler;
 
+	traits: any;
+
 	constructor(name: string, fixtureRiveter: FixtureRiveter) {
 		this.name = name;
 		this.fixtureRiveter = fixtureRiveter;
 
 		this.aliases = [];
 		this.attributes = [];
-		this.definedTraits = [];
 		this.baseTraits = [];
 		this.additionalTraits = [];
 		this.compiled = false;
 
 		this.sequenceHandler = new SequenceHandler();
 		this.declarationHandler = new DeclarationHandler(name);
 		this.callbackHandler = new CallbackHandler(fixtureRiveter);
+
+		this.traits = {};
 	}
 
 	names(): string[] {
@@ -49,17 +51,7 @@ export class Definition {
 
 	compile(): void {
 		if (!this.compiled) {
-			// Generates the list of attributes
 			this.declarationHandler.getAttributes();
-
-			for (const definedTrait of this.definedTraits) {
-				for (const baseTrait of this.getBaseTraits()) {
-					baseTrait.defineTrait(definedTrait);
-				}
-				for (const additionalTraits of this.getAdditionalTraits()) {
-					additionalTraits.defineTrait(definedTrait);
-				}
-			}
 			this.compiled = true;
 		}
 	}
@@ -73,8 +65,8 @@ export class Definition {
 	}
 
 	defineTrait(newTrait: Trait): void {
-		if (!this.definedTraits.map((t) => t.name).includes(newTrait.name)) {
-			this.definedTraits.push(newTrait);
+		if (!this.traits[newTrait.name]) {
+			this.traits[newTrait.name] = newTrait;
 		}
 	}
 
@@ -86,24 +78,10 @@ export class Definition {
 		this.baseTraits = this.baseTraits.concat(traits);
 	}
 
-	populateTraitsCache(): Record<string, Trait> {
-		if (!this.traitsCache || Object.keys(this.traitsCache).length === 0) {
-			const cache = {};
-			for (const trait of this.definedTraits) {
-				cache[trait.name] = trait;
-			}
-			this.traitsCache = cache;
-		}
-		return this.traitsCache;
-	}
-
-	traitFor(name: string): Trait | undefined {
-		const traitsCache = this.populateTraitsCache();
-		return traitsCache[name];
-	}
-
 	traitByName(name: string): Trait {
-		return this.traitFor(name) || this.fixtureRiveter.getTrait(name);
+		// This is overridden by both Fixture and Trait, so ignore it please
+		// I've only written this out so Typescript will shut up lol
+		return this.traits[name] || this.fixtureRiveter.getTrait(name);
 	}
 
 	getInternalTraits(internalTraits: string[]): Trait[] {
@@ -132,16 +110,6 @@ export class Definition {
 		].flat(Infinity).filter(Boolean);
 	}
 
-	getAttributes(): Attribute[] {
-		if (!this.attributes || this.attributes.length === 0) {
-			this.attributes = this.aggregateFromTraitsAndSelf(
-				"getAttributes",
-				() => this.declarationHandler.getAttributes(),
-			);
-		}
-		return this.attributes;
-	}
-
 	copy<T>(): T {
 		const copy = Object.assign(Object.create(Object.getPrototypeOf(this)), this);
 

lib/fixture-riveter.ts

@@ -115,16 +115,9 @@ export class FixtureRiveter {
 		return trait;
 	}
 
-	registerTrait(trait: Trait): void {
-		for (const name of trait.names()) {
-			this.traits[name] = trait;
-		}
-	}
-
-	trait(name: string, block?: blockFunction): Trait {
+	trait(name: string, block: blockFunction): void {
 		const trait = new Trait(name, this, block);
-		this.registerTrait(trait);
-		return trait;
+		this.traits[trait.name] = trait;
 	}
 
 	sequence(

lib/fixture.ts

@@ -12,6 +12,7 @@ import {
 	fixtureOptionsParser,
 } from "./fixture-options-parser";
 import {Strategy} from "./strategies/strategy";
+import {Trait} from "./trait";
 
 import {isFunction} from "lodash";
 import {Callback} from "./callback";
@@ -77,32 +78,35 @@ export class Fixture extends Definition {
 
 	compile(): void {
 		if (!this.compiled) {
-			const parentFixture = this.parentFixture();
-			parentFixture.compile();
-			for (const definedTrait of parentFixture.definedTraits) {
-				this.defineTrait(definedTrait);
-			}
+			this.parentFixture().compile();
 			super.compile();
 			this.compiled = true;
 		}
 	}
 
-	getParentAttributes(): Attribute[] {
-		const attributeNames = this.attributeNames();
-
-		return this.parentFixture()
-			.getAttributes()
-			.filter((attribute) => !attributeNames.includes(attribute.name));
+	mapTraitToThis(t: Trait): Trait {
+		t.fixture = this;
+		return t;
 	}
 
 	getAttributes(): Attribute[] {
 		this.compile();
 
-		// Need to generate child attributes before parent attributes
-		const definitionAttributes = super.getAttributes();
-		const attributesToKeep = this.getParentAttributes();
+		const attributesToKeep = this.parentFixture().getAttributes();
+
+		if (!this.attributes || this.attributes.length === 0) {
+			this.attributes = [
+				this.getBaseTraits()
+					.map((t) => this.mapTraitToThis(t))
+					.map((t) => t.getAttributes()),
+				this.declarationHandler.getAttributes(),
+				this.getAdditionalTraits()
+					.map((t) => this.mapTraitToThis(t))
+					.map((t) => t.getAttributes()),
+			].flat(Infinity).filter(Boolean);
+		}
 
-		return attributesToKeep.concat(definitionAttributes);
+		return attributesToKeep.concat(this.attributes);
 	}
 
 	getCallbacks(): Callback[] {
@@ -113,6 +117,12 @@ export class Fixture extends Definition {
 		return globalCallbacks.concat(parentCallbacks, definedCallbacks);
 	}
 
+	traitByName(name: string): Trait {
+		return this.traits[name] ||
+			this.parentFixture().traitByName(name) ||
+			this.fixtureRiveter.getTrait(name);
+	}
+
 	async run(buildStrategy: Strategy, overrides: Record<string, any> = {}): Promise<any> {
 		const evaluator = addMethodMissing(
 			new Evaluator(

lib/trait.ts

@@ -1,22 +1,46 @@
+import {Attribute} from "./attributes/attribute";
 import {Definition} from "./definition";
 import {DefinitionProxy} from "./definition-proxy";
 import {FixtureRiveter} from "./fixture-riveter";
 import {blockFunction} from "./fixture-options-parser";
 
 /* eslint-disable class-methods-use-this */
 export class Trait extends Definition {
+	fixture: any;
+
 	constructor(
 		name: string,
 		fixtureRiveter: FixtureRiveter,
 		block?: blockFunction,
 	) {
 		super(name, fixtureRiveter);
 
-		if (block) {
-			this.block = block;
-		}
+		this.block = block;
 
 		const proxy = new DefinitionProxy(this);
 		proxy.execute();
+
+		if (proxy.childFactories.length > 0) {
+			const [factory] = proxy.childFactories;
+			throw new Error(`Can't define a factory (${factory.name}) inside trait (${this.name})`);
+		}
+	}
+
+	defineTrait(newTrait: Trait): void {
+		throw new Error(`Can't define nested traits: ${newTrait.name} inside ${this.name}`);
+	}
+
+	traitByName(name: string): Trait {
+		if (this.fixture) {
+			return this.fixture.traitByName(name);
+		}
+		return this.fixtureRiveter.getTrait(name);
+	}
+
+	getAttributes(): Attribute[] {
+		return this.aggregateFromTraitsAndSelf(
+			"getAttributes",
+			() => this.declarationHandler.getAttributes(),
+		);
 	}
 }

test/acceptance/traits.ts

@@ -404,3 +404,45 @@ describe("tests from fixture_bot", function() {
 		});
 	});
 });
+
+describe("#968", function() {
+	let fr: FixtureRiveter;
+	let Demo: any;
+
+	beforeEach(async function() {
+		Demo = await defineModel("Demo", {value: "string"});
+		fr = new FixtureRiveter();
+
+		fr.fixture("parent", Demo, (f: any) => {
+			f.trait("y", (t: any) => {
+				t.value(() => "parent value");
+			});
+
+			f.trait("z", (t: any) => {
+				t.y();
+			});
+
+			f.fixture("child", Demo, (ff: any) => {
+				ff.trait("y", (t: any) => {
+					t.value(() => "child value");
+				});
+			});
+		});
+	});
+
+	specify("parent first", async function() {
+		const parent = await fr.build("parent", "z");
+		const child = await fr.build("child", "z");
+
+		expect(parent.value).to.equal("parent value");
+		expect(child.value).to.equal("child value");
+	});
+
+	specify("child first", async function() {
+		const child = await fr.build("child", "z");
+		const parent = await fr.build("parent", "z");
+
+		expect(parent.value).to.equal("parent value");
+		expect(child.value).to.equal("child value");
+	});
+});