From aa4c91223727c92f64947a7ab44986d01c3da32d Mon Sep 17 00:00:00 2001 From: Or Cohen <351445+lightpriest@users.noreply.github.com> Date: Fri, 2 Sep 2022 15:31:00 +0300 Subject: [PATCH] Enable association compatibility with Sequelize When migrating to sequelize-typescript, if model A has two associations to model B, one without an alias configuration and one which is aliased, the migration is not possible at all. ```js ModelA.hasMany(ModelB) ModelA.hasMany(ModelB, { as: "secondB" }) ModelA.findAll({ include: [ModelB] }); ``` When configured using sequelize, it attempts to find the intended association. In our case, it's obvious we want ModelB without the alias. This is true especially if you have a lot of legacy code and you've added a second association. You don't expect the existing code to break having to add the alias variant of the include every on every query. When configured using sequelize-typescript, its `HasMany` and other association decorators *always* add an alias (`as: ..`) configuration to the options object. This later causes the above query `findAll` to fail with a message saying the alias cannot be inferred. This PR modifies a few different places and allows to opt-out of this behavior. 1. The decorators themselves will not include the alias if explicitly set to `undefined`. 2. The inferrence service matches sequelize's behavior and looks for non-aliased associations. For number 1, the change in behavior should not really be noticed, unless someone used `undefined` explicitly for some reason. And even then, it should only be noticed if the model name differs from the propery name. Normally passing around `undefined` should be seen as a code-smell, but for the sake of backwards compatibility and the fact that this allows easier migration of large codebases, I see this as negligble. Added relevant tests and updated the README. A general note: --------------- We patched this internally for our implementation, and running like this for the past 1.5 years in production. With a sequelize code base of >100 models, which spans a few years with all the edge-cases you could possibly think of, including polymorphism associations. I can safely say this is battle tested. On top of that, our implemented fix was a little bit different, not including aliases *at all* unless specified, and always using properyName for the value. Generally speaking, allowing the developer to specify an alias which differs from the properyName would yield unexpected results. This is something that needs to be handled in the repo but on a different track, since it has adds backwards incompatibility if applied. --- README.md | 43 +++++++++++++++++++ .../alias-inference-service.ts | 17 +++++++- .../belongs-to-many/belongs-to-many.ts | 2 +- src/associations/belongs-to/belongs-to.ts | 2 +- src/associations/has/has-many.ts | 2 +- src/associations/has/has-one.ts | 2 +- .../specs/annotations/belongs-to-many.spec.ts | 17 +++++++- test/specs/annotations/belongs-to.spec.ts | 12 +++++- test/specs/annotations/has-many.spec.ts | 15 ++++++- test/specs/annotations/has-one.spec.ts | 15 ++++++- 10 files changed, 118 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index cec531bb..ae8e1b9a 100644 --- a/README.md +++ b/README.md @@ -564,6 +564,49 @@ explicitly: proofedBooks: Book[]; ``` +### Multiple relations of same models and sequelize aliasing compatibility + +When defining a relationship, _sequelize_ allows you to configure an alias, but it also allows you to configure empty aliases +as a sort of default relationship when querying. When using _sequelize-typescript_, it defaults to add an alias. + +Consider the following model definition: + +```typescript +@Table +class User extends Model { + @HasMany(() => Comment) + comments: Comment[]; + + @HasMany(() => Comment, { + as: 'archivedComments' + }) + archivedComments: Comment[]; +} +``` + +Running the following query will result in an error: + +```typescript +User.findOne({ include: [Comment, 'archivedComments'] }) +// Error: Alias cannot be inferred: "user" has multiple relations with "comment" +``` + +To revert to the original _sequelize_ behavior, you can turn off the automatic aliasing by specifying `undefined` for +the alias value: + +```typescript +@Table({ modelName: 'user' }) +class User extends Model { + @HasMany(() => Comment, { as: undefined }) + comments: Comment[]; + + @HasMany(() => Comment, { + as: 'archivedComments' + }) + archivedComments: Comment[]; +} +``` + ### Type safe usage of auto generated functions With the creation of a relation, sequelize generates some method on the corresponding diff --git a/src/associations/alias-inference/alias-inference-service.ts b/src/associations/alias-inference/alias-inference-service.ts index c6a7c9a8..3d188903 100644 --- a/src/associations/alias-inference/alias-inference-service.ts +++ b/src/associations/alias-inference/alias-inference-service.ts @@ -1,5 +1,18 @@ +import { BaseAssociation } from '../shared/base-association'; import { getAssociationsByRelation } from '../shared/association-service'; +/** + * Returns true if automatic association is possible given include parameters and list of associations + */ +function automaticAssociationInferrencePossible( + include: { model: any; as: any }, + associations: BaseAssociation[] +): boolean { + const hasModelOptionWithoutAsOption = !!(include.model && !include.as); + const associationsWithoutAs = associations.filter((assoc) => !assoc.getAs()); + return associationsWithoutAs.length === 1 && hasModelOptionWithoutAsOption; +} + /** * Pre conform includes, so that "as" value can be inferred from source */ @@ -44,7 +57,9 @@ function inferAliasForInclude(include: any, source: any): any { const relatedClass = include.model; const associations = getAssociationsByRelation(targetPrototype, relatedClass); - if (associations.length > 0) { + if (automaticAssociationInferrencePossible(include, associations)) { + // Do nothing, this "include" is actually fine + } else if (associations.length > 0) { if (associations.length > 1) { throw new Error( `Alias cannot be inferred: "${source.name}" has multiple ` + diff --git a/src/associations/belongs-to-many/belongs-to-many.ts b/src/associations/belongs-to-many/belongs-to-many.ts index 8a460d41..f2af363b 100644 --- a/src/associations/belongs-to-many/belongs-to-many.ts +++ b/src/associations/belongs-to-many/belongs-to-many.ts @@ -48,7 +48,7 @@ export function BelongsToMany< options = { ...throughOrOptions }; } - if (!options.as) options.as = propertyName; + if (!options.as && !('as' in options)) options.as = propertyName; addAssociation( target, diff --git a/src/associations/belongs-to/belongs-to.ts b/src/associations/belongs-to/belongs-to.ts index 3e3b7065..fe4ef517 100644 --- a/src/associations/belongs-to/belongs-to.ts +++ b/src/associations/belongs-to/belongs-to.ts @@ -20,7 +20,7 @@ export function BelongsTo( ): Function { return (target: any, propertyName: string) => { const options: BelongsToOptions = getPreparedAssociationOptions(optionsOrForeignKey); - if (!options.as) options.as = propertyName; + if (!options.as && !('as' in options)) options.as = propertyName; addAssociation(target, new BelongsToAssociation(associatedClassGetter, options)); }; } diff --git a/src/associations/has/has-many.ts b/src/associations/has/has-many.ts index 2574fb90..169066b4 100644 --- a/src/associations/has/has-many.ts +++ b/src/associations/has/has-many.ts @@ -21,7 +21,7 @@ export function HasMany( ): Function { return (target: any, propertyName: string) => { const options: HasManyOptions = getPreparedAssociationOptions(optionsOrForeignKey); - if (!options.as) options.as = propertyName; + if (!options.as && !('as' in options)) options.as = propertyName; addAssociation(target, new HasAssociation(associatedClassGetter, options, Association.HasMany)); }; } diff --git a/src/associations/has/has-one.ts b/src/associations/has/has-one.ts index eec58953..8d5330db 100644 --- a/src/associations/has/has-one.ts +++ b/src/associations/has/has-one.ts @@ -21,7 +21,7 @@ export function HasOne( ): Function { return (target: any, propertyName: string) => { const options: HasOneOptions = getPreparedAssociationOptions(optionsOrForeignKey); - if (!options.as) options.as = propertyName; + if (!options.as && !('as' in options)) options.as = propertyName; addAssociation(target, new HasAssociation(associatedClassGetter, options, Association.HasOne)); }; } diff --git a/test/specs/annotations/belongs-to-many.spec.ts b/test/specs/annotations/belongs-to-many.spec.ts index 99b75d5a..e27f2d8e 100644 --- a/test/specs/annotations/belongs-to-many.spec.ts +++ b/test/specs/annotations/belongs-to-many.spec.ts @@ -14,6 +14,9 @@ describe('BelongsToMany', () => { @Table class Team extends Model {} + @Table + class Rank extends Model {} + @Table class Player extends Model { @BelongsToMany(() => Team, { @@ -23,11 +26,23 @@ describe('BelongsToMany', () => { otherKey: 'teamId', }) teams: Team[]; + + @BelongsToMany(() => Rank, { + as: undefined, + through: 'RankPlayer', + foreignKey: 'playerId', + otherKey: 'rankId', + }) + Ranks: Rank[]; } - sequelize.addModels([Team, Player]); + sequelize.addModels([Team, Player, Rank]); it('should pass as options to sequelize association', () => { expect(Player['associations']).to.have.property(as); }); + + it('should use association model name if passed undefined for "as"', () => { + expect(Player['associations']).to.have.property('Ranks'); + }); }); diff --git a/test/specs/annotations/belongs-to.spec.ts b/test/specs/annotations/belongs-to.spec.ts index 460d2d2d..8d94db47 100644 --- a/test/specs/annotations/belongs-to.spec.ts +++ b/test/specs/annotations/belongs-to.spec.ts @@ -14,18 +14,28 @@ describe('BelongsTo', () => { @Table class Team extends Model {} + @Table + class Rank extends Model {} + @Table class Player extends Model { @BelongsTo(() => Team, { as, foreignKey: 'teamId' }) team: Team; + + @BelongsTo(() => Rank, { as: undefined, foreignKey: 'rankId' }) + Rank: Rank; } - sequelize.addModels([Team, Player]); + sequelize.addModels([Team, Player, Rank]); it('should pass as options to sequelize association', () => { expect(Player['associations']).to.have.property(as); }); + it('should use association model name if passed undefined for "as"', () => { + expect(Player['associations']).to.have.property('Rank'); + }); + it('should throw due to missing foreignKey', () => { const _sequelize = createSequelize(false); diff --git a/test/specs/annotations/has-many.spec.ts b/test/specs/annotations/has-many.spec.ts index 290cc727..000a53f1 100644 --- a/test/specs/annotations/has-many.spec.ts +++ b/test/specs/annotations/has-many.spec.ts @@ -14,6 +14,9 @@ describe('HasMany', () => { @Table class Player extends Model {} + @Table + class Rank extends Model {} + @Table class Team extends Model { @HasMany(() => Player, { @@ -21,11 +24,21 @@ describe('HasMany', () => { foreignKey: 'teamId', }) players: Player[]; + + @HasMany(() => Rank, { + as: undefined, + foreignKey: 'rankId', + }) + Ranks: Rank[]; } - sequelize.addModels([Team, Player]); + sequelize.addModels([Team, Player, Rank]); it('should pass as options to sequelize association', () => { expect(Team['associations']).to.have.property(as); }); + + it('should use association model name if passed undefined for "as"', () => { + expect(Team['associations']).to.have.property('Ranks'); + }); }); diff --git a/test/specs/annotations/has-one.spec.ts b/test/specs/annotations/has-one.spec.ts index 6cf0c89a..b3c456ac 100644 --- a/test/specs/annotations/has-one.spec.ts +++ b/test/specs/annotations/has-one.spec.ts @@ -14,6 +14,9 @@ describe('HasOne', () => { @Table class Player extends Model {} + @Table + class Rank extends Model {} + @Table class Team extends Model { @HasOne(() => Player, { @@ -21,11 +24,21 @@ describe('HasOne', () => { foreignKey: 'teamId', }) player: Player; + + @HasOne(() => Rank, { + as: undefined, + foreignKey: 'rankId', + }) + Rank: Rank; } - sequelize.addModels([Team, Player]); + sequelize.addModels([Team, Player, Rank]); it('should pass as options to sequelize association', () => { expect(Team['associations']).to.have.property(as); }); + + it('should use association model name if passed undefined for "as"', () => { + expect(Player['associations']).to.have.property('Rank'); + }); });