Improve design document by holgpar · Pull Request #906 · cap-java/cds-feature-attachments

holgpar · 2026-10-15T14:23:19Z

docs: explain event handling
docs: fix referenced class names
docs: more information on draft handling
docs: more improvements on draft activation
docs: improvements and rewrites on malware scanning

some classes refered to in the document got renamed

hyperspace-pr-bot · 2026-10-15T14:23:37Z

Summary

The following content is AI-generated and provides a summary of the pull request:

Improve Attachment Design Documentation

Documentation

📚 Clarifies the attachment feature architecture and runtime behavior, with expanded guidance for CAP event mapping, draft activation, storage backends, multi-tenancy, and malware scanning.

Changes

doc/Design.md:
- Explained how CAP events map to AttachmentService operations through ModifyAttachmentEventFactory strategies.
- Expanded draft activation documentation, including deep updates, deletion handling, contentId semantics, readonly fields, and draft/active sibling navigation.
- Clarified the internal versus external storage contract and CAP’s role in persisting attachment content.
- Documented automatic tenant propagation through the CAP request context.
- Added malware-scanning user experience details, asynchronous scan behavior, external-storage content retrieval, status validation, and rescan limitations.
- Corrected outdated class names, terminology, headings, grammar, and the table of contents.

Images and Links

🔄 Regenerate and Update Summary
✏️ Insert as PR Description (deletes this comment)
🗑️ Delete comment

PR Bot Information

Version: 1.33.5

Summary Prompt: Default Prompt
Correlation ID: f8eecca0-c8a3-11f1-87cc-bb99223b8db1
Output Template: Default Template
File Content Strategy: Full file content
LLM: gpt-5.6-sol
Event Trigger: pull_request.opened

hyperspace-pr-bot

The document is substantially clearer, but several references and lifecycle descriptions needed correction to avoid broken navigation and misleading extension guidance. Inline comments include targeted fixes.

PR Bot Information

Version: 1.33.5

Correlation ID: f8eecca0-c8a3-11f1-87cc-bb99223b8db1
Agent Instructions:
- CLAUDE.md
File Content Strategy: Full file content
LLM: gpt-5.6-sol
Event Trigger: pull_request.opened

hyperspace-pr-bot · 2026-10-15T14:24:13Z

 After the handler has checked if an attachment entity is contained in the request,
 the handler calls the `ModifyAttachmentEventFactory` implementation.
-The implementation of this interface, `DefaultModifyAttachmentEventFactory`, checks
+The implementation of this interface, `ModifyAttachmentEventFactory`, checks


Clarity: ModifyAttachmentEventFactory is a concrete class, not an implementation of an interface, so this description gives readers the wrong type hierarchy. Instead, describe the factory directly.

Suggested change

The implementation of this interface, `ModifyAttachmentEventFactory`, checks

The `ModifyAttachmentEventFactory` checks

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

hyperspace-pr-bot · 2026-10-15T14:24:14Z

+| ------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `CreateAttachmentEvent`        | Emits the `CREATE_ATTACHMENT` event on the `AttachmentService`                                                                                                                                                           |
+| `DoNothingAttachmentEvent`     | Used when the attachment entity is included in the request but nothing needs to be done, e.g. a field is updated which is not the `content` or `contentId` field                                                         |
+| `MarkAsDeletedAttachmentEvent` | Emits the `MARK_AS_DELETED` event on the `AttachmentService`; skipped during `DRAFT_PATCH` events from the `DraftService` — in that case the mark as deleted is triggered when the draft is activated                    |


Logic Error: This names the wrong service event and incorrectly says deferred deletion occurs on draft activation; the service event is MARK_ATTACHMENT_AS_DELETED, and the draft-patch content is cleaned up on draft cancellation by DraftCancelAttachmentsHandler. This could mislead plugin implementers about which event to handle and when deletion occurs.

Suggested change

| `MarkAsDeletedAttachmentEvent` | Emits the `MARK_AS_DELETED` event on the `AttachmentService`; skipped during `DRAFT_PATCH` events from the `DraftService` — in that case the mark as deleted is triggered when the draft is activated |

| `MarkAsDeletedAttachmentEvent` | Emits the `MARK_ATTACHMENT_AS_DELETED` event on the `AttachmentService`; skipped during `DRAFT_PATCH` events from the `DraftService` — if the draft is cancelled, `DraftCancelAttachmentsHandler` deletes newly uploaded content that is not referenced by the active entity |

Double-check suggestion before committing. Edit this comment for amendments.

Thank you for submitting your feedback: ✅ Helpful comment.

hyperspace-pr-bot · 2026-10-15T14:24:14Z


-For the create event a listener is registered for the end of the transaction to scan the content
-of the attachment for malware.
+For an external storage plugin, `isInternalStored` is left `false`, which


Clarity: External storage handlers explicitly set isInternalStored to false; saying it is “left” false obscures the extension contract that custom handlers must follow. Instead, state that the plugin sets the flag.

Suggested change

For an external storage plugin, `isInternalStored` is left `false`, which

For an external storage plugin, `isInternalStored` is set to `false`, which

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

hyperspace-pr-bot · 2026-10-15T14:24:14Z

 in file `com.sap.cds.services.runtime.CdsRuntimeConfiguration` to make it visible for the CAP Java runtime.

-### Handler
+### Handlers


Bug: Renaming this heading changes its generated anchor to #handlers, but the table of contents still links to #handler, leaving a broken navigation link. Consider updating the TOC entry to [Handlers](#handlers) as part of this rename.

Thank you for submitting your feedback: ✅ Helpful comment.

tomk8r

Much clearer and up to date.

Just some smaller suggestions - you can take or leave them.

tomk8r · 2026-10-16T09:07:17Z

+| ------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `CreateAttachmentEvent`        | Emits the `CREATE_ATTACHMENT` event on the `AttachmentService`                                                                                                                                                           |
+| `DoNothingAttachmentEvent`     | Used when the attachment entity is included in the request but nothing needs to be done, e.g. a field is updated which is not the `content` or `contentId` field                                                         |
+| `MarkAsDeletedAttachmentEvent` | Emits the `MARK_AS_DELETED` event on the `AttachmentService`; skipped during `DRAFT_PATCH` events from the `DraftService` — in that case the mark as deleted is triggered when the draft is activated                    |


We should rather use the bot suggestion. Most accurate (with CLAUDE support) would be:
| MarkAsDeletedAttachmentEvent | Emits the MARK_ATTACHMENT_AS_DELETED event on the AttachmentService; skipped during DRAFT_PATCH events from the DraftService — the external delete is deferred until the draft is either cancelled (DraftCancelAttachmentsHandler) or activated (UpdateAttachmentsHandler.deleteRemovedAttachments).

tomk8r · 2026-10-16T09:34:29Z

-getQualifiedName().
-
-endsWith("_drafts");
+DraftUtils.getActiveEntity(entity);  // returns the active sibling


Suggested change

DraftUtils.getActiveEntity(entity); // returns the active sibling

DraftUtils.getActiveEntity(entity); // returns the active entity, navigating to the sibling if the input is a draft entity

tomk8r · 2026-10-16T09:35:23Z

-
-endsWith("_drafts");
+DraftUtils.getActiveEntity(entity);  // returns the active sibling
+DraftUtils.getDraftEntity(entity);   // returns the draft sibling


Suggested change

DraftUtils.getDraftEntity(entity); // returns the draft sibling

DraftUtils.getDraftEntity(entity); // returns the draft entity, navigating to the sibling if the input is an active entity

tomk8r · 2026-10-16T10:13:18Z

+Tenant information does not need to be passed explicitly when emitting events
+on the `AttachmentService` — CAP propagates it automatically via the
+`RequestContext`, which is available on the current thread.



Suggested change

For asynchronous operations (e.g. the malware scan), a `RequestContextRunner` is captured on the parent thread and replayed on the child thread to restore the tenant context.

holgpar added 5 commits October 15, 2026 10:54

docs: explain event handling

a7547cb

docs: fix referenced class names

bb0dffc

some classes refered to in the document got renamed

docs: more information on draft handling

03a2c39

docs: more improvements on draft activation

9bf3e11

docs: improvements and rewrites on malware scanning

02c5e51

holgpar requested a review from a team as a code owner October 15, 2026 14:23

hyperspace-pr-bot Bot reviewed Oct 15, 2026

View reviewed changes

holgpar requested review from lisajulia and tomk8r October 15, 2026 14:48

tomk8r previously approved these changes Oct 16, 2026

View reviewed changes

review feedback

30a7f6f

holgpar dismissed tomk8r’s stale review via 30a7f6f October 19, 2026 13:49

holgpar requested a review from tomk8r October 20, 2026 08:36

tomk8r approved these changes Oct 20, 2026

View reviewed changes

	The implementation of this interface, `ModifyAttachmentEventFactory`, checks
	The `ModifyAttachmentEventFactory` checks

	\| `MarkAsDeletedAttachmentEvent` \| Emits the `MARK_AS_DELETED` event on the `AttachmentService`; skipped during `DRAFT_PATCH` events from the `DraftService` — in that case the mark as deleted is triggered when the draft is activated \|
	\| `MarkAsDeletedAttachmentEvent` \| Emits the `MARK_ATTACHMENT_AS_DELETED` event on the `AttachmentService`; skipped during `DRAFT_PATCH` events from the `DraftService` — if the draft is cancelled, `DraftCancelAttachmentsHandler` deletes newly uploaded content that is not referenced by the active entity \|

	For an external storage plugin, `isInternalStored` is left `false`, which
	For an external storage plugin, `isInternalStored` is set to `false`, which

	DraftUtils.getActiveEntity(entity); // returns the active sibling
	DraftUtils.getActiveEntity(entity); // returns the active entity, navigating to the sibling if the input is a draft entity

	DraftUtils.getDraftEntity(entity); // returns the draft sibling
	DraftUtils.getDraftEntity(entity); // returns the draft entity, navigating to the sibling if the input is an active entity


	For asynchronous operations (e.g. the malware scan), a `RequestContextRunner` is captured on the parent thread and replayed on the child thread to restore the tenant context.

Conversation

holgpar commented Oct 15, 2026

Uh oh!

hyperspace-pr-bot Bot commented Oct 15, 2026

Summary

Improve Attachment Design Documentation

Documentation

Changes

Images and Links

Uh oh!

hyperspace-pr-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

hyperspace-pr-bot Bot Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

hyperspace-pr-bot Bot Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyperspace-pr-bot Bot Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

hyperspace-pr-bot Bot Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomk8r left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hyperspace-pr-bot Bot Oct 15, 2026 •

edited

Loading

hyperspace-pr-bot Bot Oct 15, 2026 •

edited

Loading