Fix role member bulk update semantics

src/api/routes/guilds/#guild_id/roles/#role_id/members.ts

@@ -17,15 +17,28 @@
 */
 
 import { Router, Request, Response } from "express";
-import { DiscordApiErrors, Member, arrayPartition } from "@spacebar/util";
-import { route } from "@spacebar/api";
+import { RoleMembersUpdateSchema } from "@spacebar/schemas";
+import { DiscordApiErrors, Member } from "@spacebar/util";
+import { calculateRoleMemberAdditions, calculateRoleMemberReplacement, route } from "@spacebar/api";
 
 const router = Router({ mergeParams: true });
+type RoleMemberUpdateMode = "add" | "replace";
 
-router.patch("/", route({ permission: "MANAGE_ROLES" }), async (req: Request, res: Response) => {
-    // Payload is JSON containing a list of member_ids, the new list of members to have the role
+const routeOptions = route({
+    permission: "MANAGE_ROLES",
+    requestBody: "RoleMembersUpdateSchema",
+    responses: {
+        204: {},
+        403: {
+            body: "APIErrorResponse",
+        },
+    },
+});
+
+async function updateRoleMembers(req: Request, res: Response, mode: RoleMemberUpdateMode) {
+    // Payload is JSON containing a list of member_ids to add (PATCH) or set as the exact role membership (PUT)
     const { guild_id, role_id } = req.params as { [key: string]: string };
-    const { member_ids } = req.body;
+    const { member_ids } = req.body as RoleMembersUpdateSchema;
 
     // don't mess with @everyone
     if (role_id == guild_id) throw DiscordApiErrors.INVALID_ROLE;
@@ -35,12 +48,19 @@ router.patch("/", route({ permission: "MANAGE_ROLES" }), async (req: Request, re
         relations: { roles: true },
     });
 
-    const [add, remove] = arrayPartition(members, (member) => member_ids.includes(member.id) && !member.roles.map((role) => role.id).includes(role_id));
+    const { addMemberIds, removeMemberIds } =
+        mode === "replace" ? calculateRoleMemberReplacement(members, member_ids, role_id) : calculateRoleMemberAdditions(members, member_ids, role_id);
 
     // TODO (erkin): have a bulk add/remove function that adds the roles in a single txn
-    await Promise.all([...add.map((member) => Member.addRole(member.id, guild_id, role_id)), ...remove.map((member) => Member.removeRole(member.id, guild_id, role_id))]);
+    await Promise.all([
+        ...addMemberIds.map((memberId) => Member.addRole(memberId, guild_id, role_id)),
+        ...removeMemberIds.map((memberId) => Member.removeRole(memberId, guild_id, role_id)),
+    ]);
 
     res.sendStatus(204);
-});
+}
+
+router.patch("/", routeOptions, (req: Request, res: Response) => updateRoleMembers(req, res, "add"));
+router.put("/", routeOptions, (req: Request, res: Response) => updateRoleMembers(req, res, "replace"));
 
 export default router;

src/api/util/index.ts

@@ -26,3 +26,4 @@ export * from "./utility/String";
 export * from "./handlers/Voice";
 export * from "./utility/captcha";
 export * from "./utility/EmbedHandlers";
+export * from "./utility/RoleMembers";

src/api/util/utility/RoleMemberRoutes.test.ts

@@ -0,0 +1,18 @@
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import path from "node:path";
+import { describe, test } from "node:test";
+
+function getRoleMembersRouteSource() {
+    return fs.readFileSync(path.join(process.cwd(), "src/api/routes/guilds/#guild_id/roles/#role_id/members.ts"), "utf8");
+}
+
+describe("role member update route", () => {
+    test("registers PATCH additive and PUT replacement endpoints with the shared schema", () => {
+        const source = getRoleMembersRouteSource();
+
+        assert.ok(source.includes('requestBody: "RoleMembersUpdateSchema"'));
+        assert.ok(source.includes('router.patch("/", routeOptions, (req: Request, res: Response) => updateRoleMembers(req, res, "add"));'));
+        assert.ok(source.includes('router.put("/", routeOptions, (req: Request, res: Response) => updateRoleMembers(req, res, "replace"));'));
+    });
+});

src/api/util/utility/RoleMembers.test.ts

@@ -0,0 +1,73 @@
+import assert from "node:assert/strict";
+import { describe, test } from "node:test";
+import { calculateRoleMemberAdditions, calculateRoleMemberReplacement } from "./RoleMembers";
+
+describe("role member update helpers", () => {
+    const roleId = "role";
+    const otherRoleId = "other";
+    const members = [
+        { id: "already-desired", roles: [{ id: roleId }] },
+        { id: "needs-add", roles: [{ id: otherRoleId }] },
+        { id: "needs-remove", roles: [{ id: roleId }, { id: otherRoleId }] },
+        { id: "unrelated", roles: [] },
+    ];
+
+    test("PATCH additions add missing desired members without removing omitted current holders", () => {
+        const changes = calculateRoleMemberAdditions(members, ["already-desired", "needs-add"], roleId);
+
+        assert.deepEqual(changes, {
+            addMemberIds: ["needs-add"],
+            removeMemberIds: [],
+        });
+    });
+
+    test("PATCH additions keep existing holders that were omitted", () => {
+        const changes = calculateRoleMemberAdditions(members, ["needs-add"], roleId);
+
+        assert.deepEqual(changes, {
+            addMemberIds: ["needs-add"],
+            removeMemberIds: [],
+        });
+    });
+
+    test("PATCH additions deduplicate desired member ids through set semantics", () => {
+        const changes = calculateRoleMemberAdditions(members, ["needs-add", "needs-add"], roleId);
+
+        assert.deepEqual(changes, {
+            addMemberIds: ["needs-add"],
+            removeMemberIds: [],
+        });
+    });
+
+    test("PUT replacement adds missing desired members and removes omitted current holders", () => {
+        const changes = calculateRoleMemberReplacement(members, ["already-desired", "needs-add"], roleId);
+
+        assert.deepEqual(changes, {
+            addMemberIds: ["needs-add"],
+            removeMemberIds: ["needs-remove"],
+        });
+    });
+
+    test("PUT replacement keeps desired current holders and unrelated non-holders unchanged", () => {
+        const changes = calculateRoleMemberReplacement(members, ["already-desired"], roleId);
+
+        assert.equal(changes.addMemberIds.includes("already-desired"), false);
+        assert.equal(changes.removeMemberIds.includes("already-desired"), false);
+        assert.equal(changes.removeMemberIds.includes("unrelated"), false);
+    });
+
+    test("deduplicates desired member ids through set semantics", () => {
+        const changes = calculateRoleMemberReplacement(members, ["needs-add", "needs-add"], roleId);
+
+        assert.deepEqual(changes.addMemberIds, ["needs-add"]);
+    });
+
+    test("empty PUT replacement desired list removes only current role holders", () => {
+        const changes = calculateRoleMemberReplacement(members, [], roleId);
+
+        assert.deepEqual(changes, {
+            addMemberIds: [],
+            removeMemberIds: ["already-desired", "needs-remove"],
+        });
+    });
+});

src/api/util/utility/RoleMembers.ts

@@ -0,0 +1,41 @@
+type RoleMember = {
+    id: string;
+    roles: { id: string }[];
+};
+
+export type RoleMemberChanges = {
+    addMemberIds: string[];
+    removeMemberIds: string[];
+};
+
+function memberHasRole(member: RoleMember, roleId: string) {
+    return member.roles.some((role) => role.id === roleId);
+}
+
+export function calculateRoleMemberAdditions(members: RoleMember[], memberIds: string[], roleId: string): RoleMemberChanges {
+    const desiredMemberIds = new Set(memberIds);
+    const addMemberIds: string[] = [];
+
+    for (const member of members) {
+        const hasRole = memberHasRole(member, roleId);
+        const shouldHaveRole = desiredMemberIds.has(member.id);
+
+        if (shouldHaveRole && !hasRole) addMemberIds.push(member.id);
+    }
+
+    return { addMemberIds, removeMemberIds: [] };
+}
+
+export function calculateRoleMemberReplacement(members: RoleMember[], memberIds: string[], roleId: string): RoleMemberChanges {
+    const changes = calculateRoleMemberAdditions(members, memberIds, roleId);
+    const desiredMemberIds = new Set(memberIds);
+
+    for (const member of members) {
+        const hasRole = memberHasRole(member, roleId);
+        const shouldHaveRole = desiredMemberIds.has(member.id);
+
+        if (!shouldHaveRole && hasRole) changes.removeMemberIds.push(member.id);
+    }
+
+    return changes;
+}

src/schemas/uncategorised/RoleMembersUpdateSchema.test.ts

@@ -0,0 +1,25 @@
+import assert from "node:assert/strict";
+import { describe, test } from "node:test";
+import { ajv, validateSchema } from "../Validator";
+
+describe("RoleMembersUpdateSchema", () => {
+    test("accepts a member_ids array", () => {
+        assert.deepEqual(validateSchema("RoleMembersUpdateSchema", { member_ids: ["123", "456"] }), { member_ids: ["123", "456"] });
+    });
+
+    test("accepts an empty member_ids array for full replacement clears", () => {
+        assert.deepEqual(validateSchema("RoleMembersUpdateSchema", { member_ids: [] }), { member_ids: [] });
+    });
+
+    test("rejects missing member_ids", () => {
+        assert.equal(ajv.validate("RoleMembersUpdateSchema", {}), false);
+    });
+
+    test("rejects non-array member_ids", () => {
+        assert.equal(ajv.validate("RoleMembersUpdateSchema", { member_ids: "123" }), false);
+    });
+
+    test("rejects additional properties", () => {
+        assert.equal(ajv.validate("RoleMembersUpdateSchema", { member_ids: ["123"], extra: true }), false);
+    });
+});

src/schemas/uncategorised/RoleMembersUpdateSchema.ts

@@ -0,0 +1,5 @@
+import { Snowflake } from "../Identifiers";
+
+export interface RoleMembersUpdateSchema {
+    member_ids: Snowflake[];
+}

src/schemas/uncategorised/index.ts

@@ -67,6 +67,7 @@ export * from "./RelationshipPatchSchema";
 export * from "./RelationshipPutSchema";
 export * from "./RequestGuildMembersSchema";
 export * from "./RoleModifySchema";
+export * from "./RoleMembersUpdateSchema";
 export * from "./RolePositionUpdateSchema";
 export * from "./SelectProtocolSchema";
 export * from "./SettingsProtoUpdateSchema";