fix: remove pagination from entitlement org query #1415
fix: remove pagination from entitlement org query #1415igor-grubic wants to merge 4 commits intomainfrom
Conversation
|
This PR will trigger no release when merged. |
solaris007
left a comment
There was a problem hiding this comment.
Hey @igor-grubic,
Good catch on the pagination bug - .range() without .order() is a real correctness issue. PostgreSQL makes no ordering guarantees without an explicit ORDER BY, so offset-based pagination over unordered results will produce duplicates and gaps across pages. The diagnosis is spot on.
That said, I think the fix goes one step too far by removing pagination entirely. The recommended path is to fix the pagination rather than drop it.
The concern: silent truncation risk
The new code issues a single unbounded query with no .range(). Whether this returns all rows depends on PostgREST's PGRST_DB_MAX_ROWS server-side setting:
- I checked our infra (
spacecat-infrastructure/modules/mysticat_data_service/main.tf) and local dev (mysticat-data-service/docker/docker-compose.yml) -PGRST_DB_MAX_ROWSis not set anywhere, so the PostgREST default is unlimited. Today this works fine. - But if anyone ever adds a
PGRST_DB_MAX_ROWSlimit as an infra hardening measure (common practice), PostgREST returns a 200 OK with a truncated result set - no error, no signal. The function would silently return a partial list of entitled organizations. Customers would be treated as unentitled with no error surfaced anywhere. - The old paginated code was immune to this because each
.range()call supplies an explicitLIMIT ... OFFSETper page, interacting withmax-rowsin a controlled way.
Recommended fix: one-line change to the existing loop
The original bug was the missing ORDER BY, not the pagination itself. Adding .order('id') to the existing paginated loop fixes the root cause without removing the safety net:
const { data, error } = await this.postgrestService
.from('entitlements')
.select('id, product_code, tier, organizations!inner(id, name, ims_org_id)')
.eq('product_code', productCode)
.order('id') // <-- the actual fix
.range(offset, offset + DEFAULT_PAGE_SIZE - 1);This is a minimal change that fixes the duplicate/gap bug while preserving bounded page fetches.
Secondary items
- Test gap: The pagination test was deleted, but no assertion was added to verify
.order('id')is actually called. Whether pagination stays or goes, addingexpect(orderStub).to.have.been.calledWith('id')to the success-path test would catch regressions on the sort column. - Growth trajectory: The PR says "~2,500 orgs fit in a single query" - that's a point-in-time observation, not an enforced invariant. With LLMO onboarding growing, a code comment documenting this assumption would help future readers.
Summary
The allByProductCodeWithOrganization query used offset-based pagination (.range()) without .order(), causing PostgreSQL to return rows in non-deterministic order across pages -- leading to duplicate and missing orgs
With ~2,500 orgs the dataset fits in a single query, so pagination is removed and replaced with a single .order('id') call