feat: support tenant-aware ORD for extensible multitenant apps#391
feat: support tenant-aware ORD for extensible multitenant apps#391zongqichen wants to merge 3 commits intomainfrom
Conversation
Wrap ORD routes with cds.middlewares.before/after so that cds.context.tenant and cds.context.model are populated per request. This enables tenant-specific CSN (from extensibility/toggles) to be used for ORD document and metadata generation. Follows the same pattern as @cap-js/graphql. Closes #389
Previously cds.middlewares.before/after was mounted on "/" which caused all HTTP requests (including SaaS provisioning, OData) to go through CDS middlewares twice, resulting in "Cannot redefine property: authInfo". Now mounted on "/ord" so only ORD routes go through the tenant-aware middleware stack. Router paths adjusted to be relative (/v1/... instead of /ord/v1/...) since Express strips the mount prefix. External URLs remain unchanged: /ord/v1/documents/ord-document
SummaryThe following content is AI-generated and provides a summary of the pull request: Support Tenant-Aware ORD for Extensible Multitenant CAP ApplicationsNew Feature✨ ORD endpoints now support tenant-aware model resolution for extensible multitenant CAP applications. When a tenant has model extensions (via This is achieved by routing ORD requests through
Changes
Jira Issues
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The production change in lib/ord-service.js is sound in intent, but the new test file has several correctness issues: the jest.mock is called inside a test callback (never hoisted, therefore ineffective), test 4 doesn't actually exercise the production cds.context.model read path (it manually resolves the model before passing it in), and tests 2–3 rely on fragile source-text string matching rather than behavioural assertions. Additionally, lib/ord-service.js lacks a guard for cds.middlewares being undefined, which would cause a startup crash in non-MTX or older CDS environments.
PR Bot Information
Version: 1.18.5 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
ffa85910-1e3c-11f1-91ef-e0e746f7c3b4 - LLM:
anthropic--claude-4.6-sonnet - Agent Instructions:
- Event Trigger:
pull_request.opened
| const { getMetadata } = require("../../lib/index"); | ||
| const { compile: openapi } = require("@cap-js/openapi"); | ||
|
|
||
| // Provide a context model (simulates MTX-populated tenant model) | ||
| const contextModel = { | ||
| definitions: { | ||
| TestService: {}, | ||
| }, | ||
| }; | ||
| const savedContext = cds.context; | ||
| cds.context = { model: contextModel }; | ||
|
|
||
| jest.mock("@cap-js/openapi", () => ({ |
There was a problem hiding this comment.
Bug: jest.mock called after module is already required, making the mock ineffective
jest.mock("@cap-js/openapi", ...) is called at line 26, but require("@cap-js/openapi") at line 15 has already loaded the real module. Jest hoists jest.mock calls to the top of the file only when they appear at the top level of the module scope—not when nested inside a test() callback. Inside a callback, the mock is applied after the module cache is already populated, so the real openapi implementation runs instead of the mock.
The test comment at line 34 ("We only verify the call did not throw and returned a result") hints that the author already acknowledged the mock may not be working as intended—confirming the test only provides very weak coverage.
Should restructure: move the jest.mock("@cap-js/openapi", ...) call to the top level (before any describe/test), or use jest.spyOn inside the test to replace the already-imported openapi function on its module object, similar to how metaData.test.js does it at line 30 (openapi.mockImplementation(...)).
| const { getMetadata } = require("../../lib/index"); | |
| const { compile: openapi } = require("@cap-js/openapi"); | |
| // Provide a context model (simulates MTX-populated tenant model) | |
| const contextModel = { | |
| definitions: { | |
| TestService: {}, | |
| }, | |
| }; | |
| const savedContext = cds.context; | |
| cds.context = { model: contextModel }; | |
| jest.mock("@cap-js/openapi", () => ({ | |
| const { getMetadata } = require("../../lib/index"); | |
| const { compile: openapi } = require("@cap-js/openapi"); | |
| // Provide a context model (simulates MTX-populated tenant model) | |
| const contextModel = { | |
| definitions: { | |
| TestService: {}, | |
| }, | |
| }; | |
| const savedContext = cds.context; | |
| cds.context = { model: contextModel }; | |
| openapi.mockReturnValue("openapi-from-context-model"); | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| expect(result.contentType).toBe("application/json"); | ||
| } finally { | ||
| cds.context = savedContext; | ||
| jest.unmock("@cap-js/openapi"); |
There was a problem hiding this comment.
Bug: jest.unmock cannot undo an in-callback jest.mock and leaves mock state dirty
jest.unmock scheduled in finally does not actually reset an in-callback jest.mock. Module mocks set at runtime (not hoisted) stay in the module registry for the rest of the test suite. If the suggestion above (switching to mockReturnValue) is applied, this cleanup line should be removed—jest.clearAllMocks() or mockReset() in an afterEach is the right approach for spy-based mocks.
| jest.unmock("@cap-js/openapi"); | |
| cds.context = savedContext; | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| // With context model set: ord() should use the extended model | ||
| cds.context = { model: extendedModel }; | ||
| const csn = cds.context?.model || cds.model; | ||
| const tenantDocument = ord(csn); |
There was a problem hiding this comment.
Logic Error: Test 4 does not actually assert that cds.context.model is used over cds.model
The test manually computes const csn = cds.context?.model || cds.model at line 119 and then passes csn directly into ord(csn) at line 120. This means the test is calling ord() with the extended model as an explicit argument—it does not verify that ord() itself (or the service handler) reads from cds.context.model. The test would pass even if nothing in the production code ever reads cds.context.
To meaningfully validate the tenant-aware path, the test should call ord() with no arguments (or leave cds.model pointing at baseModel) and check the returned document reflects the extended model—relying on the production code to pull cds.context.model internally.
| // With context model set: ord() should use the extended model | |
| cds.context = { model: extendedModel }; | |
| const csn = cds.context?.model || cds.model; | |
| const tenantDocument = ord(csn); | |
| // With context model set: ord() should use the extended model. | |
| // Set cds.model to baseModel so the only way to get the extended | |
| // model is via cds.context.model (which the production code reads). | |
| cds.model = baseModel; | |
| cds.context = { model: extendedModel }; | |
| // Call ord() without an explicit model to exercise the context path | |
| const tenantDocument = ord(); | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| describe("2. ord-service.js ORD document handler reads cds.context.model", () => { | ||
| test("ord() is called with cds.context.model when available", async () => { | ||
| const ordServiceSource = fs.readFileSync(path.join(__dirname, "../../lib/ord-service.js"), "utf8"); | ||
|
|
||
| // The handler must use cds.context?.model || cds.model (not just cds.model) | ||
| expect(ordServiceSource).toContain("cds.context?.model || cds.model"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("3. ord-service.js uses CDS middlewares for tenant context", () => { | ||
| test("ord-service.js mounts routes through cds.middlewares (not directly on cds.app.get)", () => { | ||
| const ordServiceSource = fs.readFileSync(path.join(__dirname, "../../lib/ord-service.js"), "utf8"); | ||
|
|
||
| // Must use cds.middlewares.before to populate cds.context.tenant/model | ||
| expect(ordServiceSource).toContain("cds.middlewares.before"); | ||
| expect(ordServiceSource).toContain("cds.middlewares.after"); | ||
|
|
||
| // The tenant-aware routes (ORD document + metadata) must NOT be registered | ||
| // directly via cds.app.get — they should go through the Router + middleware stack. | ||
| // Only the config endpoint (this.path) is allowed to be on cds.app directly. | ||
| const appGetCalls = [...ordServiceSource.matchAll(/cds\.app\.get\(/g)]; | ||
| // Exactly one cds.app.get call: the config endpoint (this.path) | ||
| expect(appGetCalls).toHaveLength(1); | ||
| expect(ordServiceSource).toMatch(/cds\.app\.get\(`\$\{this\.path\}`/); | ||
|
|
||
| // The ORD document endpoint must be on the router, not cds.app | ||
| expect(ordServiceSource).toContain("router.get(`/v1/documents/ord-document`"); | ||
|
|
||
| // The router must be mounted on /ord with CDS middlewares | ||
| expect(ordServiceSource).toContain( | ||
| 'cds.app.use("/ord", cds.middlewares.before, router, cds.middlewares.after)', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Logic Error: Source-text inspection in tests 2 and 3 are brittle regression guards, not behavioral tests
Tests 2 and 3 grep the raw source of ord-service.js for specific string literals (e.g. "cds.context?.model || cds.model", "cds.middlewares.before", exact mount string). This approach:
- Breaks on any equivalent refactor (e.g. extracting a helper, whitespace change, renaming a variable) without any change in behaviour.
- Passes even if the code is present but dead (guarded by a never-true condition).
- Does not guard against
cds.middlewaresbeingundefinedat runtime—the middleware mount would silently fail or throw.
Should replace with behavioral tests: spin up a minimal Express app, mount the service, and assert that routes respond correctly with and without cds.context.model set—similar to the integration tests already present in __tests__/integration/.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| router.get(`/v1`, authMiddleware, metadataHandler); | ||
|
|
||
| // Mount the router with CDS middlewares so tenant/model context is set | ||
| cds.app.use("/ord", cds.middlewares.before, router, cds.middlewares.after); |
There was a problem hiding this comment.
Bug: cds.middlewares may be undefined in non-MTX / older CDS environments
cds.middlewares.before and cds.middlewares.after are only available when the CDS framework has initialised its middleware stack (typically in CAP 7+ with MTX). In environments where cds.middlewares is undefined (e.g. older CDS versions, unit-test environments, or apps that opt out of the middleware stack), this line will throw TypeError: Cannot read properties of undefined (reading 'before') and the entire service will fail to start.
Consider guarding the mount:
if (cds.middlewares) {
cds.app.use("/ord", cds.middlewares.before, router, cds.middlewares.after);
} else {
cds.app.use("/ord", router);
}| cds.app.use("/ord", cds.middlewares.before, router, cds.middlewares.after); | |
| // Mount the router with CDS middlewares so tenant/model context is set | |
| if (cds.middlewares) { | |
| cds.app.use("/ord", cds.middlewares.before, router, cds.middlewares.after); | |
| } else { | |
| cds.app.use("/ord", router); | |
| } | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Summary
cds.middlewares.before/afterso thatcds.context.tenantandcds.context.modelare populated per requestcds push) or feature toggles, the ORD document and metadata (OpenAPI, EDMX) reflect the tenant-specific model/.well-known/open-resource-discovery) remains outside middleware per ORD specCloses #389