feat(SITES-47203): add baseUrlContains substring search to GET /sites#2693
feat(SITES-47203): add baseUrlContains substring search to GET /sites#2693habansal wants to merge 11 commits into
Conversation
Extend the existing GET /sites controller with an optional `baseUrlLike`
query param. When provided (min 3 chars after trimming), it performs a
case-insensitive substring search on `baseURL` via an ilike where clause
and returns a non-cursor envelope `{ sites, pagination: { limit, hasMore } }`.
- Runs after the admin/S2S `site:readAll` authz check (403 preserved) and
before the cursor/legacy branches; existing behavior is unchanged.
- LIKE wildcards (`%`, `_`, `\`) in user input are escaped.
- Limit defaults to 50, clamped to MAX_LIMIT (500); fetches limit+1 rows to
derive `hasMore`.
- Adds controller tests and documents the param in the OpenAPI spec.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This PR will trigger a minor release when merged. |
…ites Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply multi-reviewer review findings for GET /sites?baseUrlLike search: - Echo the trimmed query in pagination.baseUrlLike so a new client can detect deploy skew (an older API that ignores baseUrlLike but honors limit would return the cursor envelope with unfiltered sites and no echo). - Add unconditional [sites][baseUrlLike] observability log covering both the admin and S2S paths; logs qlen/count/hasMore only, never the raw query value (URLs may be sensitive). - Enforce a max-length guard (>256 -> 400), mirroring the cursor guard. - Replace the hardcoded search default limit 50 with named constant SEARCH_DEFAULT_LIMIT (cursor path keeps DEFAULT_LIMIT=100). - OpenAPI: document post-trim length enforcement and maxLength: 256 on the baseUrlLike param, add a dedicated SiteSearchResponse schema with the baseUrlLike echo, and wire it into the GET /sites oneOf. - Tests: add empty-result, baseUrlLike echo, inclusive 3-char boundary, and over-256-char (400) cases; update existing pagination assertions for the echo field. order: 'asc' and the deferred pg_trgm index are intentionally left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t deploy ordering Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Postgres) Adds GET /sites?baseUrlLike cases to the shared sites suite — exercises the real PostgREST ilike path, N+1 hasMore, DB-level wildcard escaping, and authz parity. Runs in CI (Postgres+PostgREST harness); local run needs ECR access to the mysticat-data-service image. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The data-access where builder invokes the callback as (attrs, op) — attrs maps
model fields to DB columns, op carries the operators. The code used
`s => s.ilike('baseURL', ...)`, but `s` is the field proxy with no .ilike, so
PostgREST queries failed at runtime with 'TypeError: s.ilike is not a function'
(caught by the Postgres integration test; the unit test had stubbed a fake
single-arg builder and missed it). Fixed to `(attr, op) => op.ilike(attr.baseURL, ...)`
and updated the unit tests to mirror the real two-arg signature.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Full test plan validated end-to-end (including live dev)Verified at every layer:
Also observed the deploy-ordering hazard live (old |
There was a problem hiding this comment.
Hey @habansal,
Verdict: Request changes - two blocking issues around param-combination behavior and error resilience.
Complexity: HIGH - medium diff; API surface risk flag.
Changes: Adds optional baseUrlLike substring search to GET /sites with input validation, LIKE wildcard escaping, N+1 hasMore pagination, and a deploy-ordering echo (6 files).
Note: Recommend a human read before merge - this change amends an architectural decision record and modifies the OpenAPI contract. The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently failing (codecov/patch: 2 lines missing coverage).
Must fix before merge
- [Important] Silent
cursordiscard when combined withbaseUrlLike-src/controllers/sites.js:461(details inline) - [Important] No resilience handling for unexpected
Site.allreturn shape -src/controllers/sites.js:487(details inline)
Non-blocking (4): minor issues and suggestions
- nit:
order: 'asc'relies on the data-access layer's default sort column rather than specifying one explicitly; ITs prove stability but the assumption is implicit and fragile if the shared library changes its default -src/controllers/sites.js:485 - nit: OpenAPI
minLength: 3/maxLength: 256onbaseUrlLikedescribe post-trim semantics but apply pre-trim in the schema; a value like" ab "passes the schema but fails the server -docs/openapi/sites-api.yaml:176 - suggestion: Close the codecov gap by adding a unit test that exercises the
baseUrlLikepath with S2S auth context (theif (s2sResult.allowed)log branch) -src/controllers/sites.js:491 - suggestion: Add the effective limit to the unconditional observability log (
limit=50) to distinguish "capped at limit" from "exact match count" in production -src/controllers/sites.js:497
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 13s | Cost: $4.82 | Commit: fd8c1f6f7284b569c71f3e3e3b9907feeb56caac
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…rd, search resilience - Reject baseUrlLike combined with cursor (400) instead of silently discarding the cursor and misleading the client. - Warn on unexpected Site.all return shape before defaulting to empty. - Wrap the baseUrlLike Site.all query in try/catch: log a searchable, prefixed error line and re-throw so the framework still returns 500. - Add unit tests for the cursor guard, unexpected-shape warn, and query-failure error logging/propagation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @habansal,
Verdict: Request changes - two documentation inaccuracies in the new ADR need correction before merge.
Complexity: HIGH - large diff; API surface.
Changes: Adds optional baseUrlLike substring search to GET /sites with input validation, LIKE wildcard escaping, N+1 hasMore pagination, and a deploy-ordering echo (6 files).
Note: Recommend a human read before merge - this change amends an architectural decision record and modifies the OpenAPI contract. The bot review is a complement to, not a replacement for, a human read here.
Must fix before merge
- [Important] ADR-006 documents wrong
whereclause signature -docs/decisions/006-sites-base-url-search.md:38(details inline) - [Important] ADR-006 omits
baseUrlLikeecho from documented response shape -docs/decisions/006-sites-base-url-search.md:44(details inline)
Non-blocking (4): minor issues and suggestions
- nit:
order: 'asc'without an explicitorderByfield relies on the data-access layer's default sort column; the integration tests prove stability but the assumption is implicit -src/controllers/sites.js:485 - nit: OpenAPI
minLength: 3/maxLength: 256describe post-trim semantics but schema validators enforce pre-trim; a value like" ab "passes the schema but fails the server -docs/openapi/sites-api.yaml:170 - nit: ADR-006 introduces undefined "UbI" acronym on line 13 that is never referenced again -
docs/decisions/006-sites-base-url-search.md:13 - suggestion: Close the codecov gap by adding a unit test exercising the
baseUrlLikepath with S2S auth context (theif (s2sResult.allowed)log branch) -src/controllers/sites.js:515
Previously flagged, now resolved
- Cursor + baseUrlLike combination now explicitly rejected with 400 (guard + test added)
- Unexpected Site.all return shape now handled with try/catch, shape guard, warning log, and tests
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 59s | Cost: $3.95 | Commit: 32c8d0ce5bc815e177da235414f9d71ec30a5adf
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…icatBot review) Correct the where-builder signature to the real (attr, op) => op.ilike(attr.baseURL, ...), add the baseUrlLike echo to the documented response shape, note the deterministic order tiebreaker, and fix the UbI typo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @habansal,
Verdict: Approve - all prior blocking findings addressed; ADR corrections are accurate.
Complexity: HIGH - large diff; API surface.
Changes: Adds optional baseUrlLike substring search to GET /sites with input validation, LIKE wildcard escaping, N+1 hasMore pagination, and a deploy-ordering echo (6 files).
Note: Recommend a human read before merge - this change adds an architectural decision record and modifies the OpenAPI contract. The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently pending - confirm they pass before merge.
Previously flagged, now resolved
- Silent
cursordiscard when combined withbaseUrlLikenow explicitly rejected with 400 - Unexpected
Site.allreturn shape now handled with try/catch, shape guard, and warning log - ADR
whereclause signature corrected to(attr, op) => op.ilike(attr.baseURL, ...) - ADR response shape now includes the
baseUrlLikedeploy-ordering echo - ADR "UbI" typo corrected to "UI"
- ADR now documents
order: 'asc'tiebreaker behavior viabase.collection's#getOrderFields
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 41s | Cost: $3.97 | Commit: e115b34dca027330c6d6298b0282edb202bc228f
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Closes the codecov patch gap MysticatBot flagged — the admin tests bypass the s2sResult.allowed branch (sites.js:514-515), so add an S2S consumer (site:readAll) test that exercises the baseUrlLike search path and asserts the [s2s-readall] log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @habansal,
Verdict: Approve - new commit closes the codecov gap on the S2S auth path as suggested; no new concerns.
Complexity: HIGH - large diff; API surface.
Changes: Adds optional baseUrlLike substring search to GET /sites with input validation, LIKE wildcard escaping, N+1 hasMore pagination, and a deploy-ordering echo (6 files).
Note: Recommend a human read before merge - this change adds an architectural decision record and modifies the OpenAPI contract. The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently pending - confirm they pass before merge.
Previously flagged, now resolved
- Codecov S2S auth-path coverage gap now closed with dedicated
site:readAll+baseUrlLiketest
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 12s | Cost: $2.29 | Commit: b222558161d47e95078ca504b22810a2a7a89af7
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…gination
Rename the GET /sites substring-search query param from baseUrlLike to
baseUrlContains across the controller, OpenAPI spec, ADR-006, and tests
(param, validation messages, cursor-guard, pagination echo, and log prefixes).
Add offset pagination to the baseUrlContains search branch. The data-access
layer has no public offset option, so the offset is translated into the same
base64(JSON.stringify({ offset })) cursor the layer uses internally. The
response now echoes { limit, offset, hasMore, baseUrlContains } and the
cursor+baseUrlContains guard message points callers to offset.
Locked contract:
GET /sites?baseUrlContains=<q>&limit=<N>&offset=<M>
-> { sites: [...], pagination: { limit, offset, hasMore, baseUrlContains } }
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @habansal,
Verdict: Approve - new commit cleanly adds offset pagination and renames the param; no blocking concerns.
Complexity: HIGH - large diff; API surface.
Changes: Renames baseUrlLike to baseUrlContains, adds offset-based pagination via internal cursor construction, and updates ADR-006/OpenAPI/tests accordingly (6 files).
Note: Recommend a human read before merge - this change adds an architectural decision record and modifies the OpenAPI contract. The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (3): minor issues and suggestions
- nit:
parseIntsilently truncates floats (e.g.offset=2.5becomes2,limit=3.7becomes3) rather than rejecting them, which diverges from the documented "must be an integer" contract -src/controllers/sites.js:492 - suggestion: Reject
offsetwhen provided withoutbaseUrlContains(currently silently ignored, falling through to the cursor path). The OpenAPI spec documents this scoping, but a 400 would prevent confused clients from getting unfiltered results when they intended a search -src/controllers/sites.js:458 - suggestion: Add an integration test for
offsetbeyond total results (e.g.offset=9999) to assert the expected{ sites: [], hasMore: false }response and catch any future data-access regressions on out-of-bounds offsets -test/it/shared/tests/sites.js
Previously flagged, now resolved
- Silent
cursordiscard when combined withbaseUrlLikenow explicitly rejected with 400 (renamed tobaseUrlContains) - Unexpected
Site.allreturn shape now handled with try/catch, shape guard, and warning log - ADR
whereclause signature corrected - ADR response shape now includes the echo field
- Codecov S2S auth-path coverage gap closed
- ADR renamed throughout from
baseUrlLiketobaseUrlContainswith offset documentation added
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 21s | Cost: $5.02 | Commit: b564cf0e8351b77d332344bf260b704625d20214
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Summary
Extends the existing
GET /sitescontroller (src/controllers/sites.js,getAll) with an optionalbaseUrlContainsquery param for case-insensitive substring search on a site'sbaseURL. Part of SITES-47203 (the ESS backoffice consumes this to let users find sites by partial URL).Endpoint contract
GET /sites?baseUrlContains=<substr>[&limit=<n>]baseUrlContains(string, optional): case-insensitive substring matched againstbaseURL. Must be >= 3 chars after trimming. LIKE wildcards (%,_,\) are escaped and matched literally.limit(int, optional): bounds result size. Defaults to 50, clamped to 500 (MAX_LIMIT). Non-positive/non-integer -> 400.Response (200): non-cursor envelope
{ "sites": [ /* slim site DTOs */ ], "pagination": { "limit": 50, "hasMore": false, "baseUrlContains": "<query>" } }hasMoreis derived by fetchinglimit + 1rows. There is nocursoron this path (not cursor-iterable).Errors:
400-baseUrlContainsshorter than 3 chars (baseUrlContains must be at least 3 characters)400- invalidlimit(limit must be a positive integer)403- caller lacks admin access / S2Ssite:readAllBehavior preserved
The new branch runs after the admin/S2S
site:readAllauthz check (403 still enforced) and before the existing cursor-paginated and legacy flat-array branches. Those paths are unchanged.Tests
Added a
GET /sites - baseUrlContains substring searchsuite covering: ilikewherebuilder +order:'asc'+limit=N+1,hasMoretrue/false with body trimming, wildcard escaping, slim DTO shape, cursor-shaped result fallback, < 3 char -> 400, invalid limit -> 400, MAX_LIMIT clamp, and 403 for a non-admin/non-S2S caller. ExistinggetAlltests still pass.Also documented the param in
docs/openapi/sites-api.yaml.Test plan
npx mocha --spec=test/controllers/sites.test.js- 381 passingeslinton changed files - cleannpm run docs:lint- API description valid🤖 Generated with Claude Code
Update — local review fixes applied
Hardened after a local multi-specialist review (code/API/security/SRE/QA/spec). Added: a deploy-ordering discriminator (
pagination.baseUrlContainsecho) so clients can detect an older API that ignored the param; unconditional[sites][baseUrlContains]observability logging (no raw query); a max-length guard (≤256); a namedSEARCH_DEFAULT_LIMIT; and tests for empty-results, the echo, the 3-char boundary, and over-length. ADR-006 updated with the deploy-ordering rationale.limit, so an older deployment of this API would ignorebaseUrlContainsand return unfiltered cursor results. Deploy this PR before or together with the Backoffice consumer (OneAdobe/experience-success-studio-backoffice#332).Jira: https://jira.corp.adobe.com/browse/SITES-47203
Update —
offsetpagination + param renamebaseUrlLike→baseUrlContains(clearer intent; avoids leaking the SQLilikeinto the public contract). Thepagination.baseUrlContainsecho was renamed to match.offsetpagination:GET /sites?baseUrlContains=<q>&limit=<N>&offset=<M>→{ sites, pagination: { limit, offset, hasMore, baseUrlContains } }.offsetdefaults to 0, must be a non-negative integer. (The data-access layer exposes no publicoffset, so the controller builds the same offset-encoded cursor inline — documented in ADR-006.)it-postgrescovers an offset paging case.