feat(data-access): compound AND filter and orderBy column for postgrest queries

[bottadobe]

What

Two additive capabilities on the PostgREST query layer of @adobe/spacecat-shared-data-access:

1. op.and(...conditions) in the where builder (src/util/postgrest.utils.js) — combine multiple column predicates with AND in a single query.
2. orderBy: { attribute, direction } option on all() / #queryPage (src/models/base/base.collection.js) — order results by an arbitrary model attribute (mapped to its DB column), asc/desc, with the existing id tiebreaker preserved.

Why

The Experience Success Studio back-office /sites page currently downloads the entire sites dataset and filters/sorts it client-side — a slow initial load and an unbounded payload that grows with every site. Moving that work server-side requires the data-access layer to do two things it couldn't before:

• Filter on multiple columns at once (e.g. baseURL substring AND deliveryType AND isLive). applyWhere previously applied only a single condition, so combined server-side filters were impossible.
• Order by a non-index column (e.g. updatedAt desc). Ordering was previously locked to the index-derived sort key.

These two primitives unblock the spacecat-api-service GET /sites filtered/sorted endpoint that the back-office consumes.

How

• applyWhere gains an and operator returning { type: 'and', conditions }; a recursive applyExpr applies each sub-condition to the query in sequence (PostgREST chains filters as AND). Every existing single-condition operator path is unchanged.
• #queryPage honors options.orderBy when orderBy.attribute is set (attribute → DB column via the existing field map; direction asc/desc), keeping the id tiebreaker; when orderBy is absent, ordering is byte-for-byte as before.

Scope & backward compatibility

• Strictly additive. Single-expression where and index-derived ordering behave identically to before.
• PostgREST path only — the ElectroDB path is untouched.
• Full package unit suite (2785 tests) passes; coverage and lint gates green.

Testing

• New unit tests: compound / nested / empty op.and (test/unit/util/postgrest.utils.test.js); orderBy column+direction and the no-orderBy fallback exercised against the real PostgREST query path (test/unit/models/.../*.collection.test.js).

Downstream / follow-ups

• Consumed by spacecat-api-service GET /sites (separate PR), which whitelists sortable columns and validates direction — so unknown-column / casing concerns are handled at the API boundary.
• Minor: orderBy (and the already-absent where / between) could be added to the QueryOptions type in src/models/base/index.d.ts; best folded into the consumer PR.

[github-actions]

This PR will trigger a minor release when merged.

[MysticatBot]

Hey @bottadobe,

Verdict: Request changes - one documentation update needed before merge.
Complexity: MEDIUM - recursive expression evaluator refactor plus new query option.
Changes: Adds compound AND filter (op.and) and explicit orderBy column option for PostgREST queries in the data-access layer (4 files).

Must fix before merge

1. [Important] Stale operator list in CLAUDE.md after adding and - packages/spacecat-shared-data-access/CLAUDE.md (details inline)

Non-blocking (4): minor issues and suggestions

• nit: applyExpr recurses without a depth cap for nested and - not exploitable (callers are trusted internal services) but a one-line guard prevents accidental infinite recursion from a future bug - src/util/postgrest.utils.js:163
• suggestion: add tests for malformed orderBy (e.g. { direction: 'desc' } with no attribute) and for the default-direction path ({ attribute: 'updatedAt' } with no direction) to cover the hasExplicitOrderBy guard branches explicitly - test/unit/models/site-ims-org-access/site-ims-org-access.collection.test.js
• suggestion: the back-compat ordering test asserts only that updated_at is NOT the order column - also asserting that query.order was called (proving the fallback ran) and verifying the ascending flag would make it more robust - test/unit/models/site-ims-org-access/site-ims-org-access.collection.test.js
• suggestion: consider documenting the new orderBy: { attribute, direction } option in the data-access CLAUDE.md Common Patterns section (author notes this is planned for the consumer PR)

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 4m 11s | Cost: $4.53 | Commit: 43c80bbf7c2552d2b55140af6b0b7820ad952e13
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

issue (blocking): The data-access CLAUDE.md documents the available WHERE operators as:

PostgREST WHERE operators: eq, ne, gt, gte, lt, lte, is, in, contains, like, ilike

This list is now stale - it omits the new and combinator added here. A developer (or AI assistant) reading that documentation would conclude compound queries are not possible at this layer and try to work around the limitation.

Fix: Add and to the operator list in packages/spacecat-shared-data-access/CLAUDE.md (the "PostgREST WHERE operators" line). If the root CLAUDE.md has the same list, update it there too.

[dzehnder]

Hey @bottadobe,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repo's docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - clean, well-tested additive change, but the new orderBy option ships untyped and forwards unvalidated caller input as a raw ORDER BY column.
Complexity: MEDIUM - small diff, but it touches the data-access query layer shared by every service.
Changes: adds a compound op.and(...) filter combinator and an orderBy: { attribute, direction } option to the PostgREST query layer of spacecat-shared-data-access (4 files).

Must fix before merge

1. [Important] orderBy.attribute is not validated against the field map - unknown attributes are silently camelToSnaked into a raw .order() column - packages/spacecat-shared-data-access/src/models/base/base.collection.js:433 (details inline)
2. [Important] orderBy.direction is exact-string 'desc', so 'DESC' / invalid values silently sort ascending, and the new ascending branch is untested - packages/spacecat-shared-data-access/src/models/base/base.collection.js:436 (details inline)
3. [Important] The new public orderBy option is untyped and undocumented, and cannot be deferred to the consumer PR - packages/spacecat-shared-data-access/src/models/base/base.collection.js:432 (details inline)

Non-blocking (5): minor issues and suggestions

• nit: empty or all-falsy op.and(...) returns the query unchanged, so a where built from an empty condition list matches every row. This is intentional and tested, but document the footgun on op.and - packages/spacecat-shared-data-access/src/util/postgrest.utils.js:155
• nit: (expr.conditions || []) - the || [] is unreachable (the and builder always sets conditions to an array) and adds a dead branch against the 97% branch-coverage gate; drop it to expr.conditions.reduce(...) - packages/spacecat-shared-data-access/src/util/postgrest.utils.js:166
• suggestion: order (legacy string) and the new orderBy (object) now coexist on the base collection; when both are passed, orderBy silently wins. Document the precedence, or express order as sugar over orderBy so there is one sort path - packages/spacecat-shared-data-access/src/models/base/base.collection.js:432
• suggestion: strengthen the tests - make the back-compat assertion positive (order('id', { ascending: true })) rather than negative-only, and add cases for a missing/uppercase direction (ascending branch), an unknown orderBy.attribute, and an unsupported operator nested inside op.and(...)
• nit: the packages/spacecat-shared-data-access/CLAUDE.md "PostgREST WHERE operators" list does not include the new and operator; append it (the ordering example in the same doc also predates this option - see must-fix item 3)

Out of scope, worth tracking: ordering by a non-indexed column pushes the sort server-side, and #queryPage sorts the full filtered set before .range() paginates. For a large table (e.g. sites sorted by updatedAt) this needs a DB index in mysticat-data-service plus a sortable-column allowlist in the spacecat-api-service GET /sites consumer before it ships to a large tenant.

[dzehnder]

issue (blocking): orderBy.attribute is passed straight to #toDbField, which resolves as map[field] || camelToSnake(field) (postgrest.utils.js:81). An attribute not in the field map does not throw - it is camelToSnaked and forwarded as the raw .order() column. Because camelToSnake only rewrites case boundaries, punctuation like , . ( ) passes through, so an attribute such as id,created_at becomes the token order=id,created_at.asc. PostgREST rejects unknown identifiers (HTTP 400, so this is not SQL injection), but a caller can still order by any real column of the table, including ones never meant to be sortable - an ordering oracle - and typos surface only as an opaque Failed to query error.

This is a shared foundation library; relying on the GET /sites consumer to whitelist columns pushes a security-relevant invariant onto every current and future caller, when the field map to enforce it already lives here. Validate the attribute before ordering, e.g.:

if (hasExplicitOrderBy
    && !Object.prototype.hasOwnProperty.call(this.fieldMaps.toDbMap, options.orderBy.attribute)) {
  this.#logAndThrowError(`Failed to query [${this.entityName}]: unknown orderBy attribute [${options.orderBy.attribute}]`);
}

The same non-allowlisting passthrough already exists on the between / where paths; a shared #requireKnownAttribute helper would close it in one place.

[dzehnder]

issue (blocking): const ascending = options.orderBy.direction !== 'desc' treats any value other than the exact lowercase string 'desc' as ascending - 'DESC', 'Desc', 'descending', a typo, null, or a missing direction all silently produce the opposite of a descending request, with no error and no log. A silent wrong-order is worse than a thrown error, and this is a shared package with callers beyond the one that validates input.

Normalize or validate: String(options.orderBy.direction).toLowerCase() === 'desc', or reject anything not in { asc, desc }. The added test only exercises direction: 'desc'; the entire ascending branch of the new code is uncovered - add a test for a missing/uppercase direction.

[dzehnder]

issue (blocking): this adds a new public query option (options.orderBy: { attribute, direction }), consumed by all() / allByIndexKeys(), but QueryOptions in src/models/base/index.d.ts:32 still declares only order?: string - no orderBy. The repo-root CLAUDE.md ("TypeScript Declarations") requires .d.ts files for public APIs, and that type lives in this package, so the PR-body plan to fold it into the spacecat-api-service consumer PR is not possible: a different repo cannot edit spacecat-shared's type contract. Add it here (one line):

orderBy?: { attribute: string; direction?: 'asc' | 'desc' };

While there, the already-missing where / between options could join it. Separately, this orderBy: { attribute, ... } shape diverges from the ordering example in packages/spacecat-shared-data-access/CLAUDE.md (order: { field, direction }, which is itself already stale - the real order is a plain string). Update that example to the current contract in the same PR.

[MysticatBot]

Hey @bottadobe,

Verdict: Approve - prior blocking findings addressed; one minor doc gap remains.
Complexity: MEDIUM - recursive expression evaluator plus new query option in shared library.
Changes: Adds compound AND filter (op.and) and explicit orderBy column option for PostgREST queries, with full input validation (4 files + tests).

Previously flagged, now resolved

• orderBy.attribute now validated against the field map (throws on unknown attribute)
• orderBy.direction normalized to lowercase, validated to asc/desc only (throws on invalid)
• TypeScript type added to QueryOptions interface
• CLAUDE.md example updated to use the new orderBy shape

Non-blocking (1): minor issues and suggestions

• nit: CLAUDE.md operators list at line 198 still reads eq, ne, gt, gte, lt, lte, is, in, contains, like, ilike without the new and combinator - append it so developers discover compound query support from the docs - packages/spacecat-shared-data-access/CLAUDE.md:198

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 34s | Cost: $3.66 | Commit: 07fcfa2fc200eb4817f2a8956aa6151a1966135e
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[dzehnder]

Hey @bottadobe,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repo's docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - the three blocking findings from the prior review are resolved with attribute/direction validation, a type declaration, new tests, and a doc fix.
Complexity: MEDIUM - small diff, data-access query layer shared by every service.
Changes: hardens the new orderBy: { attribute, direction } option with field-map validation, case-insensitive direction, a QueryOptions type, and tests (this update: 4 files).
Note: CI Test check is still running - confirm green before merge.

Previously flagged, now resolved

• orderBy.attribute is now validated against the field map and throws unknown orderBy attribute, closing the raw-column passthrough.
• orderBy.direction is normalized case-insensitively, defaults to asc, and throws on invalid values; the ascending, omitted, and DESC branches are now tested.
• orderBy is declared in QueryOptions (index.d.ts), and the CLAUDE.md ordering example is updated to the orderBy: { attribute, ... } shape.

Non-blocking (3): carried over from the prior review, still open

• nit: empty or all-falsy op.and(...) still returns the query unchanged (matches every row) - packages/spacecat-shared-data-access/src/util/postgrest.utils.js:155
• nit: (expr.conditions || []) remains an unreachable branch against the 97% coverage gate - packages/spacecat-shared-data-access/src/util/postgrest.utils.js:166
• nit: the CLAUDE.md "PostgREST WHERE operators" list still omits the new and operator

[solaris007]

🎉 This PR is included in version @adobe/spacecat-shared-data-access-v4.20.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀