From 1e22d66da80e71c837facfc855e914c8c81ad5f0 Mon Sep 17 00:00:00 2001 From: Jeeiii Date: Fri, 27 Oct 2023 20:50:37 +0200 Subject: [PATCH 1/2] refactor: add many-to-many relationship between Group and Member BREAKING CHANGE: Many-to-many relationship between Group and Member fix #265 --- .../credentials/credentials.service.test.ts | 8 +- .../src/app/groups/entities/group.entity.ts | 15 ++- .../src/app/groups/entities/member.entity.ts | 27 +----- apps/api/src/app/groups/groups.service.ts | 97 +++++++++++++++---- 4 files changed, 100 insertions(+), 47 deletions(-) diff --git a/apps/api/src/app/credentials/credentials.service.test.ts b/apps/api/src/app/credentials/credentials.service.test.ts index 1fe758aa..e4f1d367 100644 --- a/apps/api/src/app/credentials/credentials.service.test.ts +++ b/apps/api/src/app/credentials/credentials.service.test.ts @@ -162,9 +162,15 @@ describe("CredentialsService", () => { }) it("Should add a member to a credential group", async () => { + const _stateId = await credentialsService.setOAuthState({ + groupId, + memberId: "125", + providerName: "twitter" + }) + const clientRedirectUri = await credentialsService.addMember( "code", - stateId + _stateId ) expect(clientRedirectUri).toBeUndefined() diff --git a/apps/api/src/app/groups/entities/group.entity.ts b/apps/api/src/app/groups/entities/group.entity.ts index fc050bc4..4746455e 100644 --- a/apps/api/src/app/groups/entities/group.entity.ts +++ b/apps/api/src/app/groups/entities/group.entity.ts @@ -3,6 +3,8 @@ import { CreateDateColumn, Entity, Index, + JoinTable, + ManyToMany, OneToMany, PrimaryColumn, UpdateDateColumn @@ -31,8 +33,17 @@ export class Group { @Column({ name: "fingerprint_duration" }) fingerprintDuration: number - @OneToMany(() => Member, (member) => member.group, { - cascade: ["insert"] + @ManyToMany(() => Member) + @JoinTable({ + name: "memberships", + joinColumn: { + name: "group", + referencedColumnName: "id" + }, + inverseJoinColumn: { + name: "member", + referencedColumnName: "id" + } }) members: Member[] diff --git a/apps/api/src/app/groups/entities/member.entity.ts b/apps/api/src/app/groups/entities/member.entity.ts index 2676b2dd..5dae0490 100644 --- a/apps/api/src/app/groups/entities/member.entity.ts +++ b/apps/api/src/app/groups/entities/member.entity.ts @@ -1,32 +1,11 @@ -import { - CreateDateColumn, - Entity, - ManyToOne, - Column, - Index, - Unique, - JoinColumn -} from "typeorm" - -import { Group } from "./group.entity" +import { CreateDateColumn, Entity, Index, PrimaryColumn } from "typeorm" @Entity("members") -@Unique(["id", "group"]) -@Index(["id", "group"]) export class Member { - @Column({ primary: true, unique: false }) + @PrimaryColumn() + @Index({ unique: true }) id: string - // In reality the relation group -> members is many-to-many. - // i.e we allow same member id to be part of many groups. - // But since this property is not used in any feature at the moment, - // it is treated as many-to-one in the code for simplicity. - @ManyToOne(() => Group, (group) => group.members, { - onDelete: "CASCADE" - }) - @JoinColumn({ name: "group_id" }) - group: Group - @CreateDateColumn({ name: "created_at" }) createdAt: Date } diff --git a/apps/api/src/app/groups/groups.service.ts b/apps/api/src/app/groups/groups.service.ts index 42836a9c..099715e0 100644 --- a/apps/api/src/app/groups/groups.service.ts +++ b/apps/api/src/app/groups/groups.service.ts @@ -341,11 +341,28 @@ export class GroupsService { async addMember(groupId: string, memberId: string): Promise { const group = await this.getGroup(groupId) - const member = new Member() - member.group = group - member.id = memberId + // Check if the member is already a group member. + const isMemberInGroup = group.members.some((m) => m.id === memberId) - group.members.push(member) + if (isMemberInGroup) { + throw new Error( + `Member '${memberId}' is already in the group '${group.name}'` + ) + } + + // Check if the member is already a member of another group. + let member = await this.memberRepository.findOne({ + where: { id: memberId } + }) + + if (!member) { + member = new Member() + member.id = memberId + + await this.memberRepository.save(member) + } + + group.members = group.members.concat([member]) await this.groupRepository.save(group) @@ -371,20 +388,33 @@ export class GroupsService { async addMembers(groupId: string, memberIds: string[]): Promise { const group = await this.getGroup(groupId) - // Prepare new members and attach them to the group - const newMembers = memberIds.map((memberId) => { - const member = new Member() - member.group = group - member.id = memberId - return member - }) + for (const memberId of memberIds) { + // Check if the member is already a group member. + const member = group.members.find((m) => m.id === memberId) + + if (!member) { + let newMember: Member + + // Check if the member is already a member of another group. + const anotherGroupMember = await this.memberRepository.findOne({ + where: { id: memberId } + }) + + if (!anotherGroupMember) { + newMember = new Member() + newMember.id = memberId + + await this.memberRepository.save(newMember) + } else newMember = anotherGroupMember + + group.members = group.members.concat([newMember]) + } + } - group.members.push(...newMembers) await this.groupRepository.save(group) const cachedGroup = this.cachedGroups.get(groupId) - // Add all the members to the cached group at once memberIds.forEach((memberId) => { cachedGroup.addMember(memberId) Logger.log( @@ -440,15 +470,28 @@ export class GroupsService { `Member '${memberId}' is not a member of group '${groupId}'` ) } - const member = group.members.find((m) => m.id === memberId) + const member: Member = group.members.find( + (m: Member) => m.id === memberId + ) + if (member) membersToRemove.push(member) } - await this.memberRepository.remove(membersToRemove) - const cachedGroup = this.cachedGroups.get(groupId) for (const memberId of memberIds) { + // Check if the member is a group member. + const memberIndex = group.members.findIndex( + (m) => m.id === memberId + ) + + if (memberIndex !== -1) { + // Remove the member from the group. + group.members.splice(memberIndex, 1) + + await this.groupRepository.save(group) + } + cachedGroup.removeMember(cachedGroup.indexOf(BigInt(memberId))) Logger.log( `GroupsService: member '${memberId}' has been removed from the group '${group.name}'` @@ -501,17 +544,31 @@ export class GroupsService { `Member '${memberId}' is not a member of group '${groupId}'` ) } - const member = group.members.find((m) => m.id === memberId) + + // Check if the member is a group member. + const member: Member = group.members.find( + (m: Member) => m.id === memberId + ) + if (member) membersToRemove.push(member) } - await this.memberRepository.remove(membersToRemove) - const cachedGroup = this.cachedGroups.get(groupId) for (const memberId of memberIds) { - cachedGroup.removeMember(cachedGroup.indexOf(BigInt(memberId))) + Logger.log(memberId) + const memberIndex = group.members.findIndex( + (m) => m.id === memberId + ) + + if (memberIndex !== -1) { + // Remove the member from the group. + group.members.splice(memberIndex, 1) + + await this.groupRepository.save(group) + } + cachedGroup.removeMember(cachedGroup.indexOf(BigInt(memberId))) Logger.log( `GroupsService: member '${memberId}' has been removed from the group '${group.name}'` ) From 20b1d6bc8dbd62fac8a506bdbeb92c912d185288 Mon Sep 17 00:00:00 2001 From: Jeeiii Date: Tue, 31 Oct 2023 09:43:52 +0100 Subject: [PATCH 2/2] chore: rectify lint errors --- apps/api/src/app/groups/groups.service.ts | 104 ++++++++++++---------- 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/apps/api/src/app/groups/groups.service.ts b/apps/api/src/app/groups/groups.service.ts index 099715e0..6ccd3dfa 100644 --- a/apps/api/src/app/groups/groups.service.ts +++ b/apps/api/src/app/groups/groups.service.ts @@ -362,7 +362,7 @@ export class GroupsService { await this.memberRepository.save(member) } - group.members = group.members.concat([member]) + group.members.push(member) await this.groupRepository.save(group) @@ -388,28 +388,32 @@ export class GroupsService { async addMembers(groupId: string, memberIds: string[]): Promise { const group = await this.getGroup(groupId) - for (const memberId of memberIds) { - // Check if the member is already a group member. - const member = group.members.find((m) => m.id === memberId) + await Promise.all( + memberIds.map(async (memberId) => { + const member = group.members.find((m) => m.id === memberId) - if (!member) { - let newMember: Member + if (!member) { + let newMember: Member - // Check if the member is already a member of another group. - const anotherGroupMember = await this.memberRepository.findOne({ - where: { id: memberId } - }) + // Check if the member is already a member of another group. + const anotherGroupMember = + await this.memberRepository.findOne({ + where: { id: memberId } + }) - if (!anotherGroupMember) { - newMember = new Member() - newMember.id = memberId + if (!anotherGroupMember) { + newMember = new Member() + newMember.id = memberId - await this.memberRepository.save(newMember) - } else newMember = anotherGroupMember + await this.memberRepository.save(newMember) + } else { + newMember = anotherGroupMember + } - group.members = group.members.concat([newMember]) - } - } + group.members.push(newMember) + } + }) + ) await this.groupRepository.save(group) @@ -479,24 +483,26 @@ export class GroupsService { const cachedGroup = this.cachedGroups.get(groupId) - for (const memberId of memberIds) { - // Check if the member is a group member. - const memberIndex = group.members.findIndex( - (m) => m.id === memberId - ) + await Promise.all( + memberIds.map(async (memberId) => { + // Check if the member is a group member. + const memberIndex = group.members.findIndex( + (m) => m.id === memberId + ) - if (memberIndex !== -1) { - // Remove the member from the group. - group.members.splice(memberIndex, 1) + if (memberIndex !== -1) { + // Remove the member from the group. + group.members.splice(memberIndex, 1) - await this.groupRepository.save(group) - } + await this.groupRepository.save(group) + } - cachedGroup.removeMember(cachedGroup.indexOf(BigInt(memberId))) - Logger.log( - `GroupsService: member '${memberId}' has been removed from the group '${group.name}'` - ) - } + cachedGroup.removeMember(cachedGroup.indexOf(BigInt(memberId))) + Logger.log( + `GroupsService: member '${memberId}' has been removed from the group '${group.name}'` + ) + }) + ) this._updateContractGroup(cachedGroup) @@ -555,24 +561,26 @@ export class GroupsService { const cachedGroup = this.cachedGroups.get(groupId) - for (const memberId of memberIds) { - Logger.log(memberId) + await Promise.all( + memberIds.map(async (memberId) => { + // Check if the member is a group member. + const memberIndex = group.members.findIndex( + (m) => m.id === memberId + ) - const memberIndex = group.members.findIndex( - (m) => m.id === memberId - ) + if (memberIndex !== -1) { + // Remove the member from the group. + group.members.splice(memberIndex, 1) - if (memberIndex !== -1) { - // Remove the member from the group. - group.members.splice(memberIndex, 1) + await this.groupRepository.save(group) + } - await this.groupRepository.save(group) - } - cachedGroup.removeMember(cachedGroup.indexOf(BigInt(memberId))) - Logger.log( - `GroupsService: member '${memberId}' has been removed from the group '${group.name}'` - ) - } + cachedGroup.removeMember(cachedGroup.indexOf(BigInt(memberId))) + Logger.log( + `GroupsService: member '${memberId}' has been removed from the group '${group.name}'` + ) + }) + ) this._updateContractGroup(cachedGroup)