Skip to content

Add for support top-level logical operators in SQL builders#181

Open
KKalheber wants to merge 4 commits into
event-driven-io:mainfrom
KKalheber:fix/logical-root-operators-sql
Open

Add for support top-level logical operators in SQL builders#181
KKalheber wants to merge 4 commits into
event-driven-io:mainfrom
KKalheber:fix/logical-root-operators-sql

Conversation

@KKalheber
Copy link
Copy Markdown

Refs #180

Summary

This patch wires top-level logical operators into the current SQL filter builders and reconnects PongoFilter<TSchema> to the supported logical subset.

What changed

  • implement top-level $or, $and, $nor handling in:
    • src/packages/pongo/src/storage/postgresql/core/sqlBuilder/filter/index.ts
    • src/packages/pongo/src/storage/sqlite/core/sqlBuilder/filter/index.ts
  • reconnect PongoFilter<TSchema> to the supported logical subset using:
    • Pick<RootFilterOperators<WithId<TSchema>>, '$and' | '$nor' | '$or'>
  • explicitly reject unsupported root operators ($text, $where, $comment) in the SQL builders instead of letting them fall through as field names
  • add unit coverage for logical operators in both SQL builder test suites

Notes

  • this keeps the existing PG / SQLite builder split instead of introducing a new shared abstraction
  • this keeps unsupported root operators out of the public PongoFilter surface for now

Verification

npm run build
npx vitest run packages/pongo/src/storage/postgresql/core/sqlBuilder/sqlBuilder.unit.spec.ts packages/pongo/src/storage/sqlite/core/sqlBuilder/sqlBuilder.unit.spec.ts

Copy link
Copy Markdown
Contributor

@oskardudycz oskardudycz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general to me, thank you for sending it.

There's one main clarification needed.

Could you also expand e2e tests to verify that those queries work on real dbs?

}

if (filters.length === 0) {
return joinOperator === OR ? SQL`1 = 0` : SQL.EMPTY;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KKalheber, could you expand on the need to add 1 = 0 and 1 = 1 in other places? If it's just to make joining subfilters easier, I'd prefer not to add these additional queries and just omit them in that case.

@oskardudycz oskardudycz changed the title feat(filters): support top-level logical operators in SQL builders Add for support top-level logical operators in SQL builders Apr 22, 2026
@oskardudycz oskardudycz added enhancement New feature or request pongo querying labels Apr 22, 2026
@oskardudycz oskardudycz added this to the 0.17.0 milestone Apr 22, 2026
@KKalheber
Copy link
Copy Markdown
Author

KKalheber commented Apr 22, 2026

Addressed both review points in commit bb2c335.

  • added e2e coverage for top-level $or, nested $and + $or, and top-level $nor in both PostgreSQL and SQLite compatibility suites
  • removed the 1 = 0 / 1 = 1 placeholder branches from the logical SQL builders and updated the unit tests accordingly
  • fixed the lint issues from the previous CI run

Local validation is complete.

On this machine, the prebuilt sqlite3 native addon could not be loaded because it expects a newer glibc than the host provides. To avoid shifting that validation burden upstream, I reran the workflow in a node:24 container and rebuilt sqlite3 from source there before running the remaining suites.

Validated locally:

  • npm ci
  • npm run build:ts
  • npm run lint
  • npm run build
  • npm run test:unit
  • npm rebuild sqlite3 --build-from-source
  • npm run test:int
  • npm run test:e2e

Results:

  • unit: 28 files passed, 605 tests passed
  • integration: 22 files passed, 285 tests passed
  • e2e: 4 files passed, 168 tests passed, 5 skipped

So the change has been exercised locally across build, lint, unit, integration, and e2e; the remaining Actions state is GitHub workflow approval for a fork PR, not missing validation work on the patch itself.

@KKalheber
Copy link
Copy Markdown
Author

On a side node - this is my first open source contribution ever x) Not sure if I have done everything right - just let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request pongo querying

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants