Skip to content

Fast, structured POST .../query/ endpoints for every object type - #913

Merged
DavidMStraub merged 18 commits into
gramps-project:masterfrom
dsblank:feat/object-query-endpoints
Aug 10, 2026
Merged

Fast, structured POST .../query/ endpoints for every object type#913
DavidMStraub merged 18 commits into
gramps-project:masterfrom
dsblank:feat/object-query-endpoints

Conversation

@dsblank

@dsblank dsblank commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

GET /api/people/?page=1 and ?page=999 both take ~8 seconds flat on a 100k-person tree, regardless of page, because the existing endpoints fully deserialize every object in the table before filtering, sorting, or paging any of it in Python — cost scales with table size, not with what's actually requested.

Adds POST /api/<type>/query/ for every primary object type (person, family, event, place, repository, source, citation, media, note, tag): a structured query (select/where/order_by/limit/after, or a where_expr string shorthand) that compiles to a single parameterized SQL statement against the columns Gramps already indexes server-side, with real keyset pagination instead of materialize-then-slice.

Where the pieces live

The compiler, query AST, and expression-language parser live in a separately maintained package, gramps-object-query-language (on PyPI) — it has no Web API-specific concepts in it at all (no Flask, no permissions, nothing HTTP-shaped), just Gramps-object-to-SQL compilation, so there's no reason for it to live in this codebase. This repo owns the policy layer: which columns are selectable, how an HTTP request maps to the query AST, and — the part that matters most — deciding per request whether a query is even allowed to reach the SQL compiler at all.

gramps-object-query-language>=0.1.0 is declared as a normal dependency in pyproject.toml.

The privacy dispatch

Every query request is routed one of two ways, decided per request from the resolved database handle:

  • Full access — compiles straight to SQL via the external compiler, as above.
  • Restricted access (a user without permission to see private records) — never touches the SQL compiler. The same query is evaluated instead through Gramps' own internal record-filtering machinery, so every value a restricted caller can see has already passed through the real, authoritative privacy rules rather than a second implementation of them living in this endpoint. Slower for that path, but correct by construction, and it holds for any future kind of access restriction this project adds later, without this endpoint needing to know what that restriction even is.

Benchmark

Measured live against a real 100k-person tree, comparing this PR's POST /api/people/query/ (gender == MALE, 42,884 matches) against the existing GET /api/people/ (no filter — always a full scan, regardless of page), on both SQLite and a SharedPostgreSQL instance, for both a full-access user and a restricted user (no permission to view private records):

Endpoint Query Access level Backend Time
GET /api/people/ (none — full scan) Full access SQLite 8.1 s
GET /api/people/ (none — full scan) Full access SharedPostgreSQL 7.0 s
GET /api/people/ (none — full scan) Restricted SQLite 30.6 s
GET /api/people/ (none — full scan) Restricted SharedPostgreSQL 84.3 s
POST /api/people/query/ gender == MALE Full access SQLite 5.4 ms
POST /api/people/query/ gender == MALE Full access SharedPostgreSQL 14.5 ms
POST /api/people/query/ gender == MALE Restricted SQLite 29.7 s
POST /api/people/query/ gender == MALE Restricted SharedPostgreSQL 84.1 s
POST /api/people/query/ (none — full scan) Full access SQLite 0.06 ms
POST /api/people/query/ (none — full scan) Full access SharedPostgreSQL 0.19 ms
POST /api/people/query/ (none — full scan) Restricted SQLite 32.1 s
POST /api/people/query/ (none — full scan) Restricted SharedPostgreSQL 99.3 s

Two things to take from this:

  • Full-access users — the case this PR is primarily for — see a ~1,500x (SQLite) / ~480x (Postgres) speedup on gender == MALE, and effectively unmeasurable-vs-8-seconds for the unfiltered case, because the query never deserializes a single object; it's pushed down entirely to SQL.
  • Restricted users land on roughly the same order of magnitude as the existing GET endpoint, not worse — both paths pay the same underlying per-object privacy-sanitization cost (see "The privacy dispatch" above), since neither one takes the SQL shortcut for this case.

Does limit change any of this? Tested limit=1 vs limit=50 vs limit=1000 on the unfiltered query, both paths: the SQL path stays flat (sub-millisecond throughout, both backends) because LIMIT is compiled directly into the SQL statement. The restricted path is also flat regardless of limit (32–35s SQLite, 95–99s Postgres, no consistent trend with limit size) — but for a less happy reason: it evaluates and sanitizes every row in the table before limit is ever applied, so asking for 1 row costs the same as asking for 1000.

Test plan

  • tests/test_object_query_parsing.py — pure unit tests for the endpoint's request-parsing helpers
  • tests/test_endpoints/test_object_query.py — per-type endpoint tests
  • tests/test_endpoints/test_people_query.py — person-endpoint tests, including both the full-access and restricted-access dispatch paths
  • All passing locally against gramps-object-query-language installed from PyPI

🤖 Generated with Claude Code

GET /api/people/?page=1 and ?page=999 both take ~8 seconds flat on a
100k-person tree, regardless of page, because the existing endpoints
fully deserialize every object in the table before filtering, sorting,
or paging any of it in Python -- cost scales with table size, not with
what's actually requested.

Adds POST /api/<type>/query/ for every primary object type: a
structured query (select/where/order_by/limit/after, or a where_expr
string shorthand) that compiles to a single parameterized SQL statement
against the columns Gramps already indexes server-side, with real
keyset pagination instead of materialize-then-slice.

The compiler, AST, and expression parser live in the separately
maintained gramps-object-query-language package (not yet on PyPI, see
its own repo) rather than in this codebase -- it has no Web API-specific
concepts in it (no Flask, no permissions, nothing HTTP-shaped), just
Gramps-object-to-SQL compilation, so there was no reason to fold it in
here. This repo owns the policy: which columns are selectable, how a
request maps to that AST, and -- the part that matters most -- deciding
per request whether the query is even allowed to reach the SQL compiler
at all.

That decision is the core of object_query.py's dispatch: a request from
a user with full access compiles straight to SQL, as above; a request
from a restricted user (someone without permission to see private
records) never touches the SQL compiler at all -- it's evaluated
instead through Gramps' own internal record-filtering machinery, so
every value a restricted caller can see has already passed through the
real, authoritative privacy rules rather than a second implementation
of them living in this endpoint. Slower for that path, but correct by
construction, and it holds for any future kind of access restriction
this project adds later without this endpoint needing to know what
that restriction even is.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread gramps_webapi/api/util.py Outdated
@dsblank
dsblank marked this pull request as ready for review August 2, 2026 01:27
dsblank and others added 8 commits August 1, 2026 22:31
gramps-object-query-language is now published on PyPI, which was the
reason the previous CI run failed (pyproject.toml depends on
gramps-object-query-language>=0.1.0, unresolvable before it was
published). No code change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mypy couldn't infer the type of `lambda obj, c=column: ...` used as
list.sort()'s key= -- the defaulted second parameter (the usual trick
for capturing a loop variable by value) breaks its unification with
sort()'s expected single-argument callable ("Cannot infer type of
lambda"). Replaced with _sort_key_for_column(), a small closure factory
with explicit annotations, which sidesteps the inference issue entirely
and needs no default-argument trick since the closure captures `column`
directly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Previous commit added the typed replacement but never swapped the call
site over to it, so the exact lambda mypy was complaining about was
still there (just shifted a few lines down) -- caught by CI re-running
and reporting the same error at the new line number.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit dropped it from `dependencies` while mistakenly
adding it to `tool.setuptools.packages.find` as if it were vendored
in this repo, breaking imports in object_query.py on CI.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Narrow _default_key_for's parameter type to str | JsonPath |
  RelatedObject, matching what resolve_column_path actually returns
  (never CollectionCount/FlatColumnRef), fixing two union-attr errors
  on .segments.
- Pass a list to where_list_to_ast in _build_where (it expects
  List[dict], not Sequence[dict]).
- Restore the _sort_key_for_column closure-factory helper, lost when
  08289de synced object_query.py from the gramps-object-query-language
  library and reintroduced the lambda obj, c=column: ... pattern mypy
  can't infer a type for.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…aller

_parse_column_ref returns the full ColumnRef union (shared with the
"count_of" branch), so mypy couldn't see that the "json_path" branch's
result is always narrower (str | JsonPath | RelatedObject, per
_default_key_for's own contract). cast() it at the one call site
instead of widening _default_key_for back out, since a CollectionCount/
FlatColumnRef genuinely can't reach it there.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ref.field is typed as the full ColumnRef union upstream, but the
recursive call only ever receives str/JsonPath/RelatedObject.
@dsblank

dsblank commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

I've tested these endpoints in combination with the latest gramps-object-query-language and everything is working as designed (with a limitation in the GOQL, see below).

Screenshot from 2026-08-03 08-22-52

Resources:

Limitation in GOQL:

  • Currently, when you return data from a proxied database, it does not respect the requested page number, nor sort order. The fix for respecting page will just be to return a slice of the returns, as with the other endpoints. The sort order is a an easy fix.

Everything in this PR should be ready.

@DavidMStraub

Copy link
Copy Markdown
Member

Thanks! I'm quite busy the next few days, please don't worry if review takes a but longer than usual.

If the sort and paging are just one-liners, should they be in this PR?

@dsblank

dsblank commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

If the sort and paging are just one-liners, should they be in this PR?

They don't live in this PR. They are fixes for gramps-object-query-language and I'll fix as soon as a get a few minutes. But this PR won't be affected nor will it change.

run_query() (gramps-object-query-language) now natively supports order_by/
limit/after/select, so _post_proxied's own hand-rolled workarounds for that
gap are no longer needed: the full Python re-sort of every match
(_sort_key/_sort_key_for_column), the manual linear-scan cursor resolution,
and the manual limit slicing. Replaced with a direct run_query(..., order_by=,
limit=, after=, select=) call, mirroring _post_sql's own response-building
shape. Adds _resolve_after_proxied, the evaluator-path counterpart to
_resolve_after, to turn a client's after=<handle> into the value tuple
run_query's keyset seek expects.

The X-Total-Count header now costs a genuinely separate run_query call, gated
behind the count flag -- matching _post_sql's own documented "second query,
opt-in" contract, which the old code didn't actually honor (it built the full
sorted match list unconditionally regardless of whether count was requested).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dsblank

dsblank commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

I've updated gramps-object-query-language and it did allow some simplifications here. Updated, and ready!

@DavidMStraub

Copy link
Copy Markdown
Member

Thanks!

Issues identified in local Opus review:


Bugs

1. Dependency floor is wrong — breaks the restricted-user path

pyproject.toml pins gramps-object-query-language>=0.3.1, but _post_proxied calls
run_query(..., order_by=, limit=, after=, select=). Those kwargs do not exist until 0.3.3:

installed version floor: 0.3.1
run_query signature: (db, spec, where) -> List[Any]
call as _post_proxied makes it -> TypeError: run_query() got an unexpected keyword argument 'order_by'

At the floor, every request from a user without PERM_VIEW_PRIVATE returns 500. CI does not
catch this because pip resolves to the latest version. Fix: >=0.3.3, plus a CI job that
resolves at the declared floor.

2. locale is silently ignored on the proxied path

_post_sql applies COLLATE; run_query always sorts in plain Python order. The same request
against the same data therefore returns a different sort order depending on the caller's
permissions. Either implement collation on the proxied path or reject locale there with 422.

Security

3. _post_sql carries no privacy predicate at all

The SQL path relies entirely on the invariant "unproxied database ⟹ caller has
PERM_VIEW_PRIVATE", which is enforced in get_db_handle() two modules away, with nothing
asserting it at the point of use. The dispatch is correct today, but if that invariant ever
changes the failure is not a partial leak — it is unrestricted access to every private record,
silently. #911 at least emitted private = 0 as a second line of defence.

Recommended: require_permissions({PERM_VIEW_PRIVATE}) at the top of _post_sql. Cheap, makes
the coupling local and testable.

Minor

  • Proxied count runs the query twice — a second full enumeration and deserialization of
    every candidate. Worth documenting the cost, or capping it.
  • next_after is non-null whenever len(rows) == limit, so a result set that is an exact
    multiple of limit always costs one extra empty request.
  • Tag has no privacy and no Filter namespace — it could take the SQL path unconditionally
    rather than the manual proxied loop.

Verified as fixed

The private sub-field exposure present in #911 is closed. Confirmed against this branch: a caller
without PERM_VIEW_PRIVATE selecting {"json_path": ["alternate_names", 0, "first_name"]} or
["attribute_list", 0, "value"] on a public person receives None, while a caller with the
permission correctly receives the value. The proxied path projects from proxy-sanitized objects,
so this holds at any relationship depth.

@DavidMStraub

Copy link
Copy Markdown
Member

A general comment: I think there's no reason we cannot use the SQL pushdown for users with PERM_VIEW_PRIVATE in the existing endpoints. From Gramps Web's point of view, that path would be the more maintainable one because we wouldn't have to maintain two duplicate sets of endpoints with completely different API. Do you absolutely need those new endpoints for your use case? If so, I'm ok if we add them. If you're flexible, I would suggest to merge the SQL pushdown into the existing endpoints instead.


Opus comment:

The existing list endpoints load every object in the table into memory, filter and sort in Python, and only then slice out a page — so cost scales with table size rather than with what was requested. Crucially, full_object(), which applies extend, profile, backlinks and keys, already runs after that slice, on the page alone.

That means the rich response and the fast path were never in tension. Pushdown just replaces "load everything, filter, sort, slice" with a single SQL statement returning the page's handles plus a count; the endpoint then materialises those ~20 objects and calls full_object() exactly as today. The response is unchanged.

Requests using arguments that cannot compile to SQL (filter, rules, gql, oql, dates, filemissing) and requests from proxied users fall back to the current path. Because both paths remain, correctness is verifiable directly: for any request, fast and slow must return identical responses.

The win lands on the unfiltered paged list view — the frontend's main screen, and today's worst case.

@dsblank

dsblank commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

If you're flexible, I would suggest to merge the SQL pushdown into the existing endpoints instead.

I think you are right that now that these endpoints allow filters (as slow as the old system, and equivalent). I appreciate the willingness to to swap the old ones out, but I'd prefer some more testing.

I propose having them separate, battle test them, and once proved correct, then we swap them out.

(By the way, if you really want to keep GQL, we could probably compile it to the JSON query format. But I think that the GOQL could be a QL that we settle on for a variety of uses as it automatically creates JOINS for SQL and Python-based code.)

Thank you for the detailed review. I will address those issues today.

… locale, privacy predicate

- Bump gramps-object-query-language floor to >=0.3.3, matching the
  order_by/limit/after/select kwargs _post_proxied actually calls;
  at the old floor (>=0.3.1) every restricted-user request 500'd.
- Reject an explicit `locale` on the proxied path with 422 instead of
  silently ignoring it -- run_query has no COLLATE equivalent, so the
  same request could return a different sort order depending on the
  caller's permissions alone.
- Add require_permissions([PERM_VIEW_PRIVATE]) at the top of _post_sql
  so the "unproxied db implies PERM_VIEW_PRIVATE" invariant is asserted
  locally, not just relied on two modules away in get_db_handle().
- Fix next_after to over-fetch by one row and trim it back off, instead
  of inferring "more pages" from len(rows) == limit -- a result set
  that's an exact multiple of limit no longer costs a wasted, empty
  follow-up request.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dsblank

dsblank commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Pushed 502838d addressing the bugs and security issue from the Opus review:

  • Dependency floor: bumped gramps-object-query-language to >=0.3.3 to match the kwargs _post_proxied actually calls. Confirmed 0.3.1/0.3.2 lack order_by/limit/after/select on run_query.
  • locale silently ignored on proxied path: now rejected with 422 instead — run_query has no COLLATE equivalent, so silently ignoring it meant the same request could sort differently depending on the caller's permissions. Added a test exercising this via the existing _FakeNonPrivateProxy fixture.
  • _post_sql had no privacy predicate of its own: added require_permissions([PERM_VIEW_PRIVATE]) at the top, so the "unproxied db ⟹ caller has PERM_VIEW_PRIVATE" invariant is asserted locally instead of only relying on get_db_handle() two modules away.
  • next_after off-by-one (minor): now over-fetches by one row and trims it back off, rather than inferring "more pages" from len(rows) == limit — an exact-multiple-of-limit result set no longer needs a wasted empty follow-up request. Fixed on both the SQL and proxied paths, with a regression test.

Left as-is for now (both flagged "minor"):

  • Proxied count running the query twice — already documented in a comment; expanded it to spell out the cost is a full re-deserialize, not just "a second query."
  • Tag taking the manual proxied loop instead of the SQL path unconditionally — a real simplification, but a separate change; happy to follow up in a subsequent PR if you'd like.

All existing tests plus 2 new ones pass (107 total across the object-query test files).

On the general architecture question about merging into the existing endpoints: agreed with keeping them separate for now per the earlier reply — happy to revisit once these have some mileage.

…is everything

When a proxied query's first page already contains every match (no
`after`, no more pages beyond what was fetched), X-Total-Count is just
len(rows) -- reuse it instead of re-running run_query a second time with
no limit, which re-deserializes and re-sanitizes every candidate row
just to recompute a number the first call already established. Only
falls back to the second, unlimited query when the page genuinely
doesn't cover the whole result set.

Left the other minor item (Tag bypassing the proxy for the SQL path)
unaddressed deliberately: Tag has no privacy today, but special-casing
it would walk back the "any current or future proxy rule is honored
without this module needing to know it" invariant the rest of the
dispatch logic relies on.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dsblank

dsblank commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Pushed 1622f0b addressing the remaining minor items:

  • Proxied count running the query twice: now short-circuits when the first (over-fetched) page already is the whole result set — no after, and no has_more — since in that case the total is just len(rows), no need to re-deserialize the whole table a second time with no limit just to recompute a number already sitting in the response. Still falls back to the full second run_query when the page genuinely doesn't cover everything (i.e. real pagination is happening). Added two tests asserting the call count (1 vs 2) in each case.
  • Tag bypassing the proxy for the SQL path: deliberately left as-is. It's true Tag has no privacy attribute in Gramps and no Filter namespace today, so it'd be safe right now — but post()'s dispatch is built around the invariant that "any current or future proxy-applied rule comes from the proxy itself, this module doesn't need to know what it is." Special-casing Tag to always bypass the proxy walks that back for one type, so if a future proxy ever wants to restrict tags, this module would silently stop honoring it. Not worth the trade for a type that costs little either way.

109 tests passing across the object-query test files.

@dsblank

dsblank commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

There are some minor differences between old endpoints and new pathways. We can resolve these of the next weeks. I think some of these can be fixed in gramps-object-query-language or just changing defauls. Claude says:


I've traced through the full sort/collation pipeline on both sides. Here's what I found:

The surname flat column is internally consistent, but doesn't match the old ?sort=surname semantics.

  • The fast endpoint's surname column, the proxied-path evaluator's _surname() fallback (gramps_object_query_language/evaluator.py:91-98), and Gramps core's own DB-populated secondary column (gramps/gen/db/generic.py:2735 _get_person_data) all agree with each other: they take primary_name.surname_list[0].surname — the bare text of the first entry only. No prefix, no connector, no other surname entries, and notably not even the .primary-flagged entry (Gramps core's own get_primary_surname() checks the .primary flag; the secondary column doesn't). That's a pre-existing Gramps-core quirk, not something this PR introduced.
  • The old GET /people/?sort=surname goes through a completely different path: sort.py's by_person_surname_key (gramps_webapi/api/resources/sort.py:73-79), which calls Name.get_surname() — and that concatenates every entry in surname_list, with prefixes and connectors joined in locale-translated form (gramps/gen/lib/surnamebase.py:180-205), then appends given name + suffix, then runs the whole string through locale.sort_key().

So for anyone with a compound/multiple surname (patronymic+matronymic, married name kept alongside birth name, etc.), or wherever two people share the same primary surname text, order_by: surname on the new endpoint and ?sort=surname on the old one will produce different orders. There's currently no workaround: OrderBy.column (query.py:1274) only accepts a flat SQL column name — a json_path into surname_list[i].prefix/.suffix can be used in select/where but not order_by at all today.

Two more differences, both intentional but easy to trip over:

  1. Default locale. The old GET endpoint resolves locale with default=True (base.py:567) — it always collates using the system locale, even if the caller passes nothing. The new _post_sql deliberately uses default=False (object_query.py:660-664, with a regression test enforcing it: test_no_locale_uses_plain_codepoint_order_not_system_locale) — no locale param means plain codepoint order, not system-locale collation. A caller relying on old default behavior needs to now pass locale explicitly.
  2. Tie-breaking. The fast endpoint auto-appends handle as a tiebreaker for deterministic pagination (effective_order_by), so ties on surname sort by handle. The old Python sort breaks ties by given name, then suffix.

What does line up: given_name (get_first_name() vs the DB column) matches on both sides, and when the old GET endpoint is called without an explicit sort=, it also just does get_person_handles(sort_handles=True) — i.e., DB ORDER BY surname, the same flat column the fast endpoint uses. So default-order GET vs. fast-endpoint order_by: surname should agree (modulo the locale-default difference above); it's specifically ?sort=surname that diverges.

@DavidMStraub

Copy link
Copy Markdown
Member

Thanks for the thorough fixes, and agreed to keep the endpoints separate for now. I think we're just a couple of review 🏓 away from merging 😉


Remaining issues:

1. Backend detection fails open. _resolve_treeid does
getattr(basedb.dbapi, "treeid", None), and None means "omit the clause" — conflating "this
backend has no treeid concept" with "detection failed". The second case yields an unscoped query,
and it does not raise on its own: on a shared backend that query succeeds and returns other
tenants' rows. _resolve_dialect's "anything unrecognised → PostgreSQL" fallback has the same
shape.

2. treeid threading is not asserted across every SQL-emitting path. The handling itself looks
thorough — main query, COUNT, RelatedObject subqueries, _resolve_after — but the tests cover
it only in places. A fake basedb exposing .dbapi.treeid (the same technique as
_FakeNonPrivateProxy) would let each of those four paths be asserted to carry the predicate and
its parameter, which catches the realistic regression: a new query shape added later without
scoping. No server required.

Standing up real PostgreSQL/SharedPostgreSQL CI is a gap in this repository rather than anything
this PR introduced, and is not something to load onto it.

3. PostgreSQL-dialect rendering is never asserted. The physical-name overrides
("desc" → "desc_", "description" → "desc_ription"), layered on the addon's own string-replace
in Connection.execute(), are currently correct by documented reasoning alone. compile_query
takes an explicit dialect, so asserting the rendered SQL for Dialect.POSTGRESQL needs no
Postgres instance.

4. No equivalence test between the two paths. _FakeNonPrivateProxy filters nothing, so the
same request through it and unproxied must return byte-identical results — isolating "does the
Python evaluator agree with the SQL compiler" from "does privacy filtering work". One such test
exists; a parametrised matrix across object types and representative
select/where/order_by/limit/after combinations would pin the two implementations
together. Nothing currently notices if a library release makes them diverge.

5. No upper bound on gramps-object-query-language. It is 0.x, where a minor bump may break
anything; there is no runtime lockfile; and object_query.py imports 28 symbols from it across
four modules, including evaluator and proxied_query. <0.4 would cover it. Re-exporting a
stable surface from the package root would reduce the coupling more durably.

Minor. The 422 message reads "not supported for a caller without PERM_VIEW_PRIVATE", but the
proxied path is taken for any proxy — test_locale_rejected_on_proxied_path exercises it as
ROLE_OWNER, who does hold the permission. Suggested: "locale-aware sorting is not supported on
the proxied query path."

…eid/dialect/equivalence test coverage

Backend detection previously failed open: an unrecognized database backend
(neither SQLite, single-user PostgreSQL, nor SharedPostgreSQL by class name)
silently ran an unscoped query on the treeid path and guessed PostgreSQL on
the dialect path, either of which can leak another tenant's rows or emit SQL
the connection can't run. Both now abort with 501 instead.

Adds test coverage that was missing: treeid threading through the main
query, COUNT query, and RelatedObject subqueries; PostgreSQL physical-name
column rendering (desc/description) asserted directly via compile_query's
explicit dialect param, no Postgres instance needed; and a parametrised
SQL-vs-proxied equivalence matrix across every object type to pin the SQL
compiler and Python evaluator together. Also caps the
gramps-object-query-language dependency at <0.4 and rewords the proxied
locale-rejection message, which incorrectly implied a permission gate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dsblank

dsblank commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Pushed 73a3694 addressing the remaining review findings:

  • Fail-closed backend detection: _resolve_dialect/_resolve_treeid previously fell open for any unrecognized database backend — defaulting to PostgreSQL syntax and to "no tenant scoping needed", respectively. Both now allowlist known backends by class name (SQLite, single-user PostgreSQL, SharedPostgreSQL) and abort with 501 for anything else, rather than risk cross-tenant row leakage or SQL that the connection can't run.
  • treeid threading tests: added unit tests calling compile_query/compile_count_query directly with treeid set, including a RelatedObject subquery case, asserting the predicate and its parameter appear on every SQL-emitting path (main query, COUNT, correlated subquery, plus the already-covered _resolve_after).
  • PostgreSQL dialect rendering tests: added tests asserting the descdesc_/descriptiondesc_ription physical-name overrides render correctly for select/where/order_by against Dialect.POSTGRESQL, with no Postgres instance required (compile_query takes the dialect explicitly).
  • SQL-vs-proxied equivalence matrix: new parametrised test across all 10 object types and 3 representative select/where/order_by shapes, asserting the unproxied SQL path and the proxied evaluator path (via a filter-nothing fake proxy) return identical results — plus a small-page-size variant to exercise cursor/after agreement at every page boundary.
  • Dependency bound: gramps-object-query-language capped at <0.4 (was unbounded above >=0.3.3).
  • Message wording: the proxied-path locale-rejection 422 no longer implies it's about PERM_VIEW_PRIVATE — reworded to "not supported on the proxied query path".

All new/updated tests pass locally (125 tests across test_object_query_parsing.py, test_endpoints/test_object_query.py, test_endpoints/test_people_query.py).

@DavidMStraub

Copy link
Copy Markdown
Member

Great, I think it's ready!

@dsblank

dsblank commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Merge at will!

Previously value_column was only blocked for 'in'/'like', so a raw
where JSON leaf with op 'regex' and value_column could compile to
Regex(column, RelatedObject(...)) with no field-vs-field support.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dsblank

dsblank commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Added a small update: regex matches.

Comment thread pyproject.toml Outdated
@dsblank

dsblank commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@DavidMStraub you want to merge this and make a release? Then I can make a release of gramps-connect

@DavidMStraub

DavidMStraub commented Aug 10, 2026

Copy link
Copy Markdown
Member

Yes, I'll tag a release at the latest towards the end of this week. #927 and #926 should also be part of it.

@DavidMStraub
DavidMStraub merged commit b66a818 into gramps-project:master Aug 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants