Skip to content

feat: rejection events#404

Open
ansgarlichter wants to merge 9 commits intocap-js:mainfrom
ansgarlichter:feat/emit-events-for-security-relevant-rejections
Open

feat: rejection events#404
ansgarlichter wants to merge 9 commits intocap-js:mainfrom
ansgarlichter:feat/emit-events-for-security-relevant-rejections

Conversation

@ansgarlichter
Copy link
Contributor

Closes #400.

If you do not see the feature request as a valid enhancement, just close the PR.

@ansgarlichter ansgarlichter requested a review from a team as a code owner March 13, 2026 09:00
Copy link
Contributor

@schiwekM schiwekM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the changes. We will add docs to the readme to document the events and then the PR is good to go.

Co-authored-by: Marten Schiwek <marten.schiwek@sap.com>
Co-authored-by: Marten Schiwek <marten.schiwek@sap.com>
Co-authored-by: Marten Schiwek <marten.schiwek@sap.com>
@ansgarlichter
Copy link
Contributor Author

Thanks again for the changes. We will add docs to the readme to document the events and then the PR is good to go.

I just had another point, when I saw your review: would it be beneficial to emit those events inside cds.spawn so that errors inside those event handlers do not crash the attachment's request? As an example: if logging to audit log fails in the event handler, the user should still get the response properly in the plugins POV.

@schiwekM
Copy link
Contributor

That should be the case ootb. Emit should write the events to the outbox, the transaction continues. And the outbox picks up the event, starts a new transaction and calls the handler

@ansgarlichter
Copy link
Contributor Author

That should be the case ootb. Emit should write the events to the outbox, the transaction continues. And the outbox picks up the event, starts a new transaction and calls the handler

Just for clarification - in the docs for the outbox (see https://pages.github.tools.sap/cap/docs/node.js/queue#persistent-queue):

Using the persistent queue, the to-be-emitted message is stored in a database table within the current transaction, therefore transactional consistency is guaranteed.
Once the transaction succeeds, the messages are read from the database table and dispatched. If processing was successful, the respective message is deleted from the database table. If processing failed, the system retries the message after exponentially increasing delays.

If I understand this correctly, the event won't be written to the outbox but the request's transaction is rolled back via req.reject the transaction is not committed. Therefore, the event is never transmitted. So we would need cds.spawn or immediate events if the emit method offers a param for this?

@schiwekM
Copy link
Contributor

Ahh, so you want to even emit the event in cases where the transaction is rolled back? Yes in that case we need to wrap it inside cds.spawn

@ansgarlichter
Copy link
Contributor Author

Ahh, so you want to even emit the event in cases where the transaction is rolled back? Yes in that case we need to wrap it inside cds.spawn

Yes, if you look in the code, the request gets rejected if for example the attachment is too large. I will adapt it accordingly.

…ithub.com:ansgarlichter/attachments into feat/emit-events-for-security-relevant-rejections
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.

AttachmentsService: Emit observable events for security-relevant upload/download rejections

2 participants