From 4a06dddf85a24088edb5f5ff7fb95eb9d9c1f61f Mon Sep 17 00:00:00 2001 From: Erik Arvidsson Date: Fri, 11 Oct 2024 16:36:41 +0200 Subject: [PATCH] chore(zero): Change the CRUD protocol (#2648) to pass primaryKey and value instead of id and value. The value already contains the id so all we really need is the primaryKey. --- .../src/services/mutagen/mutagen.test.ts | 84 ++++++++++--------- .../src/services/mutagen/mutagen.ts | 43 +++++----- .../services/mutagen/write-authorizer.test.ts | 5 +- .../src/services/mutagen/write-authorizer.ts | 45 +++++++--- packages/zero-client/src/client/crud.ts | 80 ++++++++---------- packages/zero-client/src/client/keys.ts | 17 +++- packages/zero-protocol/src/data.ts | 14 ++++ packages/zero-protocol/src/push.ts | 31 ++++--- 8 files changed, 180 insertions(+), 139 deletions(-) create mode 100644 packages/zero-protocol/src/data.ts diff --git a/packages/zero-cache/src/services/mutagen/mutagen.test.ts b/packages/zero-cache/src/services/mutagen/mutagen.test.ts index 82e5f4b590..a6628dbef2 100644 --- a/packages/zero-cache/src/services/mutagen/mutagen.test.ts +++ b/packages/zero-cache/src/services/mutagen/mutagen.test.ts @@ -1,11 +1,11 @@ import {resolver} from '@rocicorp/resolver'; import {afterEach, beforeEach, describe, expect, test} from 'vitest'; -import {Mode} from '../../db/transaction-pool.js'; import { ErrorKind, MutationType, type CRUDMutation, } from '../../../../zero-protocol/src/mod.js'; +import {Mode} from '../../db/transaction-pool.js'; import {expectTables, testDBs} from '../../test/db.js'; import type {PostgresDB} from '../../types/pg.js'; import {processMutation} from './mutagen.js'; @@ -109,9 +109,9 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: {id: '1'}, }, ], }, @@ -161,9 +161,9 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: {id: '1'}, }, ], }, @@ -211,9 +211,9 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: {id: '1'}, }, ], }, @@ -263,9 +263,9 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, // This would result in a duplicate key value if applied. - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: {id: '1'}, // This would result in a duplicate key value if applied. }, ], }, @@ -314,9 +314,9 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: {id: '1'}, }, ], }, @@ -368,9 +368,9 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: {id: '1'}, }, ], }, @@ -422,9 +422,9 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: {id: '1'}, }, ], }, @@ -469,42 +469,47 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'id_and_cols', - id: {id: '1'}, + tableName: 'id_and_cols', + primaryKey: ['id'], value: { + id: '1', col1: 'create', col2: 'create', }, }, { op: 'set', - entityType: 'id_and_cols', - id: {id: '2'}, + tableName: 'id_and_cols', + primaryKey: ['id'], value: { + id: '2', col1: 'set', col2: 'set', }, }, { op: 'update', - entityType: 'id_and_cols', - id: {id: '1'}, - partialValue: { + tableName: 'id_and_cols', + primaryKey: ['id'], + value: { + id: '1', col1: 'update', }, }, { op: 'set', - entityType: 'id_and_cols', - id: {id: '1'}, + tableName: 'id_and_cols', + primaryKey: ['id'], value: { + id: '1', col2: 'set', }, }, { op: 'delete', - entityType: 'id_and_cols', - id: {id: '2'}, + tableName: 'id_and_cols', + primaryKey: ['id'], + value: {id: '2'}, }, ], }, @@ -554,9 +559,10 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'fk_ref', - id: {id: '1'}, + tableName: 'fk_ref', + primaryKey: ['id'], value: { + id: '1', ref: '1', }, }, @@ -622,9 +628,11 @@ describe('processMutation', () => { ops: [ { op: 'create', - entityType: 'idonly', - id: {id: '1'}, - value: {}, + tableName: 'idonly', + primaryKey: ['id'], + value: { + id: '1', + }, }, ], }, diff --git a/packages/zero-cache/src/services/mutagen/mutagen.ts b/packages/zero-cache/src/services/mutagen/mutagen.ts index 30c55c739f..9f12ab706c 100644 --- a/packages/zero-cache/src/services/mutagen/mutagen.ts +++ b/packages/zero-cache/src/services/mutagen/mutagen.ts @@ -4,7 +4,12 @@ import {resolver} from '@rocicorp/resolver'; import type {JWTPayload} from 'jose'; import postgres from 'postgres'; import {assert, unreachable} from '../../../../shared/src/asserts.js'; +import * as v from '../../../../shared/src/valita.js'; import {ErrorKind} from '../../../../zero-protocol/src/mod.js'; +import { + primaryKeyValueSchema, + type PrimaryKeyValue, +} from '../../../../zero-protocol/src/primary-key.js'; import { MutationType, type CRUDMutation, @@ -302,27 +307,17 @@ export function getCreateSQL( tx: postgres.TransactionSql, create: CreateOp, ): postgres.PendingQuery { - const table = create.entityType; - const {id, value} = create; - - const valueWithIdColumns = { - ...value, - ...id, - }; - - return tx`INSERT INTO ${tx(table)} ${tx(valueWithIdColumns)}`; + return tx`INSERT INTO ${tx(create.tableName)} ${tx(create.value)}`; } export function getSetSQL( tx: postgres.TransactionSql, set: SetOp, ): postgres.PendingQuery { - const table = set.entityType; - const {id, value} = set; - + const {tableName, primaryKey, value} = set; return tx` - INSERT INTO ${tx(table)} ${tx({...value, ...id})} - ON CONFLICT (${tx(Object.keys(id))}) + INSERT INTO ${tx(tableName)} ${tx(value)} + ON CONFLICT (${tx(primaryKey)}) DO UPDATE SET ${tx(value)} `; } @@ -331,28 +326,30 @@ function getUpdateSQL( tx: postgres.TransactionSql, update: UpdateOp, ): postgres.PendingQuery { - const table = update.entityType; - const {id, partialValue} = update; - - return tx`UPDATE ${tx(table)} SET ${tx(partialValue)} WHERE ${tx(id)}`; + const table = update.tableName; + const {primaryKey, value} = update; + const id: Record = {}; + for (const key of primaryKey) { + id[key] = v.parse(value[key], primaryKeyValueSchema); + } + return tx`UPDATE ${tx(table)} SET ${tx(value)} WHERE ${tx(id)}`; } function getDeleteSQL( tx: postgres.TransactionSql, deleteOp: DeleteOp, ): postgres.PendingQuery { - const table = deleteOp.entityType; - const {id} = deleteOp; + const {tableName, primaryKey, value} = deleteOp; const conditions = []; - for (const [key, value] of Object.entries(id)) { + for (const key of primaryKey) { if (conditions.length > 0) { conditions.push(tx`AND`); } - conditions.push(tx`${tx(key)} = ${value}`); + conditions.push(tx`${tx(key)} = ${value[key]}`); } - return tx`DELETE FROM ${tx(table)} WHERE ${conditions}`; + return tx`DELETE FROM ${tx(tableName)} WHERE ${conditions}`; } async function checkSchemaVersionAndIncrementLastMutationID( diff --git a/packages/zero-cache/src/services/mutagen/write-authorizer.test.ts b/packages/zero-cache/src/services/mutagen/write-authorizer.test.ts index 0f66c61d24..7c184532fb 100644 --- a/packages/zero-cache/src/services/mutagen/write-authorizer.test.ts +++ b/packages/zero-cache/src/services/mutagen/write-authorizer.test.ts @@ -180,8 +180,9 @@ describe('can insert/update/delete/upsert', () => { (['Insert', 'Update', 'Delete', 'Upsert'] as const)) { expect( authorizer[`can${op}`](jwtPayload, { - id: {id: id ?? 1}, - entityType: 'foo', + tableName: 'foo', + primaryKey: ['id'] as const, + value: {id: id ?? 1}, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any), ).toBe(expected); diff --git a/packages/zero-cache/src/services/mutagen/write-authorizer.ts b/packages/zero-cache/src/services/mutagen/write-authorizer.ts index 2767ad0a48..387c2cfd61 100644 --- a/packages/zero-cache/src/services/mutagen/write-authorizer.ts +++ b/packages/zero-cache/src/services/mutagen/write-authorizer.ts @@ -1,3 +1,4 @@ +import type {SQLQuery} from '@databases/sql'; import {LogContext} from '@rocicorp/logger'; import type {JWTPayload} from 'jose'; import {tmpdir} from 'node:os'; @@ -6,14 +7,20 @@ import {pid} from 'node:process'; import {assert} from '../../../../shared/src/asserts.js'; import type {JSONValue} from '../../../../shared/src/json.js'; import {randInt} from '../../../../shared/src/rand.js'; +import * as v from '../../../../shared/src/valita.js'; import type { CreateOp, DeleteOp, SetOp, UpdateOp, } from '../../../../zero-protocol/src/mod.js'; +import { + primaryKeyValueSchema, + type PrimaryKeyValue, +} from '../../../../zero-protocol/src/primary-key.js'; import type {BuilderDelegate} from '../../../../zql/src/zql/builder/builder.js'; import {buildPipeline} from '../../../../zql/src/zql/builder/builder.js'; +import {MissingParameterError} from '../../../../zql/src/zql/builder/error.js'; import type {Row} from '../../../../zql/src/zql/ivm/data.js'; import {Database} from '../../../../zqlite/src/db.js'; import {compile, sql} from '../../../../zqlite/src/internal/sql.js'; @@ -29,7 +36,6 @@ import {mapLiteDataTypeToZqlSchemaValue} from '../../types/lite.js'; import {DatabaseStorage} from '../view-syncer/database-storage.js'; import type {NormalizedTableSpec} from '../view-syncer/pipeline-driver.js'; import {normalize} from '../view-syncer/pipeline-driver.js'; -import {MissingParameterError} from '../../../../zql/src/zql/builder/error.js'; export interface WriteAuthorizer { canInsert(authData: JWTPayload, op: CreateOp): boolean; @@ -89,16 +95,16 @@ export class WriteAuthorizerImpl { if (preMutationRow) { return this.canUpdate(authData, { op: 'update', - entityType: op.entityType, - id: op.id, - partialValue: op.value, + tableName: op.tableName, + primaryKey: op.primaryKey, + value: op.value, }); } return this.canInsert(authData, { op: 'create', - entityType: op.entityType, - id: op.id, + tableName: op.tableName, + primaryKey: op.primaryKey, value: op.value, }); } @@ -145,10 +151,10 @@ export class WriteAuthorizerImpl { action, 'duration:', performance.now() - start, - 'entityType:', - op.entityType, - 'id:', - op.id, + 'tableName:', + op.tableName, + 'primaryKey:', + op.primaryKey, ); } } @@ -168,7 +174,7 @@ export class WriteAuthorizerImpl { authData: JWTPayload, op: ActionOpMap[A], ) { - const rules = this.#authorizationConfig[op.entityType]; + const rules = this.#authorizationConfig[op.tableName]; if (!rules) { return true; } @@ -216,9 +222,22 @@ export class WriteAuthorizerImpl { } #getPreMutationRow(op: SetOp | UpdateOp | DeleteOp) { + const {value} = op; + const conditions: SQLQuery[] = []; + const values: PrimaryKeyValue[] = []; + for (const pk of op.primaryKey) { + conditions.push(sql`${sql.ident(pk)}=?`); + values.push(v.parse(value[pk], primaryKeyValueSchema)); + } + return this.#statementCache.use( - compile(sql`SELECT * FROM ${sql.ident(op.entityType)} WHERE id = ?`), - stmt => stmt.statement.get(op.id.id), + compile( + sql`SELECT * FROM ${sql.ident(op.tableName)} WHERE ${sql.join( + conditions, + sql` AND `, + )}`, + ), + stmt => stmt.statement.get(...values), ); } diff --git a/packages/zero-client/src/client/crud.ts b/packages/zero-client/src/client/crud.ts index deba610c89..1c3571ee7a 100644 --- a/packages/zero-client/src/client/crud.ts +++ b/packages/zero-client/src/client/crud.ts @@ -1,9 +1,6 @@ import {promiseVoid} from '../../../shared/src/resolved-promises.js'; import type {MaybePromise} from '../../../shared/src/types.js'; -import { - type PrimaryKeyValue, - type PrimaryKeyValueRecord, -} from '../../../zero-protocol/src/primary-key.js'; +import {type PrimaryKeyValueRecord} from '../../../zero-protocol/src/primary-key.js'; import { CRUD_MUTATION_NAME, type CreateOp, @@ -140,8 +137,8 @@ function makeEntityCRUDMutate( assertNotInBatch(entityType, 'create'); const op: CreateOp = { op: 'create', - entityType, - id: makeIDFromPrimaryKey(primaryKey, value), + tableName: entityType, + primaryKey, value, }; return zeroCRUD({ops: [op]}); @@ -150,8 +147,8 @@ function makeEntityCRUDMutate( assertNotInBatch(entityType, 'set'); const op: SetOp = { op: 'set', - entityType, - id: makeIDFromPrimaryKey(primaryKey, value), + tableName: entityType, + primaryKey, value, }; return zeroCRUD({ops: [op]}); @@ -160,9 +157,9 @@ function makeEntityCRUDMutate( assertNotInBatch(entityType, 'update'); const op: UpdateOp = { op: 'update', - entityType, - id: makeIDFromPrimaryKey(primaryKey, value), - partialValue: value, + tableName: entityType, + primaryKey, + value, }; return zeroCRUD({ops: [op]}); }, @@ -170,8 +167,9 @@ function makeEntityCRUDMutate( assertNotInBatch(entityType, 'delete'); const op: DeleteOp = { op: 'delete', - entityType, - id: makeIDFromPrimaryKey(primaryKey, id), + tableName: entityType, + primaryKey, + value: id, }; return zeroCRUD({ops: [op]}); }, @@ -194,8 +192,8 @@ export function makeBatchCRUDMutate< create: (value: CreateValue) => { const op: CreateOp = { op: 'create', - entityType: tableName, - id: makeIDFromPrimaryKey(primaryKey, value), + tableName, + primaryKey, value, }; ops.push(op); @@ -204,8 +202,8 @@ export function makeBatchCRUDMutate< set: (value: SetValue) => { const op: SetOp = { op: 'set', - entityType: tableName, - id: makeIDFromPrimaryKey(primaryKey, value), + tableName, + primaryKey, value, }; ops.push(op); @@ -214,9 +212,9 @@ export function makeBatchCRUDMutate< update: (value: UpdateValue) => { const op: UpdateOp = { op: 'update', - entityType: tableName, - id: makeIDFromPrimaryKey(primaryKey, value), - partialValue: value, + tableName, + primaryKey, + value, }; ops.push(op); return promiseVoid; @@ -224,8 +222,9 @@ export function makeBatchCRUDMutate< delete: (id: DeleteID) => { const op: DeleteOp = { op: 'delete', - entityType: tableName, - id: makeIDFromPrimaryKey(primaryKey, id), + tableName, + primaryKey, + value: id, }; ops.push(op); return promiseVoid; @@ -274,9 +273,9 @@ async function createImpl( schema: NormalizedSchema, ): Promise { const key = toPrimaryKeyString( - arg.entityType, - schema.tables[arg.entityType].primaryKey, - arg.id, + arg.tableName, + schema.tables[arg.tableName].primaryKey, + arg.value, ); if (!(await tx.has(key))) { await tx.set(key, arg.value); @@ -289,9 +288,9 @@ async function setImpl( schema: NormalizedSchema, ): Promise { const key = toPrimaryKeyString( - arg.entityType, - schema.tables[arg.entityType].primaryKey, - arg.id, + arg.tableName, + schema.tables[arg.tableName].primaryKey, + arg.value, ); await tx.set(key, arg.value); } @@ -302,15 +301,15 @@ async function updateImpl( schema: NormalizedSchema, ): Promise { const key = toPrimaryKeyString( - arg.entityType, - schema.tables[arg.entityType].primaryKey, - arg.id, + arg.tableName, + schema.tables[arg.tableName].primaryKey, + arg.value, ); const prev = await tx.get(key); if (prev === undefined) { return; } - const update = arg.partialValue; + const update = arg.value; const next = {...(prev as object), ...(update as object)}; await tx.set(key, next); } @@ -321,20 +320,9 @@ async function deleteImpl( schema: NormalizedSchema, ): Promise { const key = toPrimaryKeyString( - arg.entityType, - schema.tables[arg.entityType].primaryKey, - arg.id, + arg.tableName, + schema.tables[arg.tableName].primaryKey, + arg.value, ); await tx.del(key); } - -function makeIDFromPrimaryKey( - primaryKey: NormalizedPrimaryKey, - id: PrimaryKeyValueRecord, -): PrimaryKeyValueRecord { - const rv: Record = {}; - for (const key of primaryKey) { - rv[key] = id[key]; - } - return rv; -} diff --git a/packages/zero-client/src/client/keys.ts b/packages/zero-client/src/client/keys.ts index 6e77358390..9e025139d7 100644 --- a/packages/zero-client/src/client/keys.ts +++ b/packages/zero-client/src/client/keys.ts @@ -1,5 +1,7 @@ import {h64WithReverse} from '../../../shared/src/h64-with-reverse.js'; -import type {PrimaryKeyValueRecord} from '../../../zero-protocol/src/primary-key.js'; +import * as v from '../../../shared/src/valita.js'; +import {primaryKeyValueSchema} from '../../../zero-protocol/src/primary-key.js'; +import type {Row} from '../../../zql/src/zql/ivm/data.js'; import type {NormalizedPrimaryKey} from '../../../zql/src/zql/query/normalize-table-schema.js'; export const CLIENTS_KEY_PREFIX = 'c/'; @@ -26,13 +28,20 @@ export function toGotQueriesKey(hash: string): string { export function toPrimaryKeyString( tableName: string, primaryKey: NormalizedPrimaryKey, - id: PrimaryKeyValueRecord, + value: Row, ): string { + // TODO: The type system should have enforced that the value has valid primary keys. + // We should maybe tag the row with the primary key so we can enforce this. if (primaryKey.length === 1) { - return ENTITIES_KEY_PREFIX + tableName + '/' + id[primaryKey[0]]; + return ( + ENTITIES_KEY_PREFIX + + tableName + + '/' + + v.parse(value[primaryKey[0]], primaryKeyValueSchema) + ); } - const values = primaryKey.map(k => id[k]); + const values = primaryKey.map(k => v.parse(value[k], primaryKeyValueSchema)); const str = JSON.stringify(values); const idSegment = h64WithReverse(str); diff --git a/packages/zero-protocol/src/data.ts b/packages/zero-protocol/src/data.ts new file mode 100644 index 0000000000..ce976cdfd6 --- /dev/null +++ b/packages/zero-protocol/src/data.ts @@ -0,0 +1,14 @@ +import * as v from '../../shared/src/valita.js'; +import type {Row, Value} from '../../zql/src/zql/ivm/data.js'; + +export type {Row, Value}; + +export const valueSchema: v.Type = v.union( + v.null(), + v.boolean(), + v.number(), + v.string(), + v.undefined(), +); + +export const rowSchema: v.Type = v.record(valueSchema); diff --git a/packages/zero-protocol/src/push.ts b/packages/zero-protocol/src/push.ts index c6f8766e67..f8b4ab1d0a 100644 --- a/packages/zero-protocol/src/push.ts +++ b/packages/zero-protocol/src/push.ts @@ -1,8 +1,10 @@ -import {jsonObjectSchema, jsonSchema} from '../../shared/src/json-schema.js'; +import {jsonSchema} from '../../shared/src/json-schema.js'; import * as v from '../../shared/src/valita.js'; -import {primaryKeyValueRecordSchema} from './primary-key.js'; +import {rowSchema} from './data.js'; +import {primaryKeySchema, primaryKeyValueRecordSchema} from './primary-key.js'; export const CRUD_MUTATION_NAME = '_zero_crud'; + export enum MutationType { CRUD = 'crud', Custom = 'custom', @@ -13,9 +15,9 @@ export enum MutationType { */ const createOpSchema = v.object({ op: v.literal('create'), - entityType: v.string(), - id: primaryKeyValueRecordSchema, - value: jsonObjectSchema, + tableName: v.string(), + primaryKey: primaryKeySchema, + value: rowSchema, }); /** @@ -24,9 +26,9 @@ const createOpSchema = v.object({ */ const setOpSchema = v.object({ op: v.literal('set'), - entityType: v.string(), - id: primaryKeyValueRecordSchema, - value: jsonObjectSchema, + tableName: v.string(), + primaryKey: primaryKeySchema, + value: rowSchema, }); /** @@ -34,9 +36,10 @@ const setOpSchema = v.object({ */ const updateOpSchema = v.object({ op: v.literal('update'), - entityType: v.string(), - id: primaryKeyValueRecordSchema, - partialValue: jsonObjectSchema, + tableName: v.string(), + primaryKey: primaryKeySchema, + // Partial value with at least the primary key fields + value: rowSchema, }); /** @@ -44,8 +47,10 @@ const updateOpSchema = v.object({ */ const deleteOpSchema = v.object({ op: v.literal('delete'), - entityType: v.string(), - id: primaryKeyValueRecordSchema, + tableName: v.string(), + primaryKey: primaryKeySchema, + // Partial value representing the primary key + value: primaryKeyValueRecordSchema, }); const crudOpSchema = v.union(