feat(data-access): add readAll capability to Consumer.CAPABILITIES#1573
feat(data-access): add readAll capability to Consumer.CAPABILITIES#1573
Conversation
Adds 'readAll' as a valid action in Consumer.CAPABILITIES, enabling capability strings of the form `<entity>:readAll` (e.g. `site:readAll`, `organization:readAll`) to pass `ConsumerCollection.validateCapabilities`. This is the shared-lib half of the S2S readAll capability rollout. The api-service consumes this in a follow-up PR to remap GET /sites and GET /organizations from `<entity>:read` to `<entity>:readAll`, allowing platform-level S2S consumers to enumerate sites/organizations across tenants without weakening tenant isolation on per-resource operations. See docs/s2s/READALL_CAPABILITY_DESIGN.md (in adobe/spacecat-api-service) for the full design and trust-boundary analysis. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
solaris007
left a comment
There was a problem hiding this comment.
Hey @ravverma,
Clean, minimal change - this is the right scope for step 1 of the readAll rollout. A couple of minor suggestions.
Strengths
- Correctly scoped to the shared library's responsibility: adds the schema-valid action, nothing more. The route-level and controller-level enforcement stays in spacecat-api-service where it belongs.
- Existing validation chain is preserved: the "throws ValidationError for unknown entity in capability" test still exercises the entity-must-be-known guard. Adding
readAllto CAPABILITIES does not loosen entity validation. - Model unit test updated to assert the new CAPABILITIES shape. Collection integration test exercises the positive path with
site:readAllandorganization:readAll.
Issues
Minor (Nice to Have)
Test name does not match test content
consumer.collection.test.js - new test case
The test is titled "accepts the readAll action for any registered entity" but only exercises site:readAll and organization:readAll. The title claims generality ("any registered entity") that the test body does not deliver. A reader might conclude all entities were tested when only two were. Either rename to "accepts site:readAll and organization:readAll capabilities" or extend with additional entities (audit:readAll, opportunity:readAll) to match the title.
Weak result assertion
consumer.collection.test.js - new test case
expect(result).to.not.be.null is the only assertion on the result. It does not verify that the readAll capability strings survived validation and reached the create call. Compare with the sibling test for existing capabilities, which may assert more about the returned consumer shape. Consider at minimum asserting that mockElectroService.entities.consumer.create was called (i.e. validation did not reject).
Recommendations
- Add a brief JSDoc comment on the
CAPABILITIESarray explaining the semantic difference between the verb-actions (read,write,delete) and the scope+verb hybrid (readAll). A future reader encountering this array will not know thatreadAllconflates action and scope without reading a design doc in a different repo. One line is enough: "readAll is a scope+verb hybrid - see the S2S readAll design in mysticat-architecture.". - Consider adding a comment near the CAPABILITIES constant noting the design's off-ramp: "If 5+ entities adopt readAll, revisit in favor of a policy engine." This keeps the threshold visible to developers who might extend the list.
Assessment
Ready to merge? Yes
The change is minimal, correct per the design, and adequately tested. The minor suggestions above improve clarity but are not blockers. Once this merges and releases, spacecat-api-service PR 2305 can revert its dependency from the personal gist URL back to the npm registry.
solaris007
left a comment
There was a problem hiding this comment.
Approving with two small asks:
-
Add a brief JSDoc on
Consumer.CAPABILITIES(consumer.model.js:54) explaining thatreadAllis a scope+verb hybrid, currently meaningful only on routes that opt in (siteandorganization). Schema validity does not imply a reachable route. This is the one place a future maintainer could be misled. -
In the new collection test ("accepts the readAll action..."), add
expect(mockElectroService.entities.consumer.create).to.have.been.calledOnceto match the sibling happy-path assertion strength. Currently the test only proves "no exception thrown", not "validation passed and create was called". Also consider renaming the test to "accepts site:readAll and organization:readAll capabilities" since it only exercises those two.
Neither blocks the release - ship once folded in.
- Add JSDoc on Consumer.CAPABILITIES explaining readAll is a scope+verb hybrid meaningful only on opted-in routes (site, organization) - Rename test to accurately reflect the two entities exercised - Add calledOnce assertion to verify create was invoked after validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
|
Thanks @solaris007 for review All 1905 tests pass. Here's a summary of the three changes made to address the reviewer's asks:
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
## [@adobe/spacecat-shared-data-access-v3.56.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-data-access-v3.55.1...@adobe/spacecat-shared-data-access-v3.56.0) (2026-05-04) ### Features * **data-access:** add readAll capability to Consumer.CAPABILITIES ([#1573](#1573)) ([4475928](4475928))
|
🎉 This PR is included in version @adobe/spacecat-shared-data-access-v3.56.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…tant (#1579) ## Session Summary ### PR #1573 — `feat(data-access): add readAll capability to Consumer.CAPABILITIES` **Review comments addressed (pushed to branch, now merged):** 1. Added JSDoc on `Consumer.CAPABILITIES` explaining `readAll` is a scope+verb hybrid, only meaningful on opted-in routes 2. Renamed test: `"accepts the readAll action for any registered entity"` → `"accepts site:readAll and organization:readAll capabilities"` 3. Strengthened test assertion: added `expect(mockElectroService.entities.consumer.create).to.have.been.calledOnce` --- ### CI failures investigated | Failure | Root cause | Verdict | | --------------------------------------------------- | ------------------------------------------------- | ----------------------------- | | `mysticat-data-service:v1.67.8 not found` | Image removed from ECR | Infrastructure issue | | `TrialUser/TrialUserActivity IT timeout` | 10s hook timeout too tight for v5.x image startup | Code fix needed | | `HTML Utils warmup delay: expected 749 to be ≥ 750` | Wall-clock timing flake in `tokowaka-client` | Pre-existing flake, unrelated | --- ### New PR branch — `fix/it-timeout-constant` (from main) - Bumped default docker image tag to `v5.1.1` in `docker-compose.yml` - Introduced `IT_HOOK_TIMEOUT = 30000` constant in `test/it/util/util.js` - Updated all 37 IT test files to import and use the constant instead of the hard-coded `10000` Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
## [@adobe/spacecat-shared-data-access-v3.56.1](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-data-access-v3.56.0...@adobe/spacecat-shared-data-access-v3.56.1) (2026-05-04) ### Bug Fixes * **data-access:** replace hard-coded IT hook timeout with shared constant ([#1579](#1579)) ([4fbc6e0](4fbc6e0)), closes [#1573](#1573)
Adds 'readAll' as a valid action in Consumer.CAPABILITIES, enabling capability strings of the form
<entity>:readAll(e.g.site:readAll,organization:readAll) to passConsumerCollection.validateCapabilities.This is the shared-lib half of the S2S readAll capability rollout. The api-service consumes this in a follow-up PR to remap GET /sites and GET /organizations from
<entity>:readto<entity>:readAll, allowing platform-level S2S consumers to enumerate sites/organizations across tenants without weakening tenant isolation on per-resource operations.See docs/s2s/READALL_CAPABILITY_DESIGN.md (in adobe/spacecat-api-service) for the full design and trust-boundary analysis.
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!