From 4a1d05c7b99863017f897c189f05e8742703b52c Mon Sep 17 00:00:00 2001 From: Erik Arvidsson Date: Tue, 19 Nov 2024 10:20:46 +0100 Subject: [PATCH 01/11] wip(zqlite): Fix LIKE operator I thought that this would work now that we have an ICU build. But it doesn't. I'm going to have to do some more work on this. --- packages/zql/src/builder/like.test.ts | 74 ++++++++++++++++++++++++++- packages/zqlite/src/query.test.ts | 73 +++++++++++++++++++++++--- packages/zqlite/src/table-source.ts | 42 +++++++++++---- 3 files changed, 169 insertions(+), 20 deletions(-) diff --git a/packages/zql/src/builder/like.test.ts b/packages/zql/src/builder/like.test.ts index acb23301f7..977e7f9277 100644 --- a/packages/zql/src/builder/like.test.ts +++ b/packages/zql/src/builder/like.test.ts @@ -1,11 +1,13 @@ import {expect, test} from 'vitest'; import {getLikePredicate} from './like.js'; -export const cases: { +type Case = { pattern: string; flags: 'i' | ''; inputs: [string, boolean][]; -}[] = [ +}; + +export const cases: Case[] = [ { pattern: 'foo', flags: '', @@ -175,6 +177,74 @@ export const cases: { }, ]; +const more: Case[] = [ + { + pattern: 'dž', + flags: '', + inputs: [ + ['dž', true], + ['Dž', false], + ['DŽ', false], + ], + }, + { + pattern: 'dž', + flags: 'i', + inputs: [ + ['dž', true], + ['Dž', true], + ['DŽ', true], + ], + }, + + { + pattern: 'Dž', + flags: '', + inputs: [ + ['dž', false], + ['Dž', true], + ['DŽ', false], + ], + }, + { + pattern: 'Dž', + flags: 'i', + inputs: [ + ['dž', true], + ['Dž', true], + ['DŽ', true], + ], + }, + + { + pattern: 'DŽ', + flags: '', + inputs: [ + ['dž', false], + ['Dž', false], + ['DŽ', true], + ], + }, + { + pattern: 'DŽ', + flags: 'i', + inputs: [ + ['dž', true], + ['Dž', true], + ['DŽ', true], + ], + }, +]; + +cases.push(...more); +// Also test with '%' because that triggers the RegExp path. +cases.push( + ...more.map(c => ({ + ...c, + pattern: '%' + c.pattern + '%', + })), +); + test('basics', () => { for (const {pattern, flags, inputs} of cases) { const op = getLikePredicate(pattern, flags); diff --git a/packages/zqlite/src/query.test.ts b/packages/zqlite/src/query.test.ts index f63f63f944..f01a217085 100644 --- a/packages/zqlite/src/query.test.ts +++ b/packages/zqlite/src/query.test.ts @@ -1,15 +1,18 @@ import {beforeEach, expect, test} from 'vitest'; -import {Database} from './db.js'; import {createSilentLogContext} from '../../shared/src/logging-test-utils.js'; -import type {Source} from '../../zql/src/ivm/source.js'; -import {TableSource, toSQLiteTypeName} from './table-source.js'; +import {must} from '../../shared/src/must.js'; +import type {TableSchema} from '../../zero-schema/src/table-schema.js'; import {MemoryStorage} from '../../zql/src/ivm/memory-storage.js'; +import type {Source} from '../../zql/src/ivm/source.js'; import {newQuery, type QueryDelegate} from '../../zql/src/query/query-impl.js'; import {schemas} from '../../zql/src/query/test/testSchemas.js'; -import type {TableSchema} from '../../zero-schema/src/table-schema.js'; -import {must} from '../../shared/src/must.js'; +import {Database} from './db.js'; +import {TableSource, toSQLiteTypeName} from './table-source.js'; let queryDelegate: QueryDelegate; +let userSource: Source; +let issueSource: Source; + beforeEach(() => { const db = new Database(createSilentLogContext(), ':memory:'); const sources = new Map(); @@ -50,8 +53,8 @@ beforeEach(() => { }, }; - const userSource = must(queryDelegate.getSource('user')); - const issueSource = must(queryDelegate.getSource('issue')); + userSource = must(queryDelegate.getSource('user')); + issueSource = must(queryDelegate.getSource('issue')); userSource.push({ type: 'add', @@ -190,3 +193,59 @@ test('null compare', () => { ] `); }); + +test.each([ + {title: 'a', op: 'LIKE', pattern: 'a', expected: true}, + {title: 'a', op: 'ILIKE', pattern: 'a', expected: true}, + + {title: 'A', op: 'LIKE', pattern: 'a', expected: false}, + {title: 'A', op: 'ILIKE', pattern: 'a', expected: true}, + + {title: 'dž', op: 'LIKE', pattern: 'dž', expected: true}, + {title: 'dž', op: 'LIKE', pattern: 'Dž', expected: false}, + {title: 'dž', op: 'LIKE', pattern: 'DŽ', expected: false}, + {title: 'dž', op: 'ILIKE', pattern: 'dž', expected: true}, + {title: 'dž', op: 'ILIKE', pattern: 'Dž', expected: true}, + {title: 'dž', op: 'ILIKE', pattern: 'DŽ', expected: true}, + + {title: 'Dž', op: 'LIKE', pattern: 'dž', expected: false}, + {title: 'Dž', op: 'LIKE', pattern: 'Dž', expected: true}, + {title: 'Dž', op: 'LIKE', pattern: 'DŽ', expected: false}, + {title: 'Dž', op: 'ILIKE', pattern: 'dž', expected: true}, + {title: 'Dž', op: 'ILIKE', pattern: 'Dž', expected: true}, + {title: 'Dž', op: 'ILIKE', pattern: 'DŽ', expected: true}, + + {title: 'DŽ', op: 'LIKE', pattern: 'dž', expected: false}, + {title: 'DŽ', op: 'LIKE', pattern: 'Dž', expected: false}, + {title: 'DŽ', op: 'LIKE', pattern: 'DŽ', expected: true}, + {title: 'DŽ', op: 'ILIKE', pattern: 'dž', expected: true}, + {title: 'DŽ', op: 'ILIKE', pattern: 'Dž', expected: true}, + {title: 'DŽ', op: 'ILIKE', pattern: 'DŽ', expected: true}, +] as const)( + '$title $op $pattern => $expected', + ({title, op, pattern, expected}) => { + issueSource.push({ + type: 'add', + row: { + id: '0004', + title, + description: 'description 4', + closed: false, + ownerId: null, + }, + }); + + const rows = newQuery(queryDelegate, schemas.issue) + .where('title', op, pattern) + .run(); + + expect(rows.length === 1).toEqual(expected); + + // Also test with '%' because that triggers the RegExp path. + const rows2 = newQuery(queryDelegate, schemas.issue) + .where('title', op, '%' + pattern + '%') + .run(); + + expect(rows2.length === 1).toEqual(expected); + }, +); diff --git a/packages/zqlite/src/table-source.ts b/packages/zqlite/src/table-source.ts index d53c05a74d..d2bd9f3c27 100644 --- a/packages/zqlite/src/table-source.ts +++ b/packages/zqlite/src/table-source.ts @@ -9,6 +9,10 @@ import type { } from '../../zero-protocol/src/ast.js'; import type {Row, Value} from '../../zero-protocol/src/data.js'; import type {PrimaryKey} from '../../zero-protocol/src/primary-key.js'; +import type { + SchemaValue, + ValueType, +} from '../../zero-schema/src/table-schema.js'; import {assertOrderingIncludesPK} from '../../zql/src/builder/builder.js'; import type {Change} from '../../zql/src/ivm/change.js'; import { @@ -37,10 +41,6 @@ import type {Stream} from '../../zql/src/ivm/stream.js'; import {Database, Statement} from './db.js'; import {compile, format, sql} from './internal/sql.js'; import {StatementCache} from './internal/statement-cache.js'; -import type { - SchemaValue, - ValueType, -} from '../../zero-schema/src/table-schema.js'; type Connection = { input: Input; @@ -208,7 +208,8 @@ export class TableSource implements Source { assert(idx !== -1, 'Connection not found'); this.#connections.splice(idx, 1); }, - appliedFilters: !transformedFilters.conditionsRemoved, + appliedFilters: + !transformedFilters.conditionsRemoved && !transformedFilters.hasLike, }; const connection: Connection = { @@ -836,32 +837,41 @@ type NoSubqueryCondition = function transformFilters(filters: Condition | undefined): { filters: NoSubqueryCondition | undefined; conditionsRemoved: boolean; + // SQLite does not support LIKE so we apply the filter if it is a LIKE operator. + hasLike: boolean; } { if (!filters) { - return {filters: undefined, conditionsRemoved: false}; + return {filters: undefined, conditionsRemoved: false, hasLike: false}; } switch (filters.type) { case 'simple': - return {filters, conditionsRemoved: false}; + return { + filters, + conditionsRemoved: false, + hasLike: isLikeOp(filters), + }; case 'correlatedSubquery': - return {filters: undefined, conditionsRemoved: true}; + return {filters: undefined, conditionsRemoved: true, hasLike: false}; case 'and': { const transformedConditions = []; + let hasLike = false; for (const cond of filters.conditions) { assert(cond.type === 'simple' || cond.type === 'correlatedSubquery'); if (cond.type === 'simple') { transformedConditions.push(cond); + hasLike ||= isLikeOp(cond); } } const conditionsRemoved = transformedConditions.length !== filters.conditions.length; if (transformedConditions.length === 0) { - return {filters: undefined, conditionsRemoved}; + return {filters: undefined, conditionsRemoved, hasLike}; } if (transformedConditions.length === 1) { return { filters: transformedConditions[0], conditionsRemoved, + hasLike, }; } return { @@ -870,26 +880,36 @@ function transformFilters(filters: Condition | undefined): { conditions: transformedConditions, }, conditionsRemoved, + hasLike, }; } case 'or': { const transformedConditions: NoSubqueryCondition[] = []; let conditionsRemoved = false; + let hasLike = false; for (const cond of filters.conditions) { assert(cond.type !== 'or'); const transformed = transformFilters(cond); if (transformed.filters === undefined) { - return {filters: undefined, conditionsRemoved: true}; + return {filters: undefined, conditionsRemoved: true, hasLike: false}; + } + conditionsRemoved ||= transformed.conditionsRemoved; + if (transformed.filters.type === 'simple') { + hasLike ||= isLikeOp(transformed.filters); } - conditionsRemoved = conditionsRemoved || transformed.conditionsRemoved; transformedConditions.push(transformed.filters); } return { filters: {type: 'or', conditions: transformedConditions}, conditionsRemoved, + hasLike, }; } default: unreachable(filters); } } + +function isLikeOp(cond: SimpleCondition): boolean { + return cond.op === 'LIKE' || cond.op === 'NOT LIKE'; +} From d7bf7836e396ec13112735157c6c38611f3a062d Mon Sep 17 00:00:00 2001 From: Darick Tong <132324914+darkgnotic@users.noreply.github.com> Date: Mon, 18 Nov 2024 09:47:31 -0800 Subject: [PATCH 02/11] fix(zero-cache): flags doc grammar, error message improvements (#3042) --- .../zero-cache/src/config/zero-config.test.ts | 2 +- packages/zero-cache/src/config/zero-config.ts | 15 +++++++++++---- .../change-streamer/pg/schema/shard.pg-test.ts | 2 +- .../services/change-streamer/pg/schema/shard.ts | 2 +- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/zero-cache/src/config/zero-config.test.ts b/packages/zero-cache/src/config/zero-config.test.ts index f5bdb091c7..41e7c9b890 100644 --- a/packages/zero-cache/src/config/zero-config.test.ts +++ b/packages/zero-cache/src/config/zero-config.test.ts @@ -151,7 +151,7 @@ test('zero-cache --help', () => { Automatically wipe and resync the replica when replication is halted. This situation can occur for configurations in which the upstream database provider prohibits event trigger creation, preventing the zero-cache from - being able to correctly replicating schema changes. For such configurations, + being able to correctly replicate schema changes. For such configurations, an upstream schema change will instead result in halting replication with an error indicating that the replica needs to be reset. diff --git a/packages/zero-cache/src/config/zero-config.ts b/packages/zero-cache/src/config/zero-config.ts index 9b7034d88b..f07a8720c2 100644 --- a/packages/zero-cache/src/config/zero-config.ts +++ b/packages/zero-cache/src/config/zero-config.ts @@ -3,7 +3,7 @@ */ import * as v from '../../../shared/src/valita.js'; -import {parseOptions, type Config} from './config.js'; +import {ExitAfterUsage, parseOptions, type Config} from './config.js'; /** * Configures the view of the upstream database replicated to this zero-cache. @@ -242,7 +242,7 @@ export const zeroOptions = { `Automatically wipe and resync the replica when replication is halted.`, `This situation can occur for configurations in which the upstream database`, `provider prohibits event trigger creation, preventing the zero-cache from`, - `being able to correctly replicating schema changes. For such configurations,`, + `being able to correctly replicate schema changes. For such configurations,`, `an upstream schema change will instead result in halting replication with an`, `error indicating that the replica needs to be reset.`, ``, @@ -291,8 +291,15 @@ let loadedConfig: ZeroConfig | undefined; export function getZeroConfig(argv = process.argv.slice(2)): ZeroConfig { if (!loadedConfig) { - const config = parseOptions(zeroOptions, argv, ENV_VAR_PREFIX); - loadedConfig = config; + try { + const config = parseOptions(zeroOptions, argv, ENV_VAR_PREFIX); + loadedConfig = config; + } catch (e) { + if (e instanceof ExitAfterUsage) { + process.exit(0); + } + throw e; + } } return loadedConfig; diff --git a/packages/zero-cache/src/services/change-streamer/pg/schema/shard.pg-test.ts b/packages/zero-cache/src/services/change-streamer/pg/schema/shard.pg-test.ts index 99ec773c55..29d2e65b7d 100644 --- a/packages/zero-cache/src/services/change-streamer/pg/schema/shard.pg-test.ts +++ b/packages/zero-cache/src/services/change-streamer/pg/schema/shard.pg-test.ts @@ -238,7 +238,7 @@ describe('change-source/pg', () => { "Must be superuser to create an event trigger." Proceeding in degraded mode: schema changes will halt replication, - after which the operator is responsible for resyncing the replica.", + requiring the replica to be reset (manually or with --auto-reset).", ], ] `); diff --git a/packages/zero-cache/src/services/change-streamer/pg/schema/shard.ts b/packages/zero-cache/src/services/change-streamer/pg/schema/shard.ts index f166234712..357cb42d72 100644 --- a/packages/zero-cache/src/services/change-streamer/pg/schema/shard.ts +++ b/packages/zero-cache/src/services/change-streamer/pg/schema/shard.ts @@ -218,7 +218,7 @@ export async function setupTablesAndReplication( `Unable to create event triggers for schema change detection:\n\n` + `"${e.hint ?? e.message}"\n\n` + `Proceeding in degraded mode: schema changes will halt replication,\n` + - `after which the operator is responsible for resyncing the replica.`, + `requiring the replica to be reset (manually or with --auto-reset).`, ); } } From e219836bf3a15beffdb479e710d047352f162bab Mon Sep 17 00:00:00 2001 From: Darick Tong <132324914+darkgnotic@users.noreply.github.com> Date: Mon, 18 Nov 2024 10:05:29 -0800 Subject: [PATCH 03/11] chore(zero-cache): disable coverage in continuous integration (#3043) --- packages/zero-cache/vitest.config.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/zero-cache/vitest.config.ts b/packages/zero-cache/vitest.config.ts index b54ace7232..83f96b1022 100644 --- a/packages/zero-cache/vitest.config.ts +++ b/packages/zero-cache/vitest.config.ts @@ -3,13 +3,15 @@ import {config} from '../shared/src/tool/vitest-config.js'; const {define, esbuild} = config; +const ci = process.env['CI'] === 'true' || process.env['CI'] === '1'; + export default defineConfig({ define, esbuild, test: { reporters: 'basic', coverage: { - enabled: true, + enabled: !ci, // Don't run coverage in continuous integration. reporter: [['html'], ['clover', {file: 'coverage.xml'}]], include: ['src/**'], }, From d7ae35ad2f7a4f1276ee6fb55dfc64c2e8f4ab62 Mon Sep 17 00:00:00 2001 From: Greg Baker Date: Mon, 18 Nov 2024 11:12:01 -0700 Subject: [PATCH 04/11] chore(zql): small exists cleanups (#3044) --- packages/zql/src/builder/builder.ts | 2 +- packages/zql/src/query/query-impl.ts | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/zql/src/builder/builder.ts b/packages/zql/src/builder/builder.ts index b02318de52..4c96979797 100644 --- a/packages/zql/src/builder/builder.ts +++ b/packages/zql/src/builder/builder.ts @@ -324,7 +324,7 @@ function gatherCorrelatedSubqueryQueriesFromCondition( return csqs; } -const EXISTS_LIMIT = 5; +const EXISTS_LIMIT = 3; export function assertOrderingIncludesPK( ordering: Ordering, diff --git a/packages/zql/src/query/query-impl.ts b/packages/zql/src/query/query-impl.ts index 144eb4db4a..72291e08a6 100644 --- a/packages/zql/src/query/query-impl.ts +++ b/packages/zql/src/query/query-impl.ts @@ -368,8 +368,6 @@ export abstract class AbstractQuery< query: Query, ) => Query = q => q, ): Condition => { - //++subqueryFilterCount; - const related = this.#schema.relationships[relationship]; assert(related, 'Invalid relationship'); const fieldRelationship = related; From 72b5033fd9951e14ecc2724db07de82006f6422b Mon Sep 17 00:00:00 2001 From: Matthew Wonlaw Date: Mon, 18 Nov 2024 13:16:27 -0500 Subject: [PATCH 05/11] fix(zqlite): do not create a new schema reference each time --- packages/zqlite/src/query.test.ts | 25 +++++++++++++++++++++++++ packages/zqlite/src/table-source.ts | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/zqlite/src/query.test.ts b/packages/zqlite/src/query.test.ts index f01a217085..692bb2e570 100644 --- a/packages/zqlite/src/query.test.ts +++ b/packages/zqlite/src/query.test.ts @@ -194,6 +194,31 @@ test('null compare', () => { `); }); +test('or', () => { + const query = newQuery(queryDelegate, schemas.issue).where(({or, cmp}) => + or(cmp('ownerId', '=', '0001'), cmp('ownerId', '=', '0002')), + ); + const data = query.run(); + expect(data).toMatchInlineSnapshot(` + [ + { + "closed": false, + "description": "description 1", + "id": "0001", + "ownerId": "0001", + "title": "issue 1", + }, + { + "closed": false, + "description": "description 2", + "id": "0002", + "ownerId": "0002", + "title": "issue 2", + }, + ] + `); +}); + test.each([ {title: 'a', op: 'LIKE', pattern: 'a', expected: true}, {title: 'a', op: 'ILIKE', pattern: 'a', expected: true}, diff --git a/packages/zqlite/src/table-source.ts b/packages/zqlite/src/table-source.ts index d2bd9f3c27..8f70e517d2 100644 --- a/packages/zqlite/src/table-source.ts +++ b/packages/zqlite/src/table-source.ts @@ -197,7 +197,7 @@ export class TableSource implements Source { connect(sort: Ordering, optionalFilters?: Condition | undefined) { const transformedFilters = transformFilters(optionalFilters); const input: SourceInput = { - getSchema: () => this.#getSchema(connection), + getSchema: () => schema, fetch: req => this.#fetch(req, connection), cleanup: req => this.#cleanup(req, connection), setOutput: output => { @@ -219,6 +219,7 @@ export class TableSource implements Source { filters: transformedFilters.filters, compareRows: makeComparator(sort), }; + const schema = this.#getSchema(connection); assertOrderingIncludesPK(sort, this.#primaryKey); this.#connections.push(connection); From 33d0d1ccff2c5d922334c5727c42d856c18bf30b Mon Sep 17 00:00:00 2001 From: Matthew Wonlaw Date: Mon, 18 Nov 2024 13:35:15 -0500 Subject: [PATCH 06/11] feat(zql): remove `MissingParameterError` now that we can check against `null`, missing params is not an error. A missing param defaults to a `null` value. To be used in permission code where the `authData` may not exist. --- .../src/services/mutagen/write-authorizer.ts | 42 +++++-------------- packages/zql/src/builder/builder.ts | 8 +--- packages/zql/src/builder/error.ts | 1 - 3 files changed, 12 insertions(+), 39 deletions(-) delete mode 100644 packages/zql/src/builder/error.ts diff --git a/packages/zero-cache/src/services/mutagen/write-authorizer.ts b/packages/zero-cache/src/services/mutagen/write-authorizer.ts index 44d73d2511..73fb48fd17 100644 --- a/packages/zero-cache/src/services/mutagen/write-authorizer.ts +++ b/packages/zero-cache/src/services/mutagen/write-authorizer.ts @@ -21,7 +21,6 @@ import { } from '../../../../zero-protocol/src/primary-key.js'; import type {BuilderDelegate} from '../../../../zql/src/builder/builder.js'; import {buildPipeline} from '../../../../zql/src/builder/builder.js'; -import {MissingParameterError} from '../../../../zql/src/builder/error.js'; import {Database} from '../../../../zqlite/src/db.js'; import {compile, sql} from '../../../../zqlite/src/internal/sql.js'; import {StatementCache} from '../../../../zqlite/src/internal/statement-cache.js'; @@ -240,38 +239,19 @@ export class WriteAuthorizerImpl { } for (const [_, rule] of policy) { + const input = buildPipeline(rule, this.#builderDelegate, { + authData: authData as Record, + preMutationRow, + }); try { - const input = buildPipeline(rule, this.#builderDelegate, { - authData: authData as Record, - preMutationRow, - }); - try { - const res = input.fetch({}); - for (const _ of res) { - // if any row is returned at all, the - // rule passes. - return true; - } - } finally { - input.destroy(); - } - } catch (e) { - if (!(e instanceof MissingParameterError)) { - throw e; + const res = input.fetch({}); + for (const _ of res) { + // if any row is returned at all, the + // rule passes. + return true; } - - // Authorization rules may refer to parameters - // that are missing due to the current login context not having - // those values. E.g., referring to a claim that user does not have. - // In that case, the rule is not applicable. This is ok since - // all rules can only `allow` or `skip`. If we supported - // `deny` rules this would not work. - // - // The way to support `deny` would be to allow `null` and `undefined` - // into ZQL pipelines. Right now `where` clauses cannot take `null` - // or `undefined` values for two reasons: - // 1. We do not have `IS` and `IS NOT` operators in ZQL yet. - // 2. Our predicates do not compare `NULL` and `UNDEFINED` the same way SQL would. + } finally { + input.destroy(); } } diff --git a/packages/zql/src/builder/builder.ts b/packages/zql/src/builder/builder.ts index 4c96979797..f62c0297dc 100644 --- a/packages/zql/src/builder/builder.ts +++ b/packages/zql/src/builder/builder.ts @@ -26,7 +26,6 @@ import type {Input, Storage} from '../ivm/operator.js'; import {Skip} from '../ivm/skip.js'; import type {Source} from '../ivm/source.js'; import {Take} from '../ivm/take.js'; -import {MissingParameterError} from './error.js'; import {createPredicate} from './filter.js'; export type StaticQueryParameters = { @@ -139,12 +138,7 @@ export function bindStaticParameters( staticQueryParameters, 'Static query params do not exist', )[value.anchor]; - assert(anchor !== undefined, `Missing parameter: ${value.anchor}`); - const resolvedValue = anchor[value.field]; - // eslint-disable-next-line eqeqeq - if (resolvedValue == null) { - throw new MissingParameterError(); - } + const resolvedValue = anchor?.[value.field] ?? null; return { type: 'literal', value: resolvedValue as LiteralValue, diff --git a/packages/zql/src/builder/error.ts b/packages/zql/src/builder/error.ts deleted file mode 100644 index d0f55e9fd8..0000000000 --- a/packages/zql/src/builder/error.ts +++ /dev/null @@ -1 +0,0 @@ -export class MissingParameterError extends Error {} From f3afb4155f48d2cd57a26c057dbf83f33ba55253 Mon Sep 17 00:00:00 2001 From: Matthew Wonlaw Date: Mon, 18 Nov 2024 19:41:09 -0500 Subject: [PATCH 07/11] chore(permissions): swap to `StatementRunner` --- .../src/services/mutagen/write-authorizer.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/zero-cache/src/services/mutagen/write-authorizer.ts b/packages/zero-cache/src/services/mutagen/write-authorizer.ts index 73fb48fd17..614bdf0599 100644 --- a/packages/zero-cache/src/services/mutagen/write-authorizer.ts +++ b/packages/zero-cache/src/services/mutagen/write-authorizer.ts @@ -23,7 +23,6 @@ import type {BuilderDelegate} from '../../../../zql/src/builder/builder.js'; import {buildPipeline} from '../../../../zql/src/builder/builder.js'; import {Database} from '../../../../zqlite/src/db.js'; import {compile, sql} from '../../../../zqlite/src/internal/sql.js'; -import {StatementCache} from '../../../../zqlite/src/internal/statement-cache.js'; import {TableSource} from '../../../../zqlite/src/table-source.js'; import type {ZeroConfig} from '../../config/zero-config.js'; import {listTables} from '../../db/lite-tables.js'; @@ -35,6 +34,7 @@ import type { AuthorizationConfig, Policy, } from '../../../../zero-schema/src/compiled-authorization.js'; +import {StatementRunner} from '../../db/statements.js'; export interface WriteAuthorizer { canInsert(authData: JWTPayload, op: InsertOp): boolean; @@ -49,7 +49,7 @@ export class WriteAuthorizerImpl { readonly #builderDelegate: BuilderDelegate; readonly #tableSpecs: Map; readonly #tables = new Map(); - readonly #statementCache: StatementCache; + readonly #statementRunner: StatementRunner; readonly #lc: LogContext; constructor( @@ -75,7 +75,7 @@ export class WriteAuthorizerImpl { this.#tableSpecs = new Map( listTables(replica).map(spec => [spec.name, normalize(spec)]), ); - this.#statementCache = new StatementCache(replica); + this.#statementRunner = new StatementRunner(replica); } canInsert(authData: JWTPayload, op: InsertOp) { @@ -218,14 +218,14 @@ export class WriteAuthorizerImpl { values.push(v.parse(value[pk], primaryKeyValueSchema)); } - return this.#statementCache.use( + return this.#statementRunner.get( compile( sql`SELECT * FROM ${sql.ident(op.tableName)} WHERE ${sql.join( conditions, sql` AND `, )}`, ), - stmt => stmt.statement.get(...values), + ...values, ); } From 846c52918f5707f3b4d876a17aac2510de69020a Mon Sep 17 00:00:00 2001 From: Darick Tong <132324914+darkgnotic@users.noreply.github.com> Date: Mon, 18 Nov 2024 17:03:38 -0800 Subject: [PATCH 08/11] feat(shared): improve parse error message for valita unions (#3049) --- packages/shared/src/valita.test.ts | 88 ++++++++++++-------- packages/shared/src/valita.ts | 69 ++++++++++++++- packages/zero-client/src/client/zero.test.ts | 5 +- 3 files changed, 123 insertions(+), 39 deletions(-) diff --git a/packages/shared/src/valita.test.ts b/packages/shared/src/valita.test.ts index 7250720324..3036b16ed6 100644 --- a/packages/shared/src/valita.test.ts +++ b/packages/shared/src/valita.test.ts @@ -3,38 +3,38 @@ import {assert} from './asserts.js'; import * as v from './valita.js'; import {parse} from './valita.js'; -test('basic', () => { - const t = (s: v.Type, val: unknown, message?: string) => { - const r1 = v.test(val, s); - const r2 = v.testOptional(val, s); - let ex; - try { - const parsed = parse(val, s); - expect(parsed).toBe(val); - - expect(r1.ok).toBe(true); - expect(r1.ok && r1.value).toBe(val); - - expect(r2.ok).toBe(true); - expect(r2.ok && r2.value).toBe(val); - } catch (err) { - ex = err; - } - - if (message !== undefined) { - assert(ex instanceof TypeError); - expect(ex.message).toBe(message); - - expect(r1.ok).toBe(false); - expect(!r1.ok && r1.error).toBe(message); - - expect(r2.ok).toBe(false); - expect(!r2.ok && r2.error).toBe(message); - } else { - expect(ex).toBe(undefined); - } - }; +const t = (s: v.Type, val: unknown, message?: string) => { + const r1 = v.test(val, s); + const r2 = v.testOptional(val, s); + let ex; + try { + const parsed = parse(val, s); + expect(parsed).toBe(val); + + expect(r1.ok).toBe(true); + expect(r1.ok && r1.value).toBe(val); + + expect(r2.ok).toBe(true); + expect(r2.ok && r2.value).toBe(val); + } catch (err) { + ex = err; + } + + if (message !== undefined) { + assert(ex instanceof TypeError); + expect(ex.message).toBe(message); + + expect(r1.ok).toBe(false); + expect(!r1.ok && r1.error).toBe(message); + + expect(r2.ok).toBe(false); + expect(!r2.ok && r2.error).toBe(message); + } else { + expect(ex).toBe(undefined); + } +}; +test('basic', () => { { const s = v.string(); t(s, 'ok'); @@ -225,8 +225,8 @@ test('basic', () => { t(s, [1, 1]); t(s, ['b', 'b']); t(s, [true, true]); - t(s, ['d', true], 'Invalid union value'); - t(s, [1, '1'], 'Invalid union value'); + t(s, ['d', true], 'Invalid union value: ["d",true]'); + t(s, [1, '1'], 'Invalid union value: [1,"1"]'); t(s, {}, 'Expected array. Got object'); t(s, 'a', 'Expected array. Got "a"'); @@ -237,6 +237,28 @@ test('basic', () => { } }); +test('union error message', () => { + const type = v.union( + v.tuple([v.literal('a'), v.object({a: v.string()})]), + v.tuple([v.literal('b'), v.object({b: v.number()})]), + v.tuple([v.literal('c'), v.object({c: v.boolean()})]), + ); + // Test once with the union itself, then with union(union) and union(union(union)) + // to verify recursion. + for (const s of [type, v.union(type), v.union(v.union(type))]) { + t(s, ['a', {a: 'payload'}]); + t(s, ['b', {b: 123}]); + t(s, ['c', {c: true}]); + t(s, ['a', {b: 'not the right field'}], 'Missing property a at 1'); + t( + s, + ['b', {b: 'not a number'}], + 'Expected number at 1.b. Got "not a number"', + ); + t(s, ['c', {c: 1}], 'Expected boolean at 1.c. Got 1'); + } +}); + test('testOptional', () => { const s = v.number().optional(); diff --git a/packages/shared/src/valita.ts b/packages/shared/src/valita.ts index 0f50b50bf3..a4ec7b1ac2 100644 --- a/packages/shared/src/valita.ts +++ b/packages/shared/src/valita.ts @@ -56,7 +56,12 @@ function displayList( return `${expected.slice(0, -2).map(toDisplay).join(', ')}, ${suffix}`; } -function getMessage(err: v.Err | v.ValitaError, v: unknown): string { +function getMessage( + err: v.Err | v.ValitaError, + v: unknown, + schema: v.Type | v.Optional, + mode: ParseOptionsMode | undefined, +): string { const firstIssue = err.issues[0]; const {path} = firstIssue; const atPath = path?.length ? ` at ${path.join('.')}` : ''; @@ -102,7 +107,9 @@ function getMessage(err: v.Err | v.ValitaError, v: unknown): string { )}${atPath}`; case 'invalid_union': - return `Invalid union value${atPath}`; + return schema.name === 'union' + ? getDeepestUnionParseError(v, schema as v.UnionType, mode ?? 'strict') + : `Invalid union value${atPath}`; case 'custom_error': { const {error} = firstIssue; @@ -116,6 +123,57 @@ function getMessage(err: v.Err | v.ValitaError, v: unknown): string { } } +type FailedType = {type: v.Type; err: v.Err}; + +function getDeepestUnionParseError( + value: unknown, + schema: v.UnionType, + mode: ParseOptionsMode, +): string { + const failures: FailedType[] = []; + for (const type of schema.options) { + const r = type.try(value, {mode}); + if (!r.ok) { + failures.push({type, err: r}); + } + } + if (failures.length) { + // compare the first and second longest-path errors + failures.sort(pathCmp); + if (failures.length === 1 || pathCmp(failures[0], failures[1]) < 0) { + return getMessage(failures[0].err, value, failures[0].type, mode); + } + } + // paths are equivalent + try { + const str = JSON.stringify(value); + return `Invalid union value: ${str}`; + } catch (e) { + // fallback if the value could not be stringified + return `Invalid union value`; + } +} + +// Descending-order comparison of Issue paths. +// * [1, 'a'] sorts before [1] +// * [1] sorts before [0] (i.e. errors later in the tuple sort before earlier errors) +function pathCmp(a: FailedType, b: FailedType) { + const aPath = a.err.issues[0].path; + const bPath = b.err.issues[0].path; + if (aPath.length !== bPath.length) { + return bPath.length - aPath.length; + } + for (let i = 0; i < aPath.length; i++) { + if (bPath[i] > aPath[i]) { + return -1; + } + if (bPath[i] < aPath[i]) { + return 1; + } + } + return 0; +} + /** * 'strip' allows unknown properties and removes unknown properties. * 'strict' errors if there are unknown properties. @@ -160,7 +218,10 @@ export function test( ): Result { const res = schema.try(value, mode ? {mode} : undefined); if (!res.ok) { - return {ok: false, error: getMessage(res, value)}; + return { + ok: false, + error: getMessage(res, value, schema, mode), + }; } return res; } @@ -188,7 +249,7 @@ export function testOptional( return res; } const err = new v.ValitaError(res); - return {ok: false, error: getMessage(err, value)}; + return {ok: false, error: getMessage(err, value, schema, mode)}; } /** diff --git a/packages/zero-client/src/client/zero.test.ts b/packages/zero-client/src/client/zero.test.ts index b781e45665..64b1e67f20 100644 --- a/packages/zero-client/src/client/zero.test.ts +++ b/packages/zero-client/src/client/zero.test.ts @@ -25,6 +25,7 @@ import { pushMessageSchema, } from '../../../zero-protocol/src/push.js'; import type {NullableVersion} from '../../../zero-protocol/src/version.js'; +import type {Schema} from '../../../zero-schema/src/mod.js'; import type {WSString} from './http-string.js'; import type {ZeroOptions} from './options.js'; import type {QueryManager} from './query-manager.js'; @@ -48,7 +49,6 @@ import { RUN_LOOP_INTERVAL_MS, type UpdateNeededReason, } from './zero.js'; -import type {Schema} from '../../../zero-schema/src/mod.js'; let realSetTimeout: typeof setTimeout; let clock: sinon.SinonFakeTimers; @@ -2075,7 +2075,8 @@ suite('Invalid Downstream message', () => { const found = r.testLogSink.messages.some(m => m[2].some( - v => v instanceof Error && v.message.includes('Invalid union value.'), + v => + v instanceof Error && v.message.includes('Missing property pokeID'), ), ); expect(found).true; From eeed90984f3cbe946296a14cf09f43aec4e7b1ed Mon Sep 17 00:00:00 2001 From: Aaron Boodman Date: Mon, 18 Nov 2024 17:05:15 -0800 Subject: [PATCH 09/11] Bump version to 0.6.2024111900+a6527c --- apps/zbugs/package.json | 2 +- package-lock.json | 6 +++--- packages/zero/package.json | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/apps/zbugs/package.json b/apps/zbugs/package.json index aef2f89af5..5f62bfa8af 100644 --- a/apps/zbugs/package.json +++ b/apps/zbugs/package.json @@ -22,7 +22,7 @@ "@headlessui/react": "^2.1.8", "@octokit/core": "^6.1.2", "@radix-ui/react-tooltip": "^1.1.4", - "@rocicorp/zero": "0.6.2024111800+725d79", + "@rocicorp/zero": "0.6.2024111900+a6527c", "@schickling/fps-meter": "^0.1.2", "classnames": "^2.5.1", "dotenv": "^16.4.5", diff --git a/package-lock.json b/package-lock.json index 71b488c10e..b62539a38b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -30,7 +30,7 @@ "@headlessui/react": "^2.1.8", "@octokit/core": "^6.1.2", "@radix-ui/react-tooltip": "^1.1.4", - "@rocicorp/zero": "0.6.2024111800+725d79", + "@rocicorp/zero": "0.6.2024111900+a6527c", "@schickling/fps-meter": "^0.1.2", "classnames": "^2.5.1", "dotenv": "^16.4.5", @@ -15523,7 +15523,7 @@ }, "packages/zero": { "name": "@rocicorp/zero", - "version": "0.6.2024111800+725d79", + "version": "0.6.2024111900+a6527c", "dependencies": { "@badrap/valita": "^0.3.11", "@databases/escape-identifier": "^1.0.3", @@ -26351,7 +26351,7 @@ "@radix-ui/react-tooltip": "^1.1.4", "@rocicorp/eslint-config": "^0.5.1", "@rocicorp/prettier-config": "^0.2.0", - "@rocicorp/zero": "0.6.2024111800+725d79", + "@rocicorp/zero": "0.6.2024111900+a6527c", "@schickling/fps-meter": "^0.1.2", "@tailwindcss/forms": "^0.5.0", "@types/js-cookie": "^3.0.6", diff --git a/packages/zero/package.json b/packages/zero/package.json index 8aecf78445..2bbfe3b8b1 100644 --- a/packages/zero/package.json +++ b/packages/zero/package.json @@ -1,6 +1,6 @@ { "name": "@rocicorp/zero", - "version": "0.6.2024111800+725d79", + "version": "0.6.2024111900+a6527c", "scripts": { "build": "rm -rf out && npm run build-server && npm run build-client", "build-client": "tsc -p tsconfig.client.json && tsc-alias -p tsconfig.client.json && node tool/build.js", From c6e5dde0646227c22ae0e71769cd26d19f744eba Mon Sep 17 00:00:00 2001 From: Matthew Wonlaw Date: Mon, 18 Nov 2024 20:12:30 -0500 Subject: [PATCH 10/11] chore(permissions): rename rule -> policy These variables are collections of rules which are policies. --- .../src/services/mutagen/write-authorizer.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/zero-cache/src/services/mutagen/write-authorizer.ts b/packages/zero-cache/src/services/mutagen/write-authorizer.ts index 614bdf0599..a426d37583 100644 --- a/packages/zero-cache/src/services/mutagen/write-authorizer.ts +++ b/packages/zero-cache/src/services/mutagen/write-authorizer.ts @@ -184,23 +184,23 @@ export class WriteAuthorizerImpl { preMutationRow = this.#getPreMutationRow(op); } - const rowRules = rules.row; + const rowPolicies = rules.row; if ( - rowRules && - !this.#passesPolicy(rowRules[action], authData, preMutationRow) + rowPolicies && + !this.#passesPolicy(rowPolicies[action], authData, preMutationRow) ) { return false; } - const cellRules = rules.cell; - if (cellRules) { - for (const [column, rule] of Object.entries(cellRules)) { + const cellPolicies = rules.cell; + if (cellPolicies) { + for (const [column, policy] of Object.entries(cellPolicies)) { if (action === 'update' && op.value[column] === undefined) { // If the column is not being updated, we do not need to check // the column rules. continue; } - if (!this.#passesPolicy(rule[action], authData, preMutationRow)) { + if (!this.#passesPolicy(policy[action], authData, preMutationRow)) { return false; } } From 198294386edb286307bdbec6da55fc2431527bd2 Mon Sep 17 00:00:00 2001 From: Matthew Wonlaw Date: Mon, 18 Nov 2024 20:23:56 -0500 Subject: [PATCH 11/11] feat(zql): `cmpLit` function I tried `lit` on for size but it just felt more awkward. Example: ```ts issue.where(({cmp, lit}) => cmp(lit(22), '<', 42)); // vs issue.where(({cmpLit}) => cmpLit(22, '<', 42)); // 1. cmpLit is less to type // 2. cmpLit doesn't have the confusing asymmetry of only one literal needing to be wrapped in `lit` ``` --- packages/zql/src/query/expression.ts | 15 ++++++- .../zql/src/query/query-impl.query.test.ts | 39 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/packages/zql/src/query/expression.ts b/packages/zql/src/query/expression.ts index 84474ccc30..482d99b114 100644 --- a/packages/zql/src/query/expression.ts +++ b/packages/zql/src/query/expression.ts @@ -101,6 +101,19 @@ export class ExpressionBuilder { return cmp(field, opOrValue, value); } + cmpLit( + left: ASTParameter | LiteralValue, + op: Operator, + right: ASTParameter | LiteralValue, + ): Condition { + return { + type: 'simple', + left: isParameter(left) ? left : {type: 'literal', value: left}, + right: isParameter(right) ? right : {type: 'literal', value: right}, + op, + }; + } + and = and; or = or; not = not; @@ -203,7 +216,7 @@ export function cmp( } function isParameter( - value: ASTParameter | LiteralValue, + value: ASTParameter | LiteralValue | null, ): value is ASTParameter { return ( typeof value === 'object' && (value as {type: string})?.type === 'static' diff --git a/packages/zql/src/query/query-impl.query.test.ts b/packages/zql/src/query/query-impl.query.test.ts index 17169feff6..88f0ea0ce9 100644 --- a/packages/zql/src/query/query-impl.query.test.ts +++ b/packages/zql/src/query/query-impl.query.test.ts @@ -865,3 +865,42 @@ test('null compare', () => { }, ]); }); + +test('literal filter', () => { + const queryDelegate = new QueryDelegateImpl(); + addData(queryDelegate); + + let rows = newQuery(queryDelegate, issueSchema) + .where(({cmpLit}) => cmpLit(true, '=', false)) + .run(); + + expect(rows).toEqual([]); + + rows = newQuery(queryDelegate, issueSchema) + .where(({cmpLit}) => cmpLit(true, '=', true)) + .run(); + + expect(rows).toEqual([ + { + closed: false, + description: 'description 1', + id: '0001', + ownerId: '0001', + title: 'issue 1', + }, + { + closed: false, + description: 'description 2', + id: '0002', + ownerId: '0002', + title: 'issue 2', + }, + { + closed: false, + description: 'description 3', + id: '0003', + ownerId: null, + title: 'issue 3', + }, + ]); +});