Skip to content

Conversation

tyrellshawn
Copy link

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 value true when emitting events for services that have a systemId in their credentials. This header is required for compliance with SAP event specifications when using the event-connectivity plan for customers.

Changes

  • Modified emitToEventBroker() method to accept an optional addComplianceHeader boolean parameter
  • Updated handle() method to determine when compliance header should be added based on presence of credentials.systemId
  • Added conditional header logic to include ce-xsapcomplianteventspec: true when systemId is present in service credentials
  • Updated test configuration to include systemId: "321" in event-broker-ias-single-tenant default-env.json
  • Enhanced test expectations to verify the presence of ce-xsapcomplianteventspec header in emitted events

Technical Details

The implementation uses a clean, encapsulated approach:

  • Services with credentials.systemId (event-connectivity plan) → adds compliance header
  • Services without credentials.systemId (event-mesh-multi-tenant plan) → no compliance header
  • Boolean parameter pattern maintains separation of concerns

await this.emitToEventBroker(_msg)

// Determine if we should add the xsapcomplianteventspec header based on systemId
const shouldAddComplianceHeader = !!this.options.credentials.systemId

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

Copy link
Author

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

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

Copy link

@i-kushnir i-kushnir left a 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.

@tyrellshawn
Copy link
Author

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
When I recorded the teams meeting I was using this branch for testing and included it as a reference in the teams channel. One thing I would suggest here is that for the line:
const systemId = credentials.systemId? credentials.systemId : cds.context.tenant

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants