Support inline single attachments via Attachment type#768
Support inline single attachments via Attachment type#768busehalis-sap wants to merge 5 commits intomainfrom
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Support Inline Single Attachments via
|
![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 inline single-attachment support via the Attachment type, which is a meaningful extension of the existing composition-based approach. However, there are several substantive correctness issues that need attention before merging: a recurring "first prefix wins" bug across CreateAttachmentEvent, MarkAsDeletedAttachmentEvent, and DefaultAttachmentsServiceHandler that will produce wrong behavior for entities with more than one inline attachment field; an inverted/dead branch in ModifyApplicationHandlerHelper for contentId resolution; missing root-entity key columns in AttachmentsReader when both inline and expand columns are present; and a conflation in ReadAttachmentsHandler.getAttachmentAssociations that incorrectly treats inline-only root entities as direct media association roots. These should be resolved before the feature is considered production-ready.
PR Bot Information
Version: 1.19.3 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Event Trigger:
pull_request.opened - Correlation ID:
bbb28610-2bff-11f1-8fd9-1dd0dd2b50a5 - LLM:
anthropic--claude-4.6-sonnet
| private static Optional<String> detectInlinePrefix(Path path) { | ||
| List<String> prefixes = | ||
| ApplicationHandlerHelper.getInlineAttachmentFieldNames(path.target().entity()); | ||
| if (!prefixes.isEmpty() | ||
| && !path.target().entity().getAnnotationValue("_is_media_data", false)) { | ||
| return Optional.of(prefixes.get(0)); |
There was a problem hiding this comment.
Logic Error: detectInlinePrefix always picks the first inline prefix, silently ignoring entities with multiple inline attachment fields.
When an entity has more than one inline Attachment field (e.g. coverImage: Attachment and thumbnail: Attachment), prefixes.get(0) always returns the first one. The CdsDataProcessor converter is already called once per matched content element, so path already points to the specific element being processed. The prefix should be derived from element.getName() (available inside processEvent via the calling path) or, better, passed in from the converter context rather than re-detecting from the entity's full prefix list.
Consider replacing detectInlinePrefix(path) with ApplicationHandlerHelper.getInlineAttachmentPrefix(path.target().entity(), element.getName()), using the actual element name from the converter callback—similar to how ReadAttachmentsHandler.processAfter correctly does it at line 124.
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
| private static Optional<String> detectInlinePrefix(Path path) { | ||
| List<String> prefixes = | ||
| ApplicationHandlerHelper.getInlineAttachmentFieldNames(path.target().entity()); | ||
| if (!prefixes.isEmpty() | ||
| && !path.target().entity().getAnnotationValue("_is_media_data", false)) { | ||
| return Optional.of(prefixes.get(0)); |
There was a problem hiding this comment.
Logic Error: Same multi-inline-field bug as in CreateAttachmentEvent — detectInlinePrefix picks prefixes.get(0) regardless of which field is actually being processed.
If an entity has two inline attachment fields, the delete handler will always clear the first prefix's fields, leaving the second prefix's fields with stale data, and potentially clearing the wrong prefix's fields entirely.
Should derive the prefix from the element name in context, not unconditionally take the first element of the list.
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
| private static Optional<String> detectInlinePrefix(CdsEntity entity) { | ||
| List<String> prefixes = ApplicationHandlerHelper.getInlineAttachmentFieldNames(entity); | ||
| if (!prefixes.isEmpty() && !entity.getAnnotationValue("_is_media_data", false)) { | ||
| return Optional.of(prefixes.get(0)); | ||
| } | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Logic Error: Same first-prefix-wins bug: detectInlinePrefix takes prefixes.get(0) unconditionally.
afterCreateAttachment is called with an AttachmentCreateEventContext that already carries the attachmentEntity. If the entity has multiple inline attachment fields the wrong (or first) prefix will be forwarded to the malware scanner, causing the scanner to update the wrong flattened column after the transaction.
Consider passing the prefix used during the create operation directly via the event context instead of re-deriving it here.
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
| String contentId; | ||
| Optional<String> inlinePrefix = | ||
| ApplicationHandlerHelper.getInlineAttachmentPrefix( | ||
| path.target().entity(), Attachments.CONTENT_ID); | ||
| if (inlinePrefix.isEmpty()) { | ||
| // Try to detect prefix from any inline attachment field present in data | ||
| contentId = | ||
| resolveInlineOrDirectField( | ||
| path.target().values(), path.target().entity(), Attachments.CONTENT_ID); | ||
| } else { | ||
| contentId = | ||
| (String) path.target().values().get(inlinePrefix.get() + "_" + Attachments.CONTENT_ID); | ||
| } | ||
| if (contentId == null) { | ||
| contentId = (String) path.target().values().get(Attachments.CONTENT_ID); | ||
| } |
There was a problem hiding this comment.
Logic Error: The contentId resolution logic is inverted and has a dead branch.
getInlineAttachmentPrefix(entity, Attachments.CONTENT_ID) looks for a prefix whose value starts with "contentId_", but the field name is "contentId" not a prefixed element name. This call will almost always return Optional.empty(), causing the code to always fall into the if (inlinePrefix.isEmpty()) branch. The subsequent if (contentId == null) fallback then re-reads the direct field — this is the exact same field already tried in resolveInlineOrDirectField, making the fallback redundant.
The intent appears to be: resolve contentId from the prefixed field when the entity is an inline-attachment host. Consider instead using getInlineAttachmentPrefix(entity, element.getName()) at the point where the element name is available, or simply call resolveInlineOrDirectField directly and remove the dead else branch.
| String contentId; | |
| Optional<String> inlinePrefix = | |
| ApplicationHandlerHelper.getInlineAttachmentPrefix( | |
| path.target().entity(), Attachments.CONTENT_ID); | |
| if (inlinePrefix.isEmpty()) { | |
| // Try to detect prefix from any inline attachment field present in data | |
| contentId = | |
| resolveInlineOrDirectField( | |
| path.target().values(), path.target().entity(), Attachments.CONTENT_ID); | |
| } else { | |
| contentId = | |
| (String) path.target().values().get(inlinePrefix.get() + "_" + Attachments.CONTENT_ID); | |
| } | |
| if (contentId == null) { | |
| contentId = (String) path.target().values().get(Attachments.CONTENT_ID); | |
| } | |
| // Resolve contentId: try inline prefixed fields first, then fall back to direct field | |
| String contentId = | |
| resolveInlineOrDirectField( | |
| path.target().values(), path.target().entity(), Attachments.CONTENT_ID); | |
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
| public static List<String> getInlineAttachmentFieldNames(CdsStructuredType entity) { | ||
| List<String> fieldNames = new ArrayList<>(); | ||
| var elements = entity.elements(); | ||
| if (elements == null) return fieldNames; | ||
| String contentSuffix = "_content"; | ||
| elements | ||
| .filter(e -> e.getName().endsWith(contentSuffix)) | ||
| .filter(e -> e.getAnnotationValue(ANNOTATION_IS_MEDIA_DATA, false)) | ||
| .filter(e -> e.findAnnotation(ANNOTATION_CORE_MEDIA_TYPE).isPresent()) | ||
| .forEach( | ||
| e -> { | ||
| String prefix = | ||
| e.getName().substring(0, e.getName().length() - contentSuffix.length()); | ||
| if (!prefix.isEmpty() && !fieldNames.contains(prefix)) { | ||
| fieldNames.add(prefix); | ||
| } | ||
| }); | ||
| return fieldNames; |
There was a problem hiding this comment.
Performance: getInlineAttachmentFieldNames uses List.contains in its inner forEach, giving O(n²) behaviour when an entity has many inline attachment fields.
Although practically rare today, the list is built by iterating all elements and then checking !fieldNames.contains(prefix) for every element. Should use a LinkedHashSet internally (to preserve insertion order and deduplicate in O(1)) and convert to List before returning.
| public static List<String> getInlineAttachmentFieldNames(CdsStructuredType entity) { | |
| List<String> fieldNames = new ArrayList<>(); | |
| var elements = entity.elements(); | |
| if (elements == null) return fieldNames; | |
| String contentSuffix = "_content"; | |
| elements | |
| .filter(e -> e.getName().endsWith(contentSuffix)) | |
| .filter(e -> e.getAnnotationValue(ANNOTATION_IS_MEDIA_DATA, false)) | |
| .filter(e -> e.findAnnotation(ANNOTATION_CORE_MEDIA_TYPE).isPresent()) | |
| .forEach( | |
| e -> { | |
| String prefix = | |
| e.getName().substring(0, e.getName().length() - contentSuffix.length()); | |
| if (!prefix.isEmpty() && !fieldNames.contains(prefix)) { | |
| fieldNames.add(prefix); | |
| } | |
| }); | |
| return fieldNames; | |
| public static List<String> getInlineAttachmentFieldNames(CdsStructuredType entity) { | |
| var elements = entity.elements(); | |
| if (elements == null) return List.of(); | |
| String contentSuffix = "_content"; | |
| java.util.LinkedHashSet<String> fieldNames = new java.util.LinkedHashSet<>(); | |
| elements | |
| .filter(e -> e.getName().endsWith(contentSuffix)) | |
| .filter(e -> e.getAnnotationValue(ANNOTATION_IS_MEDIA_DATA, false)) | |
| .filter(e -> e.findAnnotation(ANNOTATION_CORE_MEDIA_TYPE).isPresent()) | |
| .forEach( | |
| e -> { | |
| String prefix = | |
| e.getName().substring(0, e.getName().length() - contentSuffix.length()); | |
| if (!prefix.isEmpty()) { | |
| fieldNames.add(prefix); | |
| } | |
| }); | |
| return new ArrayList<>(fieldNames); | |
| } | |
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
| // Also check for inline attachment type fields on the entity itself | ||
| if (ApplicationHandlerHelper.hasInlineAttachmentElements(entity)) { | ||
| var identifier = new AssociationIdentifier(associationName, entity.getQualifiedName()); | ||
| var inlinePath = new LinkedList<>(firstList); | ||
| inlinePath.addLast(identifier); | ||
| internalResultList.add(inlinePath); | ||
| } |
There was a problem hiding this comment.
Logic Error: The inline path added to internalResultList duplicates the identifier that was already added to firstList for the composition-based path.
When hasInlineAttachmentElements is true and there are also composition children, firstList may already contain parent identifiers accumulated from outer recursive calls. Cloning firstList into inlinePath and then adding the current identifier reuses that accumulated state correctly — but if the root entity itself has both inline attachments and composition children, the inline identifier gets added to internalResultList here (line 90), but the composition children also add their full paths in the loop below, potentially causing the root entity's inline path to appear multiple times in the final NodeTree if findEntityPath is called with the root entity on subsequent recursive calls.
This should be verified against the AssociationCascaderTest.rootEntityWithInlineAttachmentDoesNotAddExtraTreeChild test to ensure NodeTree.addPath deduplicates correctly; otherwise, readAttachments may issue redundant DB queries.
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
| if (!expandList.isEmpty() || !inlineColumns.isEmpty()) { | ||
| List<CqnSelectListItem> allItems = new ArrayList<>(inlineColumns); | ||
| allItems.addAll(expandList); | ||
| select = Select.from(statement.ref()).columns(allItems); |
There was a problem hiding this comment.
Bug: When an entity has both composition-based attachments (non-empty expandList) and inline attachment fields (non-empty inlineColumns), the key columns of the root entity are never added to the select. The query will only return inline columns and expand columns, omitting the root entity's primary keys. This means the returned Attachments records will lack key data needed for matching in getExistingAttachment.
| if (!expandList.isEmpty() || !inlineColumns.isEmpty()) { | |
| List<CqnSelectListItem> allItems = new ArrayList<>(inlineColumns); | |
| allItems.addAll(expandList); | |
| select = Select.from(statement.ref()).columns(allItems); | |
| Select<?> select; | |
| if (!expandList.isEmpty() || !inlineColumns.isEmpty()) { | |
| List<CqnSelectListItem> allItems = new ArrayList<>(); | |
| allItems.add(CQL.get("*")); // ensure root key columns are always included | |
| allItems.addAll(inlineColumns); | |
| allItems.addAll(expandList); | |
| select = Select.from(statement.ref()).columns(allItems); | |
| } else { | |
| select = Select.from(statement.ref()).columns(StructuredType::_all); | |
| } | |
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
| List<String> associationNames = new ArrayList<>(); | ||
| if (ApplicationHandlerHelper.isMediaEntity(entity)) { | ||
| if (ApplicationHandlerHelper.isDirectMediaEntity(entity)) { | ||
| associationNames.add(associationName); | ||
| } | ||
|
|
||
| // Also add inline attachment field names (for inline type support) | ||
| List<String> inlineFields = ApplicationHandlerHelper.getInlineAttachmentFieldNames(entity); | ||
| if (!inlineFields.isEmpty() && !associationNames.contains(associationName)) { | ||
| // Use empty string to signify inline fields on the root entity | ||
| associationNames.add(associationName); | ||
| } |
There was a problem hiding this comment.
Logic Error: getAttachmentAssociations now adds associationName (which is "" for the root entity) to associationNames for both direct media entities and entities with inline attachment fields. When the root entity has inline attachments but no @_is_media_data annotation, adding "" to associationNames causes BeforeReadItemsModifier to treat the root as a media association and attempt to add contentId/status/scannedAt fields unconditionally (via the composition-based path), which is incorrect for inline-only roots.
The two cases (direct media vs inline) should be tracked separately — the existing code at lines 155-164 conflates them. Only the inline case should be handled here; the inlinePrefixes list passed to BeforeReadItemsModifier (line 107) already handles inline field injection correctly.
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
1 participant
What
Adds support for inline (single) attachments using the
Attachmenttype directly on an entity field (e.g.profilePicture: Attachment), in addition to the existing composition-basedComposition of many Attachmentspattern.Key Changes
@_is_media_dataannotation at element levelcoverImage_contentId→contentId)How it works
When an entity has
field: Attachment, CDS flattens the Attachment fields into the parent table with a prefix (e.g.coverImage_content,coverImage_contentId). The handlers detect this via the@_is_media_dataannotation at element level, extract the prefixed data into normalizedAttachmentsmaps, and inject parent entity keys for downstream matching.