-
Notifications
You must be signed in to change notification settings - Fork 5
feat(event-broker): add compliance header support for event emission #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
await this.emitToEventBroker(_msg) | ||
|
||
// Determine if we should add the xsapcomplianteventspec header based on systemId | ||
const shouldAddComplianceHeader = !!this.options.credentials.systemId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just move this shouldAddComplianceHeader into emitToEventBroker directly ?
also rename it to: isProducerConfigured. Also maybe we should just always sent this header, regardless of the systemID. ce-xsapcomplianteventspec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense I think the only issue is if people aren't expecting this header in consumption from an mt app we would be sending an additional field that "could" be a risk. I think the other suggestions are fine tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a discussion with Claude. The goal of adding Compliance Header is for the Developer to be informed and make a confirmation that the Event structure is compliant. This check with SystemId doesn't satisfy the goal.
So we should create a NEW boolean configuration parameter sapCompliantEvent
under the
"requires": {
"kinds": {
"event-broker": {
```
And enable setting the header based on the true/false value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we are missing appending SystemId to the CE_source which has only 2 segments for EC plan.
Yes correct seems those changes were only present in this branch. https://github.com/tyrellshawn/event-broker/blob/testing/cds-plugin.js#L279 We could have const systemId = cds.context.tenant? cds.context.tenant : credentials.systemId. The latter suggestion makes more sense in my mind because if the plan supports multi tenancy later the developer would need to provide the tenant in the context and that would take precedence over the systemId else we fall back on the systemId which should be present. We can be safe here as well. If the systemId is still undefined at this point for both cases (credential.systemId and context tenant both empty) we could throw an error but these are just suggestion so take with a grain of salt |
Summary
Adds support for
ce-xsapcomplianteventspec
header in Event Hub emissions for event-connectivity plan.The Event Hub service now conditionally adds the
ce-xsapcomplianteventspec
header with valuetrue
when emitting events for services that have asystemId
in their credentials. This header is required for compliance with SAP event specifications when using the event-connectivity plan for customers.Changes
emitToEventBroker()
method to accept an optionaladdComplianceHeader
boolean parameterhandle()
method to determine when compliance header should be added based on presence ofcredentials.systemId
ce-xsapcomplianteventspec: true
whensystemId
is present in service credentialssystemId: "321"
in event-broker-ias-single-tenant default-env.jsonce-xsapcomplianteventspec
header in emitted eventsTechnical Details
The implementation uses a clean, encapsulated approach:
credentials.systemId
(event-connectivity plan) → adds compliance headercredentials.systemId
(event-mesh-multi-tenant plan) → no compliance header