Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: stop factory creating unused initial association #36

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ export class Factory<Model, Attributes, Params = any, ReturnType = Model> {
): Promise<ReturnType> {
const defaultAttributesWithAssociations = await this.resolveAssociations(
'create',
override,
additionalParams,
);

Expand Down Expand Up @@ -190,6 +191,7 @@ export class Factory<Model, Attributes, Params = any, ReturnType = Model> {

const attributesWithAssociations = await this.resolveAssociations(
'build',
override,
additionalParams,
);

Expand Down Expand Up @@ -409,11 +411,17 @@ export class Factory<Model, Attributes, Params = any, ReturnType = Model> {

private async resolveAssociations(
associationType: 'build' | 'create',
override?: Override<Attributes, ReturnType>,
additionalParams?: Params,
): Promise<Attributes> {
const attributes = await this.defaultAttributesFactory({
const rawAttributes = await this.defaultAttributesFactory({
transientParams: additionalParams,
});
const attributes = mergeDeep<Override<Attributes, ReturnType>>(
rawAttributes as Override<Attributes, ReturnType>,
override,
);

const defaultWithAssociations: Dictionary = {};

for (const prop in attributes as Dictionary) {
Expand Down
54 changes: 54 additions & 0 deletions test/factory.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { FactoryGirl } from '@src/factory-girl';
import { plainObject } from '@src/utils';
import { PartialDeep } from 'type-fest';
import { ModelAdapter, ObjectAdapter, SequelizeAdapter } from '../lib';
import { InstanceOrInterface } from '../lib/types/instance-or-interface.type';
import { Association, DeepPartialAttributes } from '../src';

type User = {
Expand Down Expand Up @@ -488,6 +490,58 @@ describe('Factory', () => {
expect(address).toMatchObject(addressAttributes);
expect(address.userId).toBe(userAttributes.id);
});

it('it should not create overridden associations', async () => {
Copy link
Owner

@thiagomini thiagomini Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature is something that is not exclusive to Sequelize, so it'd be better to test it with the Object Adapter (which is more database-agnostic). If you want to double check this is not creating a new entry in the database when used with Sequelize, I advise also adding a test to the sequelize-specific test suite

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thiagomini sorry I didn't notice this for a while - I've changed it to ObjectAdapter 👍 doesn't seem like the issue was specific to sequelize so no need to put it into a specific test suite I think

// Arrange
let createdAdditionalUser = false;

class TestableUserAdapter implements ObjectAdapter<User> {
build(
model: User,
props: PartialDeep<
InstanceOrInterface<User>,
{ recurseIntoArrays: true }
>,
): User {
return props as User;
}
async save(user: User): Promise<User> {
if (user.id !== 100) {
createdAdditionalUser = true;
}
return user;
}
get<K extends keyof User>(model: User, key: K): User[K] {
return model[key];
}
}

const userAttributes = buildUserAttributes();
const addressAttributes = buildAddressAttributes();

const userFactory = FactoryGirl.define(
plainObject<User>(),
() => userAttributes,
new TestableUserAdapter(),
);
const addressFactory = FactoryGirl.define(plainObject<Address>(), () => ({
...addressAttributes,
userId: userFactory.associate('id'),
}));

// Act
const user = await userFactory.create({
id: 100,
});
const address = await addressFactory.create({
userId: user.id,
});

// Assert
expect(createdAdditionalUser).toBeFalsy();
expect(address).toMatchObject(addressAttributes);
expect(address.userId).toBe(user.id);
});
});

describe('createMany', () => {
Expand Down
Loading