feat: composable typed queries, aggregates and composite cursors - #229
Conversation
andrewzolotukhin
left a comment
There was a problem hiding this comment.
One general comment: add JSDoc to all public class members and exported functions, so consumer can understand why do we need them.
| export function query< | ||
| TLocalSchema extends ObjectSchemaBuilder<any, any, any, any, any, any, any> | ||
| >( | ||
| export function query( | ||
| knex: Knex, | ||
| schema: TLocalSchema, | ||
| schema: any, | ||
| baseQuery?: Knex.QueryBuilder | ||
| ): SchemaQueryBuilder<TLocalSchema, QueryResultType<TLocalSchema>> { | ||
| return new SchemaQueryBuilder<TLocalSchema, QueryResultType<TLocalSchema>>( | ||
| knex, | ||
| schema, | ||
| baseQuery | ||
| ); | ||
| ): any { | ||
| if (isTableAlias(schema)) return new AliasedQueryBuilder(knex, schema); | ||
| return new SchemaQueryBuilder(knex, schema, baseQuery); | ||
| } |
There was a problem hiding this comment.
do we miss some typings here? I want to have IDE hints and type checks everywhere, so this looks suspicious. Seems like we downgraded from strongly typed TSourceSchema to any.
There was a problem hiding this comment.
Addressed in dbca5c2. The query() implementation now retains the source schema/alias generics and explicitly returns the corresponding typed builder instead of accepting/returning any. createQuery() now has real overloads and a checked satisfies BoundQuery construction, without the factory cast. Added compile-time regressions for ordinary and aliased calls, optional base queries, both transaction helpers, selected fields, left-join nullability, invalid property access, and ORM re-exports. Local full suite passes: 4,320 tests, no type errors; PostgreSQL transaction cases also pass.
| schema: S, | ||
| name: N | ||
| ): TableAlias<S, N> { | ||
| if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(name)) { |
There was a problem hiding this comment.
maybe let's extract this check to somewhere, I feel that checking the string to be a valid SQL identifier could be a common task.
There was a problem hiding this comment.
Addressed in dbca5c2. Extracted and exported isSqlIdentifier(value: unknown): value is string and reused it in alias(). It intentionally validates the existing conservative ASCII single-name format, not every dialect's identifier grammar; qualified/quoted names and non-string values are rejected without coercion. JSDoc explains that identifier bindings are still required. Added dedicated validation and alias-integration tests, plus type narrowing coverage.
| eq(t.task.ownerId, t.owner.id), | ||
| or(eq(t.task.ownerId, t.owner.id)) |
There was a problem hiding this comment.
I feel that syntax like:
or(eq(t.task.ownerId, t.owner.id),
eq(t.task.ownerId, t.owner.id))
is more expressive, can we support it? With multiple nestings if needed (to support complex conditions).
There was a problem hiding this comment.
Yes—or(eq(...), eq(...)) and arbitrary nested and()/or() groups were already supported by the recursive predicate renderer. In dbca5c2 I replaced the unhelpful example with meaningful alternatives, added a three-level AND/OR SQL-parenthesization regression and a real PostgreSQL result test, and documented the syntax. Empty groups explicitly throw. All 25 PostgreSQL cases pass locally.
| import('@cleverbrush/schema').InferType< | ||
| RelatedSchema<TEntity, K> | ||
| > |
There was a problem hiding this comment.
do we really need this import() here or we can just use import type at the top of the file?
There was a problem hiding this comment.
Addressed in dbca5c2: added one top-level import type { InferType } from '@cleverbrush/schema' and replaced all three inline type-import expressions in this file. No runtime import or behavior change; build and type tests pass.
|
Implemented the review follow-up in dbca5c2, including the package-wide JSDoc request from the review.
Validation is complete: 4,320 tests / 184 files, no type errors; 25 PostgreSQL integration tests. Lint, build and both documentation-site typechecks pass. API docs generate 826 HTML files with 0 errors (242 non-fatal reference/highlighting warnings remain across the monorepo). Both GitHub checks passed on the new commit. Changesets remain minor-only. No Xpenser changes or consumer-proposal document changes are included. No preview/SigNoz applies to this library-only PR; Telegram remains skipped because the notifier requires a preview/deployment URL. Ready for re-review; not merged. |
Original request
Implement point 1 (1.1–1.4: composable typed database queries) from the consumer-experience proposal as a separate Framework PR against
development. Keep the APIs application-agnostic, preserve existing consumers, add documentation/tests, and use minor changesets only. Include every aggregate family and make aggregate output schemas optional.What changed
alias(schema, name), typed multi-table selectors,eq/and/orpredicates, inner/left joins, and inferred flat results (including left-join nullability). Each aliased source retains its scopes and soft-delete filters. Supports bound queries, transactions, grouped projections and SQL-native HAVING.countValue/countDistinctValue/sumValue/avgValue/minValue/maxValuehelpers and matchingaggregate.*expressions for grouped projections. Counts are checked safe integers; sum/avg and exact numeric extrema preserve database text. Optional output schemas replace decoding and infer their output types. Scalar helpers retain filters/transactions while ignoring pagination without mutating their source. Aggregate DTOs are not attached to the ORM identity map.paginateAfter({ orderBy, limit, cursor })supports mixed directions and schema-declared unique tie-breakers. Versioned cursors retain exact database timestamp/numeric values. Required eager relations filter before page limits. Unsupported query shapes and malformed/incompatible cursors fail explicitly; old single-column cursor calls remain unchanged.@cleverbrush/knex-schemaand@cleverbrush/orm.No Xpenser code, dependency versions, production deployment, or local consumer-experience document changes are included.
Review follow-up
Addressed all four inline comments and the package-wide JSDoc request in
dbca5c28:query()implementation generic over the source schema and alias, with an explicit typed return. GivecreateQuery()real overloads and usesatisfies BoundQueryinstead of casting the bound factory. Type regressions cover ordinary/aliased queries, base queries, transactions, inferred projections, nullable joins, invalid fields, and ORM re-exports.isSqlIdentifier(value)and reuse it for alias validation. Document its conservative ASCII single-identifier contract; test valid names, malformed strings, and non-string inputs.and(...or(...and(...)))predicates in consumer docs and SQL tests, and assert their actual PostgreSQL results. The existing predicate engine already supports this syntax, so no redundant runtime rewrite was needed.import('@cleverbrush/schema').InferTypeuses indbset.tswith one top-level type import.Changeset bumps remain minor. The local consumer-experience proposal is untouched and is not committed.
Reasoning
These additions remove repeated consumer-side SQL metadata/conversion plumbing without introducing application policy or replacing existing ORM APIs. Nested eager loading and flat joins remain separate tools with explicit cardinality. Precision-safe defaults avoid silent numeric/date round-trips; applications can opt into their own output parser.
Cursors are positions, not authorization or snapshots: consumers must reapply access filters and choose indexes. This PR makes no measured Xpenser performance claim; application adoption and representative performance checks follow publication.
Type of Change
Blog post
Skipped the product blog: this is a Framework library/API change, with consumer-facing package and API-site documentation instead. No Xpenser feature is being shipped in this PR.
Screenshots / preview evidence
Not applicable: no application UI change or PR preview deployment is configured in this repository. Executable consumer/type examples and isolated PostgreSQL tests exercise the library behavior directly.
Integration coverage includes scoped left joins, grouped HAVING, all aggregate families and null/empty results, exact large decimals, parent/child cardinality, bound ordering and projection aliases, tracking isolation, transaction visibility, tied microsecond timestamps, mixed cursor directions, composite keys, required relations, and filter-scope preservation.
Validation
npm ci --no-audit --no-fundnpm run lintnpm run buildnpm run typecheck:schema-sitenpm run typecheck:docs-sitenpm run test— 4,320 tests / 184 files, no type errorsnpm run test:queries:integrationwith isolated PostgreSQL 16 — 25 passednpm run docs— generated 826 HTML files, 0 errors; 242 non-fatal TypeDoc reference/highlighting warnings remain across the monorepo.git diff --checknpm pack --workspace @cleverbrush/knex-schema --dry-run --json— consumer guide included in published filesdbca5c28dbca5c28Checklist
npm run lintand fixed any issuesnpm run testand all tests pass