From 756c07b0b0173ef512c1d8cfccbb70fa8e44593a Mon Sep 17 00:00:00 2001 From: DJAscendance Date: Thu, 30 Jul 2026 09:16:17 -0400 Subject: [PATCH 1/2] feat: add member_data, the per-member attribute store Lane 1 foundation. In CS 4.x this is the MD / memdata table, keyed by member id, and it is where the buddy list actually lives -- ten slots BU0..BU9 holding NICKNAMES, settled by writing a buddy on a live 4.1 server and diffing the data files. Buddies are not a join table and not in groups/groupmem; those back the Group entries in the access-rights model instead. sqserver.sql names this table Member_Data. Generic key/value rather than a column per feature, because that is what the original is: buddy slots, the hide-yourself privacy flag (IMS) and similar state are all named attributes there. Adding a feature should not need a migration. Deliberately NOT normalised into a friend table. The ten-slot nickname-keyed shape is the fidelity target, and "improving" it into (member_id, friend_member_id) rows would lose two behaviours the original has: a slot can name someone who does not exist or who later renames, and the slot INDEX is meaningful and stable. Two details worth keeping: - An empty or null value DELETES the row rather than storing ''. Otherwise "unset" has two representations and a cleared buddy slot reads back as '' from one path and null from another. - getByPrefix escapes LIKE metacharacters, so getByPrefix('X_') cannot also match 'XA1'. Reading a whole family ('BU') is the normal access pattern and a caller should not be able to widen it accidentally. value is text, not json: MySQL 5.7 is the target and every value the original stores here is a short scalar. Verified against a throwaway MySQL 5.7: upsert replaces rather than duplicating (1 row, not 2); set('') leaves 0 rows and reads back null; ten BU slots round-trip and a BU prefix read excludes IMS; getByPrefix('X_') returns X_1 and not XA1; setMany applies a mixed set+unset atomically. No unit spec: this layer is thin query glue, so mocking knex would mostly assert the mock. The behaviour that matters -- upsert, unset semantics, prefix escaping -- was exercised against a real database instead. Services built on it (buddy list, privacy flag) carry real logic and will get specs. --- .../20260730140000_create_member_data.ts | 57 +++++++++ api/src/db/db.class.ts | 3 + api/src/repositories/index.ts | 1 + .../member-data/member-data.repository.ts | 117 ++++++++++++++++++ api/src/types/models/index.ts | 1 + api/src/types/models/member-data.model.ts | 16 +++ 6 files changed, 195 insertions(+) create mode 100644 api/db/migrations/20260730140000_create_member_data.ts create mode 100644 api/src/repositories/member-data/member-data.repository.ts create mode 100644 api/src/types/models/member-data.model.ts diff --git a/api/db/migrations/20260730140000_create_member_data.ts b/api/db/migrations/20260730140000_create_member_data.ts new file mode 100644 index 00000000..502f29b5 --- /dev/null +++ b/api/db/migrations/20260730140000_create_member_data.ts @@ -0,0 +1,57 @@ +import { Knex } from 'knex'; + +/** + * Per-member named attributes -- the substrate the social layer hangs off. + * + * In CS 4.x this is the MD / memdata table, keyed by member id, and it is where the + * buddy list actually lives: ten slots BU0..BU9 holding NICKNAMES, settled by writing a + * buddy on a live 4.1 server and diffing the data files. Buddies are NOT a join table and + * NOT in groups/groupmem -- those back the Group entries in the access-rights model + * instead. sqserver.sql names this table Member_Data. + * + * A generic key/value store rather than a column per feature, because that is what the + * original is: buddies (BU0..BU9), the hide-yourself privacy flag (IMS) and the per-place + * defaults all live here as named attributes. Adding a feature should not need a + * migration. + * + * Deliberately NOT normalised into a friend table. A ten-slot nickname-keyed list is the + * fidelity target, and "improving" it into (member_id, friend_member_id) rows would lose + * two behaviours the original has: a buddy slot can name someone who does not exist (or + * who later renames), and the slot INDEX is meaningful and stable. + * + * `value` is text, not json: MySQL 5.7 is the deployment target, its JSON support is + * weaker than 8.0's, and every value the original stores here is a short scalar anyway. + */ + +const COLLATE = 'utf8mb4_unicode_ci'; +const tableName = 'member_data'; + +export async function up(knex: Knex): Promise { + if (await knex.schema.hasTable(tableName)) return; + + console.log(`Creating ${tableName} table`); + await knex.schema.createTable(tableName, table => { + table.collate(COLLATE); + table.increments('id').primary(); + table.timestamps(false, true); + + table.integer('member_id').unsigned().notNullable(); + table.foreign('member_id').references('member.id'); + + // Attribute name, e.g. BU0..BU9 for buddy slots, IMS for the privacy flag. + table.string('name', 32).notNullable(); + table.text('value'); + + // One row per (member, attribute). Writes are upserts against this. + table.unique(['member_id', 'name']); + // Reads are almost always "all attributes for this member", or a prefix scan of one + // family (BU%), so member_id leads. + table.index(['member_id', 'name']); + }); +} + +export async function down(knex: Knex): Promise { + if (!await knex.schema.hasTable(tableName)) return; + console.log(`Dropping ${tableName} table`); + await knex.schema.dropTable(tableName); +} diff --git a/api/src/db/db.class.ts b/api/src/db/db.class.ts index 42a8104e..5190462d 100644 --- a/api/src/db/db.class.ts +++ b/api/src/db/db.class.ts @@ -31,6 +31,9 @@ export class Db { get member() { return this.knex('member'); } + get memberData() { + return this.knex('member_data'); + } get message() { return this.knex('message'); } diff --git a/api/src/repositories/index.ts b/api/src/repositories/index.ts index 7936f133..67a1cd4f 100644 --- a/api/src/repositories/index.ts +++ b/api/src/repositories/index.ts @@ -9,6 +9,7 @@ export * from './hood/hood.repository'; export * from './mall-object/mall-object.repository'; export * from './map-location/map-location.repository'; export * from './member/member.repository'; +export * from './member-data/member-data.repository'; export * from './message/message.repository'; export * from './object/object.repository'; export * from './object-instance/object-instance.repository'; diff --git a/api/src/repositories/member-data/member-data.repository.ts b/api/src/repositories/member-data/member-data.repository.ts new file mode 100644 index 00000000..1cd9de04 --- /dev/null +++ b/api/src/repositories/member-data/member-data.repository.ts @@ -0,0 +1,117 @@ +import { Service } from 'typedi'; + +import { Db } from '../../db'; +import { MemberData } from '../../types/models'; + +/** + * Reads and writes per-member named attributes (the CS 4.x MD / memdata table). + * + * Callers should generally go through a feature-specific service -- the buddy list, the + * privacy flag -- rather than reaching for raw attribute names here, so the meaning of + * each name stays in one place. + */ +@Service() +export class MemberDataRepository { + constructor(private db: Db) {} + + /** Every attribute for a member, as a plain name -> value object. */ + public async getAll(memberId: number): Promise> { + const rows = await this.db.knex('member_data') + .select('name', 'value') + .where('member_id', memberId); + return rows.reduce((acc, row) => { + acc[row.name] = row.value; + return acc; + }, {} as Record); + } + + /** A single attribute value, or null if it is unset. */ + public async get(memberId: number, name: string): Promise { + const row = await this.db.knex('member_data') + .select('value') + .where({ member_id: memberId, name }) + .first(); + return row ? row.value : null; + } + + /** + * Attributes whose name starts with `prefix`, as name -> value. + * + * Used to read a whole family at once, e.g. 'BU' for every buddy slot. The prefix is + * escaped so a caller cannot smuggle LIKE wildcards in and widen the match. + */ + public async getByPrefix( + memberId: number, + prefix: string, + ): Promise> { + const escaped = prefix.replace(/[\\%_]/g, char => `\\${char}`); + const rows = await this.db.knex('member_data') + .select('name', 'value') + .where('member_id', memberId) + .andWhere('name', 'like', `${escaped}%`) + .orderBy('name'); + return rows.reduce((acc, row) => { + acc[row.name] = row.value; + return acc; + }, {} as Record); + } + + /** + * Sets an attribute, replacing any existing value. + * + * A null or empty value DELETES the row rather than storing an empty string, so + * "unset" has exactly one representation. Otherwise a cleared buddy slot could read + * back as '' from one code path and null from another. + */ + public async set(memberId: number, name: string, value: string | null): Promise { + if (value === null || value === undefined || value === '') { + await this.unset(memberId, name); + return; + } + await this.db.knex('member_data') + .insert({ member_id: memberId, name, value }) + .onConflict(['member_id', 'name']) + .merge(['value']); + } + + /** Sets several attributes in one transaction, so a partial write cannot land. */ + public async setMany( + memberId: number, + values: Record, + ): Promise { + const entries = Object.entries(values); + if (!entries.length) return; + + await this.db.knex.transaction(async trx => { + const toDelete = entries + .filter(([, value]) => value === null || value === undefined || value === '') + .map(([name]) => name); + const toUpsert = entries + .filter(([, value]) => !(value === null || value === undefined || value === '')) + .map(([name, value]) => ({ member_id: memberId, name, value: value as string })); + + if (toDelete.length) { + await trx('member_data').where('member_id', memberId).whereIn('name', toDelete).del(); + } + if (toUpsert.length) { + await trx('member_data') + .insert(toUpsert) + .onConflict(['member_id', 'name']) + .merge(['value']); + } + }); + } + + public async unset(memberId: number, name: string): Promise { + await this.db.knex('member_data').where({ member_id: memberId, name }).del(); + } + + /** Raw rows, for callers that need timestamps or ids. */ + public async findByMember(memberId: number): Promise { + return this.db.memberData.where({ member_id: memberId }); + } + + public async removeAllForMember(memberId: number): Promise { + await this.db.knex('member_data').where('member_id', memberId).del(); + } +} diff --git a/api/src/types/models/index.ts b/api/src/types/models/index.ts index f6e807f6..f8527796 100644 --- a/api/src/types/models/index.ts +++ b/api/src/types/models/index.ts @@ -3,6 +3,7 @@ export * from './club-member.model'; export * from './mall.model'; export * from './map-location.model'; export * from './member.model'; +export * from './member-data.model'; export * from './message.model'; export * from './store.model'; export * from './object.model'; diff --git a/api/src/types/models/member-data.model.ts b/api/src/types/models/member-data.model.ts new file mode 100644 index 00000000..9939dd33 --- /dev/null +++ b/api/src/types/models/member-data.model.ts @@ -0,0 +1,16 @@ +import { Model } from './model'; + +/** + * A named per-member attribute, as stored in the db. + * + * The CS 4.x MD / memdata table. Buddy slots (BU0..BU9), the hide-yourself flag (IMS) and + * similar per-member state live here as named attributes rather than as dedicated columns. + */ +export interface MemberData extends Model { + /** ID of the member the attribute belongs to */ + member_id: number; + /** Attribute name, e.g. 'BU0' or 'IMS' */ + name: string; + /** Attribute value; null clears it */ + value: string | null; +} From 895742fb117fd543d369405d2227bb1029de0f89 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 11:18:50 -0400 Subject: [PATCH 2/2] Add MemberDataRepository specs and drop the duplicate index Copilot review of #7. Three of four findings; the fourth did not reproduce. The repository had no spec despite carrying the non-obvious behaviour of this PR -- unset-on-empty, LIKE escaping, and the transactional setMany. member.repository.spec.ts is the precedent, so this follows it. 19 tests: get returning null for a missing row, getAll reducing to name -> value, getByPrefix's escaping (including a literal backslash), set deleting rather than storing an empty value for each of null / '' / undefined, and setMany's upsert half, delete half, mixed batch, no-op on {}, and the fact that it does not issue a pointless delete when nothing is cleared. The spec builds a local chainable mock instead of using @spec/mocks. This repository calls db.knex('member_data') as a FUNCTION and awaits the builder, whereas the shared mockDb exposes knex as a plain object whose builder is not thenable -- awaiting it yields the builder rather than rows. Reshaping a mock that every other repository spec depends on, to add coverage for one new repository, is a worse trade than keeping the change contained here. Two traps worth recording, since both produced confidently wrong tests before they were caught. Returning the thenable builder from an async helper makes the runtime await it, so the caller gets the query result instead of the builder it wanted to assert on -- it is now returned wrapped. And a `result: any = []` default parameter swallows an explicit undefined, so the "no row" test was really asserting against a truthy empty array; useBuilder takes rest args and checks length instead. The migration no longer declares index(['member_id', 'name']) alongside unique(['member_id', 'name']). A UNIQUE constraint IS a btree index in MySQL, and because member_id leads it already serves both "all attributes for this member" and the BU% prefix scan. The second declaration was the same index twice: extra storage and extra write cost on every upsert for no query the optimiser could not already satisfy. set and setMany now type value as `string | null | undefined`. The undefined branch was flagged as unreachable, which is true of the declared type and false of reality: these values arrive from request bodies and dynamically built objects where a missing key is undefined long before it meets a typed boundary. Widening the type is the honest fix; deleting the guard would have removed real runtime protection to satisfy a signature that was wrong. Not reproduced: the report that the setMany delete chain exceeds the 100-character limit. No line in the file is over 100 -- that one is 93 -- and eslint reports nothing on it. Left alone. Verified: tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors. Full suite compared against a stashed baseline test-name by test-name: identical failures, no regressions. Note the new spec is untracked and so was present in both runs; the baseline still covers the migration and repository changes, which were stashed. --- .../20260730140000_create_member_data.ts | 9 +- .../member-data.repository.spec.ts | 205 ++++++++++++++++++ .../member-data/member-data.repository.ts | 11 +- 3 files changed, 220 insertions(+), 5 deletions(-) create mode 100644 api/src/repositories/member-data/member-data.repository.spec.ts diff --git a/api/db/migrations/20260730140000_create_member_data.ts b/api/db/migrations/20260730140000_create_member_data.ts index 502f29b5..f3ed8a3d 100644 --- a/api/db/migrations/20260730140000_create_member_data.ts +++ b/api/db/migrations/20260730140000_create_member_data.ts @@ -43,10 +43,13 @@ export async function up(knex: Knex): Promise { table.text('value'); // One row per (member, attribute). Writes are upserts against this. + // + // This is also the read index: a UNIQUE constraint IS a btree index in MySQL, and + // because member_id leads it already serves "all attributes for this member" and a + // prefix scan of one family (BU%). A separate index(['member_id', 'name']) alongside it + // would be the same index twice -- extra storage and extra write cost on every upsert, + // for no additional query the optimiser could not already satisfy. table.unique(['member_id', 'name']); - // Reads are almost always "all attributes for this member", or a prefix scan of one - // family (BU%), so member_id leads. - table.index(['member_id', 'name']); }); } diff --git a/api/src/repositories/member-data/member-data.repository.spec.ts b/api/src/repositories/member-data/member-data.repository.spec.ts new file mode 100644 index 00000000..15deada7 --- /dev/null +++ b/api/src/repositories/member-data/member-data.repository.spec.ts @@ -0,0 +1,205 @@ +import { Container } from 'typedi'; + +import { Db } from '../../db/db.class'; +import { MemberDataRepository } from './member-data.repository'; + +/** + * A local chainable mock rather than @spec/mocks' shared one. + * + * This repository calls `db.knex('member_data')` as a FUNCTION and awaits the resulting + * builder, whereas the shared mockDb exposes `knex` as a plain object whose builder is not + * thenable -- awaiting it yields the builder itself, not rows. Rather than reshape a mock + * every other repository spec depends on, this keeps the change contained. + */ +function queryBuilder(result: any) { + const calls: any[][] = []; + const qb: any = { + calls, + /** Makes the builder awaitable, which is how knex builders actually behave. */ + then: (resolve: (value: any) => void) => resolve(result), + }; + [ + 'select', 'where', 'andWhere', 'whereIn', 'orderBy', + 'insert', 'onConflict', 'merge', 'del', 'first', + ].forEach(method => { + qb[method] = jest.fn((...args: any[]) => { + calls.push([method, ...args]); + return qb; + }); + }); + return qb; +} + +/** The arguments of the first call to `method`, or undefined if it was never called. */ +const argsOf = (qb: any, method: string): any[] | undefined => + qb.calls.find((call: any[]) => call[0] === method)?.slice(1); + +const called = (qb: any, method: string): boolean => + qb.calls.some((call: any[]) => call[0] === method); + +describe('MemberDataRepository', () => { + const MEMBER = 11; + let repository: MemberDataRepository; + let qb: any; + let knex: any; + + /** + * Rest args rather than a default parameter: `.first()` resolves to undefined when there + * is no row, and a default would turn an explicit useBuilder(undefined) into [], which is + * truthy and would quietly test the opposite of the no-row case. + */ + const useBuilder = (...result: any[]) => { + qb = queryBuilder(result.length ? result[0] : []); + knex.mockReturnValue(qb); + return qb; + }; + + beforeEach(() => { + knex = jest.fn(); + knex.transaction = jest.fn(); + Container.reset(); + Container.set(Db, { knex } as any); + repository = Container.get(MemberDataRepository); + useBuilder([]); + }); + + it('should create', () => { + expect(repository).toBeTruthy(); + }); + + describe('get', () => { + it('returns the stored value', async () => { + useBuilder({ value: 'hello' }); + expect(await repository.get(MEMBER, 'IMS')).toBe('hello'); + }); + + /** Absent and empty must not be distinguishable to callers. */ + it('returns null when there is no row', async () => { + useBuilder(undefined); + expect(await repository.get(MEMBER, 'IMS')).toBeNull(); + }); + }); + + describe('getAll', () => { + it('reduces rows to a name -> value object', async () => { + useBuilder([ + { name: 'IMS', value: '1' }, + { name: 'BU0', value: 'HawK' }, + ]); + expect(await repository.getAll(MEMBER)).toEqual({ IMS: '1', BU0: 'HawK' }); + }); + + it('is an empty object rather than null when nothing is stored', async () => { + expect(await repository.getAll(MEMBER)).toEqual({}); + }); + }); + + describe('getByPrefix', () => { + it('matches on the prefix and orders by name', async () => { + useBuilder([{ name: 'BU0', value: 'a' }]); + await repository.getByPrefix(MEMBER, 'BU'); + expect(argsOf(qb, 'andWhere')).toEqual(['name', 'like', 'BU%']); + expect(called(qb, 'orderBy')).toBe(true); + }); + + /** + * '_' and '%' are LIKE wildcards. Unescaped, a prefix of 'BU_' would match 'BUxx' and a + * caller could widen their own read past the family they asked for. + */ + it('escapes LIKE wildcards in the prefix', async () => { + await repository.getByPrefix(MEMBER, 'B_U%'); + expect(argsOf(qb, 'andWhere')).toEqual(['name', 'like', 'B\\_U\\%%']); + }); + + it('escapes a literal backslash too', async () => { + await repository.getByPrefix(MEMBER, 'B\\U'); + expect(argsOf(qb, 'andWhere')).toEqual(['name', 'like', 'B\\\\U%']); + }); + }); + + describe('set', () => { + it('upserts a real value', async () => { + await repository.set(MEMBER, 'IMS', '1'); + expect(argsOf(qb, 'insert')).toEqual([{ member_id: MEMBER, name: 'IMS', value: '1' }]); + expect(argsOf(qb, 'onConflict')).toEqual([['member_id', 'name']]); + expect(called(qb, 'del')).toBe(false); + }); + + /** + * "Unset" has to have exactly one representation, or a cleared buddy slot reads back as + * '' from one path and null from another. + */ + it.each([ + ['null', null], + ['an empty string', ''], + ['undefined', undefined], + ])('deletes the row when given %s', async (_label, value) => { + await repository.set(MEMBER, 'IMS', value as any); + expect(called(qb, 'del')).toBe(true); + expect(called(qb, 'insert')).toBe(false); + }); + }); + + describe('setMany', () => { + /** + * Runs the transaction callback against a trx that yields the same builder. + * + * Returns the builder WRAPPED, because the builder is thenable: returning it bare from + * an async function makes the runtime await it, so the caller would receive the query + * result ([]) instead of the builder whose calls they want to assert on. + */ + const runTransaction = async (values: Record) => { + const trxBuilder = queryBuilder([]); + const trx: any = jest.fn(() => trxBuilder); + knex.transaction.mockImplementation(async (cb: any) => cb(trx)); + await repository.setMany(MEMBER, values); + return { builder: trxBuilder }; + }; + + it('does nothing at all when given no values', async () => { + await repository.setMany(MEMBER, {}); + expect(knex.transaction).not.toHaveBeenCalled(); + }); + + it('runs inside a transaction so a partial write cannot land', async () => { + await runTransaction({ BU0: 'a' }); + expect(knex.transaction).toHaveBeenCalledTimes(1); + }); + + it('upserts the set values', async () => { + const { builder: trxBuilder } = await runTransaction({ BU0: 'a', BU1: 'b' }); + expect(argsOf(trxBuilder, 'insert')).toEqual([[ + { member_id: MEMBER, name: 'BU0', value: 'a' }, + { member_id: MEMBER, name: 'BU1', value: 'b' }, + ]]); + }); + + it('deletes the cleared ones by name', async () => { + const { builder: trxBuilder } = await runTransaction({ BU0: null, BU1: '', BU2: undefined }); + expect(argsOf(trxBuilder, 'whereIn')).toEqual(['name', ['BU0', 'BU1', 'BU2']]); + expect(called(trxBuilder, 'del')).toBe(true); + }); + + /** A mixed batch has to do both halves, not pick one. */ + it('handles a mix of sets and clears in one call', async () => { + const { builder: trxBuilder } = await runTransaction({ BU0: 'a', BU1: null }); + expect(argsOf(trxBuilder, 'whereIn')).toEqual(['name', ['BU1']]); + expect(argsOf(trxBuilder, 'insert')).toEqual([[ + { member_id: MEMBER, name: 'BU0', value: 'a' }, + ]]); + }); + + it('does not issue a delete when nothing is being cleared', async () => { + const { builder: trxBuilder } = await runTransaction({ BU0: 'a' }); + expect(called(trxBuilder, 'del')).toBe(false); + }); + }); + + describe('removeAllForMember', () => { + it('deletes every attribute for the member', async () => { + await repository.removeAllForMember(MEMBER); + expect(argsOf(qb, 'where')).toEqual(['member_id', MEMBER]); + expect(called(qb, 'del')).toBe(true); + }); + }); +}); diff --git a/api/src/repositories/member-data/member-data.repository.ts b/api/src/repositories/member-data/member-data.repository.ts index 1cd9de04..178d3798 100644 --- a/api/src/repositories/member-data/member-data.repository.ts +++ b/api/src/repositories/member-data/member-data.repository.ts @@ -63,7 +63,14 @@ export class MemberDataRepository { * "unset" has exactly one representation. Otherwise a cleared buddy slot could read * back as '' from one code path and null from another. */ - public async set(memberId: number, name: string, value: string | null): Promise { + public async set( + memberId: number, + name: string, + // undefined is accepted alongside null, not dead defensive code: these values come from + // request bodies and dynamically built objects, where a missing key reads as undefined + // long before it reaches a typed boundary. The guard below treats it as "unset". + value: string | null | undefined, + ): Promise { if (value === null || value === undefined || value === '') { await this.unset(memberId, name); return; @@ -77,7 +84,7 @@ export class MemberDataRepository { /** Sets several attributes in one transaction, so a partial write cannot land. */ public async setMany( memberId: number, - values: Record, + values: Record, ): Promise { const entries = Object.entries(values); if (!entries.length) return;