feat: Implement Extension API for Integration Dependencies and add related tests#399
feat: Implement Extension API for Integration Dependencies and add related tests#399f-nie wants to merge 10 commits intocap-js:mainfrom
Conversation
![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 a well-structured Extension Registry pattern with good test coverage. There are four issues to address: the most critical is the spread ordering in createEventIntegrationDependency that allows user config to silently overwrite computed structural fields (including the aspects array containing the provider-merged event resources); the JSDoc example in cds-plugin.js documents the wrong provider contract which will mislead consumers; a noisy Logger.log fires on every ORD generation for the legitimate "no events subscribed" case; and one test block exercises only inline test logic rather than any plugin code.
PR Bot Information
Version: 1.18.10 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Event Trigger:
pull_request.opened - Correlation ID:
aefffa00-222d-11f1-996c-2f03946238c3 - LLM:
anthropic--claude-4.6-sonnet - Agent Instructions:
lib/integration-dependency.js
Outdated
| return { | ||
| ordId: `${appConfig.ordNamespace}:${ORD_RESOURCE_TYPE.integrationDependency}:${EVENT_INTEGRATION_DEPENDENCY_RESOURCE_NAME}:v1`, | ||
| title: "Consumed Events", | ||
| version: "1.0.0", | ||
| releaseStatus: "active", | ||
| visibility: RESOURCE_VISIBILITY.public, | ||
| mandatory: false, | ||
| partOfPackage: packageId, | ||
| aspects: [ | ||
| { | ||
| title: "Subscribed Event Types", | ||
| mandatory: false, | ||
| eventResources: allEventResources, | ||
| ...eventIntegrationDepConfig.aspect, // Allow aspect customization | ||
| }, | ||
| ], | ||
| ...eventIntegrationDepConfig, // Allow top-level customization (except computed fields) |
There was a problem hiding this comment.
Bug: Spread of eventIntegrationDepConfig can silently overwrite computed fields
...eventIntegrationDepConfig is spread at the top level of the returned object after aspects, ordId, partOfPackage, etc. are already set. This means a user who places any of these keys (e.g. ordId, partOfPackage, aspects) inside cdsrc.json's eventIntegrationDependency section will overwrite the values that were intentionally computed by this function — including the structured aspects array containing all provider-merged eventResources.
The same pattern exists in createIntegrationDependency for integrationDepConfig, so this is a pre-existing issue surfaced by the new code. However the aspects array here is the critical payload of the new feature and is silently overridable.
Consider placing the spread before the computed fields, or explicitly omitting dangerous keys:
| return { | |
| ordId: `${appConfig.ordNamespace}:${ORD_RESOURCE_TYPE.integrationDependency}:${EVENT_INTEGRATION_DEPENDENCY_RESOURCE_NAME}:v1`, | |
| title: "Consumed Events", | |
| version: "1.0.0", | |
| releaseStatus: "active", | |
| visibility: RESOURCE_VISIBILITY.public, | |
| mandatory: false, | |
| partOfPackage: packageId, | |
| aspects: [ | |
| { | |
| title: "Subscribed Event Types", | |
| mandatory: false, | |
| eventResources: allEventResources, | |
| ...eventIntegrationDepConfig.aspect, // Allow aspect customization | |
| }, | |
| ], | |
| ...eventIntegrationDepConfig, // Allow top-level customization (except computed fields) | |
| // Destructure to prevent eventIntegrationDepConfig from overriding computed structural fields | |
| const { aspect: _, aspects: _aspects, ordId: _ordId, partOfPackage: _pkg, ...safeTopLevelConfig } = eventIntegrationDepConfig; | |
| return { | |
| ordId: `${appConfig.ordNamespace}:${ORD_RESOURCE_TYPE.integrationDependency}:${EVENT_INTEGRATION_DEPENDENCY_RESOURCE_NAME}:v1`, | |
| title: "Consumed Events", | |
| version: "1.0.0", | |
| releaseStatus: "active", | |
| visibility: RESOURCE_VISIBILITY.public, | |
| mandatory: false, | |
| partOfPackage: packageId, | |
| ...safeTopLevelConfig, // Allow top-level customization (except structural/computed fields) | |
| aspects: [ | |
| { | |
| title: "Subscribed Event Types", | |
| mandatory: false, | |
| ...eventIntegrationDepConfig.aspect, // Allow aspect customization | |
| eventResources: allEventResources, | |
| }, | |
| ], | |
| }; |
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
| * @example | ||
| * const ord = require("@cap-js/ord"); | ||
| * ord.registerIntegrationDependencyProvider(() => ({ | ||
| * namespace: "sap.s4", | ||
| * events: ["sap.s4.beh.salesorder.v1.SalesOrder.Changed.v1"] | ||
| * })); |
There was a problem hiding this comment.
Bug: JSDoc example in cds-plugin.js shows the old provider contract, not the one actually implemented
The @example block documents the provider returning { namespace, events }, but the actual contract (as implemented in extensionRegistry.js and all tests) is { eventResources: [{ ordId, events }] }. A consumer reading only this file will write a non-functional provider that silently produces no output (filtered out in getProvidedIntegrationDependencies).
Should update the example to match the real contract:
| * @example | |
| * const ord = require("@cap-js/ord"); | |
| * ord.registerIntegrationDependencyProvider(() => ({ | |
| * namespace: "sap.s4", | |
| * events: ["sap.s4.beh.salesorder.v1.SalesOrder.Changed.v1"] | |
| * })); | |
| * @example | |
| * const ord = require("@cap-js/ord"); | |
| * ord.registerIntegrationDependencyProvider(() => ({ | |
| * eventResources: [ | |
| * { | |
| * ordId: "sap.s4:eventResource:CE_SALESORDEREVENTS:v1", | |
| * events: ["sap.s4.beh.salesorder.v1.SalesOrder.Changed.v1"] | |
| * } | |
| * ] | |
| * })); | |
| */ |
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
lib/extension-registry.js
Outdated
| .filter((result) => { | ||
| // Validate result structure - must have eventResources array | ||
| if (!result) return false; | ||
| if (!Array.isArray(result.eventResources) || result.eventResources.length === 0) { | ||
| Logger.log("Integration Dependency provider returned no eventResources"); |
There was a problem hiding this comment.
Logic Error: getProvidedIntegrationDependencies filters out results with an empty eventResources array with a Logger.log, but the same empty-array case is also checked (and silently ignored) in createEventIntegrationDependency at line 165–175. This is fine for defence-in-depth, but the Logger.log message "Integration Dependency provider returned no eventResources" fires even for a provider that returned { eventResources: [] } after a legitimate "no events subscribed right now" state — which is a valid runtime condition, not something to warn about.
More critically, a provider that returns { eventResources: [] } will cause this log to fire at INFO level on every ORD document generation even when the application has simply not subscribed to any events. Consider silently dropping empty results without logging, or logging at debug level only.
| .filter((result) => { | |
| // Validate result structure - must have eventResources array | |
| if (!result) return false; | |
| if (!Array.isArray(result.eventResources) || result.eventResources.length === 0) { | |
| Logger.log("Integration Dependency provider returned no eventResources"); | |
| .filter((result) => { | |
| // Validate result structure - must have non-empty eventResources array | |
| if (!result) return false; | |
| if (!Array.isArray(result.eventResources) || result.eventResources.length === 0) { | |
| return false; | |
| } | |
| return true; | |
| }); |
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("Event Broker config-based eventResources simulation", () => { | ||
| it("should build eventResources from config and subscribed events", () => { | ||
| // Simulate the Event Broker's buildEventResourcesFromConfig logic | ||
| const eventResourcesConfig = [ | ||
| { | ||
| ordId: "sap.s4:eventResource:CE_BUSINESSPARTNEREVENTS:v1", | ||
| eventTypes: ["sap.s4.beh.businesspartner.v1.BusinessPartner.Changed.v1"], | ||
| }, | ||
| ]; | ||
| const subscribedEvents = ["sap.s4.beh.businesspartner.v1.BusinessPartner.Changed.v1"]; | ||
|
|
||
| // Simulate matching logic - Event Broker returns {ordId, events} | ||
| const subscribedSet = new Set(subscribedEvents); | ||
| const eventResources = []; | ||
|
|
||
| for (const config of eventResourcesConfig) { | ||
| const matchedEventTypes = config.eventTypes.filter((et) => subscribedSet.has(et)); | ||
| if (matchedEventTypes.length > 0) { | ||
| eventResources.push({ | ||
| ordId: config.ordId, | ||
| events: matchedEventTypes, // Simple array, not subset structure | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| expect(eventResources).toHaveLength(1); | ||
| expect(eventResources[0].ordId).toBe("sap.s4:eventResource:CE_BUSINESSPARTNEREVENTS:v1"); | ||
| expect(eventResources[0].events).toHaveLength(1); | ||
| expect(eventResources[0].events[0]).toBe("sap.s4.beh.businesspartner.v1.BusinessPartner.Changed.v1"); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Logic Error: The last describe block ("Event Broker config-based eventResources simulation") does not test any code from the ORD plugin — it only exercises inline logic written directly in the test body. This means it provides no coverage guarantee for the actual integration between Event Broker and the ORD plugin, and could pass even if getIntegrationDependencies or extensionRegistry were completely broken.
Consider either removing this test (it tests a hypothetical external plugin's internal logic) or converting it into an actual assertion against getIntegrationDependencies by registering a provider that simulates this matching logic.
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
- Resolve conflicts in copilot-instructions.md, systemPatterns.md, integration-dependency.test.js - Rename extensionRegistry.js to extension-registry.js (consistent naming convention) - Rename extension registry test files to kebab-case - Update all imports to use new kebab-case file names
- Prevent config spread from silently overwriting computed fields (ordId, partOfPackage, aspects)
Use _.omit to exclude dangerous keys before spreading user config
- Fix JSDoc example in cds-plugin.js to show correct provider contract
Old: { namespace, events } -> New: { eventResources: [{ ordId, events }] }
- Change Logger.log to Logger.debug for empty eventResources
Empty array is valid runtime state (no events subscribed)
- Convert simulation test to actual integration test
Now registers provider and tests via getIntegrationDependencies
SummaryThe following content is AI-generated and provides a summary of the pull request: Implement Extension API for Integration DependenciesNew Feature✨ Introduces an Extension Registry pattern that enables external plugins (e.g., The design follows a loose-coupling approach (similar to the Java plugin's Spring DI Provider Registration API (for external plugins): const ord = require("@cap-js/ord");
ord.registerIntegrationDependencyProvider(() => ({
eventResources: [
{
ordId: "sap.s4:eventResource:CE_SALESORDEREVENTS:v1",
events: ["sap.s4.beh.salesorder.v1.SalesOrder.Changed.v1"],
},
],
}));Changes
Jira IssuesGitHub Issues:
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
There was a problem hiding this comment.
The PR introduces a well-structured Extension Registry pattern, but has a critical documentation bug: the README's provider contract example and description show the wrong API shape ({ namespace, events } instead of { eventResources: [{ ordId, events }] }), which would cause silent failures for any external plugin author following the docs. Additionally, the memory bank has a duplicate section number and an incorrect file path reference. The redundant guard in createEventIntegrationDependency should also be cleaned up for clarity.
PR Bot Information
Version: 1.19.1 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.6-sonnet - Agent Instructions:
- Correlation ID:
d54410a0-23b0-11f1-8328-7f939c0f1521 - Event Trigger:
pull_request.ready_for_review
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
|
After reviewing the PR and getting myself familiar with the context - I do believe that this new feature is having a great overlap with the |
2 participants
Closes #372, Closes #208
Summary
This PR introduces an Extension Registry pattern that enables external plugins (e.g.,
@cap-js/event-broker) to register Integration Dependency providers with the ORD plugin at runtime.Motivation
Applications using
@cap-js/event-brokerto consume external CloudEvents should be able to document these dependencies in their ORD metadata. The original issue proposed adding Event Broker detection logic directly to the ORD plugin. However, this approach would create tight coupling between plugins.Instead, this PR implements a provider registry pattern (similar to the Java plugin's Spring DI
OrdIntegrationDependencyProviderbeans) that allows external plugins to contribute Integration Dependency data without modifying the ORD plugin.Changes
New Files
lib/extensionRegistry.jsModified Files
lib/integrationDependency.jscreateEventIntegrationDependency()that consumes provider datacds-plugin.jsregisterIntegrationDependencyProvider()for external pluginsmemory-bank/systemPatterns.md.github/copilot-instructions.mdNew Tests
__tests__/unit/extensionRegistry.test.js__tests__/unit/extensionRegistry.integration.test.jsAPI
Provider Registration (for external plugins)
Provider Contract
Providers return
{ eventResources: [{ ordId, events }] }ornull:ordId: The external event resource ORD ID (from SAP Business Accelerator Hub)events: Array of event type names that the application subscribes to (multiple events can belong to the same event resource)The ORD plugin transforms this into the final ORD
subsetstructure:{ "integrationDependencies": [ { "ordId": "customer.app:integrationDependency:consumedEvents:v1", "title": "Consumed Events", "aspects": [ { "eventResources": [ { "ordId": "sap.s4:eventResource:CE_SALESORDEREVENTS:v1", "subset": [ { "eventType": "sap.s4.beh.salesorder.v1.SalesOrder.Changed.v1" }, { "eventType": "sap.s4.beh.salesorder.v1.SalesOrder.Created.v1" } ] } ] } ] } ] }Design Decisions
Why Extension Registry instead of direct Event Broker detection?
The Extension Registry pattern:
Why providers return simple
{ ordId, events }instead of full ORD structure?Runtime-Only Support
Like the Java plugin, Integration Dependencies from providers are only available at runtime (
cds.once('served')). Build-time generation would require a different mechanism (not implemented).Testing
All 37 tests pass.
Checklist
systemPatterns.md)References
Related PR for Event Broker plugin to consume the extension registry: cap-js/event-broker#48