feat: support ORD document perspective configuration#392
feat: support ORD document perspective configuration#392zongqichen wants to merge 1 commit intomainfrom
Conversation
Users can now configure the ORD document perspective and described system
metadata via .cdsrc.json:
{ "ord": {
"perspective": "system-version",
"describedSystemVersion": { "version": "1.2.0" },
"describedSystemInstance": { "baseUrl": "https://..." },
"describedSystemType": { "systemNamespace": "customer.myapp" }
}}
- perspective field added to both config endpoint and ORD document
- Invalid perspective values are dropped with a warning
- describedSystemVersion/Instance/Type passthrough to ORD document
- Backward compatible: no changes when perspective is not configured
Closes #272
Closes #185
SummaryThe following content is AI-generated and provides a summary of the pull request: feat: Support ORD Document Perspective ConfigurationNew Feature✨ Adds support for configuring the Configuration example ( {
"ord": {
"perspective": "system-version",
"describedSystemVersion": { "version": "1.2.0" }
}
}Changes
GitHub 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 PR introduces perspective support in a clean, backward-compatible way, but has three substantive issues: the config endpoint emits unvalidated perspective strings while the ORD document silently drops them (inconsistency); the afterEach teardown will crash if cds.env.ord is undefined; and the described-system companion fields (describedSystemVersion, describedSystemInstance, describedSystemType) are emitted even when no valid perspective is present, which would produce a spec-violating document.
PR Bot Information
Version: 1.18.5 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - Correlation ID:
14868b80-1e43-11f1-867d-cbc0069693e1 - Agent Instructions:
| if (ordConfig?.perspective) { | ||
| documentEntry.perspective = ordConfig.perspective; |
There was a problem hiding this comment.
Bug: perspective in the config endpoint document entry is not validated against ALLOWED_PERSPECTIVES
In lib/ord.js, _getPerspectiveFields validates the perspective value against ALLOWED_PERSPECTIVES before writing it into the ORD document. However, defaults.baseTemplate blindly copies whatever ordConfig.perspective contains into the config endpoint response — including invalid values. An operator who misconfigures .cdsrc.json with perspective: "invalid-value" will get the invalid string silently emitted at the config endpoint while being dropped (with a warning) from the document, creating an inconsistency.
Consider importing and checking ALLOWED_PERSPECTIVES here, mirroring the validation in _getPerspectiveFields:
| if (ordConfig?.perspective) { | |
| documentEntry.perspective = ordConfig.perspective; | |
| if (ordConfig?.perspective && ALLOWED_PERSPECTIVES.includes(ordConfig.perspective)) { | |
| documentEntry.perspective = ordConfig.perspective; | |
| } |
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
| afterEach(() => { | ||
| delete cds.env.ord.perspective; | ||
| delete cds.env.ord.describedSystemVersion; | ||
| delete cds.env.ord.describedSystemInstance; | ||
| delete cds.env.ord.describedSystemType; | ||
| }); |
There was a problem hiding this comment.
Bug: afterEach will throw if cds.env.ord is undefined or null
If cds.env.ord happens to be undefined (e.g. the test environment initialises without a .cdsrc.json that sets the ord key), then delete cds.env.ord.perspective throws a TypeError: Cannot read properties of undefined. The existing pattern in the rest of the test file uses cds.env.ord = {} as a reset guard.
Consider guarding the deletions:
| afterEach(() => { | |
| delete cds.env.ord.perspective; | |
| delete cds.env.ord.describedSystemVersion; | |
| delete cds.env.ord.describedSystemInstance; | |
| delete cds.env.ord.describedSystemType; | |
| }); | |
| afterEach(() => { | |
| if (cds.env.ord) { | |
| delete cds.env.ord.perspective; | |
| delete cds.env.ord.describedSystemVersion; | |
| delete cds.env.ord.describedSystemInstance; | |
| delete cds.env.ord.describedSystemType; | |
| } | |
| }); |
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
| const _getPerspectiveFields = (appConfig) => { | ||
| const result = {}; | ||
| const perspective = appConfig.env?.perspective; | ||
|
|
||
| if (perspective) { | ||
| if (ALLOWED_PERSPECTIVES.includes(perspective)) { | ||
| result.perspective = perspective; | ||
| } else { | ||
| Logger.warn(`Invalid perspective "${perspective}". Allowed: ${ALLOWED_PERSPECTIVES.join(", ")}`); | ||
| } | ||
| } | ||
|
|
||
| if (appConfig.env?.describedSystemVersion) { | ||
| result.describedSystemVersion = appConfig.env.describedSystemVersion; | ||
| } | ||
| if (appConfig.env?.describedSystemInstance) { | ||
| result.describedSystemInstance = appConfig.env.describedSystemInstance; | ||
| } | ||
| if (appConfig.env?.describedSystemType) { | ||
| result.describedSystemType = appConfig.env.describedSystemType; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Logic Error: describedSystemVersion, describedSystemInstance, and describedSystemType are passed through unconditionally regardless of whether perspective is valid or even set
The ORD spec requires these fields to be paired with a valid perspective value (e.g. describedSystemVersion is only meaningful with perspective: "system-version"). As currently written, a caller can omit perspective (or provide an invalid one) and still have describedSystemVersion / describedSystemInstance / describedSystemType emitted in the document, producing a malformed ORD document.
Consider only emitting the companion fields when a valid perspective is present:
| const _getPerspectiveFields = (appConfig) => { | |
| const result = {}; | |
| const perspective = appConfig.env?.perspective; | |
| if (perspective) { | |
| if (ALLOWED_PERSPECTIVES.includes(perspective)) { | |
| result.perspective = perspective; | |
| } else { | |
| Logger.warn(`Invalid perspective "${perspective}". Allowed: ${ALLOWED_PERSPECTIVES.join(", ")}`); | |
| } | |
| } | |
| if (appConfig.env?.describedSystemVersion) { | |
| result.describedSystemVersion = appConfig.env.describedSystemVersion; | |
| } | |
| if (appConfig.env?.describedSystemInstance) { | |
| result.describedSystemInstance = appConfig.env.describedSystemInstance; | |
| } | |
| if (appConfig.env?.describedSystemType) { | |
| result.describedSystemType = appConfig.env.describedSystemType; | |
| } | |
| return result; | |
| const _getPerspectiveFields = (appConfig) => { | |
| const result = {}; | |
| const perspective = appConfig.env?.perspective; | |
| if (!perspective) { | |
| return result; | |
| } | |
| if (!ALLOWED_PERSPECTIVES.includes(perspective)) { | |
| Logger.warn(`Invalid perspective "${perspective}". Allowed: ${ALLOWED_PERSPECTIVES.join(", ")}`); | |
| return result; | |
| } | |
| result.perspective = perspective; | |
| if (appConfig.env?.describedSystemVersion) { | |
| result.describedSystemVersion = appConfig.env.describedSystemVersion; | |
| } | |
| if (appConfig.env?.describedSystemInstance) { | |
| result.describedSystemInstance = appConfig.env.describedSystemInstance; | |
| } | |
| if (appConfig.env?.describedSystemType) { | |
| result.describedSystemType = appConfig.env.describedSystemType; | |
| } | |
| return result; | |
| }; |
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
perspectivefield to ORD config endpoint and ORD documentdescribedSystemVersion,describedSystemInstance,describedSystemTypepassthrough from.cdsrc.jsonConfiguration example (.cdsrc.json)
{ "ord": { "perspective": "system-version", "describedSystemVersion": { "version": "1.2.0" } } }Closes #272
Closes #185