Skip to content

Commit

Permalink
fix: improve association decorators (#16486)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ephys
ephys committed Sep 12, 2023
1 parent fd403a2 commit b8edd52
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 16 deletions.
20 changes: 13 additions & 7 deletions packages/core/src/decorators/legacy/associations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ function decorateAssociation(
throw new TypeError('Symbol associations are not currently supported. We welcome a PR that implements this feature.');
}

if (options.as) {
throw new Error('The "as" option is not allowed when using association decorators. The name of the decorated field is used as the association name.');
}

const associations = registeredAssociations.get(sourceClass) ?? [];
registeredAssociations.set(sourceClass, associations);

Expand All @@ -61,7 +65,7 @@ function decorateAssociation(

export function HasOne<Target extends Model>(
target: MaybeForwardedModelStatic<Target>,
optionsOrForeignKey: HasOneOptions<string, AttributeNames<Target>> | AttributeNames<Target>,
optionsOrForeignKey: Omit<HasOneOptions<string, AttributeNames<Target>>, 'as'> | AttributeNames<Target>,
) {
return (source: Model, associationName: string | symbol) => {
const options = isString(optionsOrForeignKey) ? { foreignKey: optionsOrForeignKey } : optionsOrForeignKey;
Expand All @@ -72,7 +76,7 @@ export function HasOne<Target extends Model>(

export function HasMany<Target extends Model>(
target: MaybeForwardedModelStatic<Target>,
optionsOrForeignKey: HasManyOptions<string, AttributeNames<Target>> | AttributeNames<Target>,
optionsOrForeignKey: Omit<HasManyOptions<string, AttributeNames<Target>>, 'as'> | AttributeNames<Target>,
) {
return (source: Model, associationName: string | symbol) => {
const options = isString(optionsOrForeignKey) ? { foreignKey: optionsOrForeignKey } : optionsOrForeignKey;
Expand All @@ -83,12 +87,14 @@ export function HasMany<Target extends Model>(

export function BelongsTo<SourceKey extends string, Target extends Model>(
target: MaybeForwardedModelStatic<Target>,
optionsOrForeignKey: BelongsToOptions<SourceKey, AttributeNames<Target>> | SourceKey,
optionsOrForeignKey: Omit<BelongsToOptions<SourceKey, AttributeNames<Target>>, 'as'> | SourceKey,
) {
return (
// This type is a hack to make sure the source model declares a property named [SourceKey].
// The error message is going to be horrendous, but at least it's enforced.
source: Model<{ [key in SourceKey]: any }>,
// Ideally we'd type this in a way that enforces that the sourceKey is an attribute of the source model,
// but that does not work when the model itself receives its attributes as a generic parameter.
// We'll revisit this when we have a better solution.
// source: Model<{ [key in SourceKey]: any }>,
source: Model,
associationName: string,
) => {
const options = isString(optionsOrForeignKey) ? { foreignKey: optionsOrForeignKey } : optionsOrForeignKey;
Expand All @@ -99,7 +105,7 @@ export function BelongsTo<SourceKey extends string, Target extends Model>(

export function BelongsToMany(
target: MaybeForwardedModelStatic,
options: BelongsToManyOptions,
options: Omit<BelongsToManyOptions, 'as'>,
): PropertyDecorator {
return (
source: Object,
Expand Down
107 changes: 98 additions & 9 deletions packages/core/test/unit/decorators/associations.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { BelongsTo, BelongsToMany, HasMany, HasOne } from '@sequelize/core/decor
import { sequelize, typeTest } from '../../support';

const CANNOT_INHERIT_ASSOCIATION_ERROR = /Models that use @HasOne, @HasMany, or @BelongsToMany associations cannot be inherited from/;
const CANNOT_USE_AS_ERROR = 'The "as" option is not allowed when using association decorators. The name of the decorated field is used as the association name.';

describe('@BelongsTo', () => {
it('defines a belongsTo association', () => {
Expand Down Expand Up @@ -38,18 +39,31 @@ describe('@BelongsTo', () => {
});
});

typeTest('errors if the foreign key does not exist on the target model', () => {
// This test is temporarily disabled until we find a solution that works with generics
// typeTest('errors if the foreign key does not exist on the target model', () => {
// class User extends Model<InferAttributes<User>> {}
//
// // eslint-disable-next-line @typescript-eslint/no-unused-vars
// class Profile extends Model<InferAttributes<Profile>> {
// // @ts-expect-error -- This must error, "usrId" does not exist on "Profile"
// @BelongsTo(() => User, 'usrId')
// declare user1: Profile;
//
// // @ts-expect-error -- This must error, "usrId" does not exist on "Profile"
// @BelongsTo(() => User, { foreignKey: 'usrId' })
// declare user2: User;
//
// declare userId: number;
// }
// });

typeTest('does not error when the model is generic (inheritance)', () => {
class User extends Model<InferAttributes<User>> {}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
class Profile extends Model<InferAttributes<Profile>> {
// @ts-expect-error -- This must error, "usrId" does not exist on "Profile"
@BelongsTo(() => User, 'usrId')
declare user1: Profile;

// @ts-expect-error -- This must error, "usrId" does not exist on "Profile"
@BelongsTo(() => User, { foreignKey: 'usrId' })
declare user2: User;
class Profile<T extends Profile<T> = Profile<any>> extends Model<InferAttributes<T>> {
@BelongsTo(() => User, 'userId')
declare user: Profile;

declare userId: number;
}
Expand Down Expand Up @@ -117,6 +131,25 @@ describe('@BelongsTo', () => {

expect(() => sequelize.addModels([User, DummyModel])).to.throw(`You have defined two associations with the same name "dummy" on the model "User"`);
});

it('throws if the "as" option is used', () => {
class DummyModel extends Model<InferAttributes<DummyModel>> {}

expect(() => {
class User extends Model<InferAttributes<User>> {
@BelongsTo(() => DummyModel, {
foreignKey: 'dummyId',
// @ts-expect-error -- forbidden option
as: 'dummy',
})
declare dummy?: NonAttribute<DummyModel>;

declare dummyId: number;
}

return User;
}).to.throw(CANNOT_USE_AS_ERROR);
});
});

describe('@HasOne', () => {
Expand Down Expand Up @@ -181,6 +214,25 @@ describe('@HasOne', () => {

expect(() => sequelize.addModels([DummyModel, User])).to.throw(CANNOT_INHERIT_ASSOCIATION_ERROR);
});

it('throws if the "as" option is used', () => {
class DummyModel extends Model<InferAttributes<DummyModel>> {
declare dummyId: number;
}

expect(() => {
class User extends Model<InferAttributes<User>> {
@HasOne(() => DummyModel, {
foreignKey: 'dummyId',
// @ts-expect-error -- forbidden option
as: 'dummy',
})
declare dummy?: NonAttribute<DummyModel>;
}

return User;
}).to.throw(CANNOT_USE_AS_ERROR);
});
});

describe('@HasMany', () => {
Expand Down Expand Up @@ -245,6 +297,25 @@ describe('@HasMany', () => {

expect(() => sequelize.addModels([DummyModel, User])).to.throw(CANNOT_INHERIT_ASSOCIATION_ERROR);
});

it('throws if the "as" option is used', () => {
class DummyModel extends Model<InferAttributes<DummyModel>> {
declare dummyId: number;
}

expect(() => {
class User extends Model<InferAttributes<User>> {
@HasMany(() => DummyModel, {
foreignKey: 'dummyId',
// @ts-expect-error -- forbidden option
as: 'dummy',
})
declare dummy?: NonAttribute<DummyModel>;
}

return User;
}).to.throw(CANNOT_USE_AS_ERROR);
});
});

describe('@BelongsToMany', () => {
Expand Down Expand Up @@ -313,4 +384,22 @@ describe('@BelongsToMany', () => {

expect(() => sequelize.addModels([DummyModel, User])).to.throw(CANNOT_INHERIT_ASSOCIATION_ERROR);
});

it('throws if the "as" option is used', () => {
class Role extends Model<InferAttributes<Role>> {}

expect(() => {
class User extends Model<InferAttributes<User>> {
@BelongsToMany(() => Role, {
// @ts-expect-error -- forbidden option
as: 'roles',
through: 'UserRole',
inverse: { as: 'users' },
})
declare roles: Role[];
}

return User;
}).to.throw(CANNOT_USE_AS_ERROR);
});
});

0 comments on commit b8edd52

Please sign in to comment.