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/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-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).`, ); } } diff --git a/packages/zero-cache/src/services/mutagen/write-authorizer.ts b/packages/zero-cache/src/services/mutagen/write-authorizer.ts index 44d73d2511..a426d37583 100644 --- a/packages/zero-cache/src/services/mutagen/write-authorizer.ts +++ b/packages/zero-cache/src/services/mutagen/write-authorizer.ts @@ -21,10 +21,8 @@ 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'; import {TableSource} from '../../../../zqlite/src/table-source.js'; import type {ZeroConfig} from '../../config/zero-config.js'; import {listTables} from '../../db/lite-tables.js'; @@ -36,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; @@ -50,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( @@ -76,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) { @@ -185,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; } } @@ -219,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, ); } @@ -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/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/**'], }, 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; 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", diff --git a/packages/zql/src/builder/builder.ts b/packages/zql/src/builder/builder.ts index b02318de52..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, @@ -324,7 +318,7 @@ function gatherCorrelatedSubqueryQueriesFromCondition( return csqs; } -const EXISTS_LIMIT = 5; +const EXISTS_LIMIT = 3; export function assertOrderingIncludesPK( ordering: Ordering, 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 {} 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/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', + }, + ]); +}); 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; diff --git a/packages/zqlite/src/query.test.ts b/packages/zqlite/src/query.test.ts index f63f63f944..692bb2e570 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,84 @@ 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}, + + {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..8f70e517d2 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; @@ -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 => { @@ -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 = { @@ -218,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); @@ -836,32 +838,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 +881,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'; +}