Skip to content

Single Attachment Bug Fixes#472

Open
eric-pSAP wants to merge 15 commits into
mainfrom
singleAttBugFixes
Open

Single Attachment Bug Fixes#472
eric-pSAP wants to merge 15 commits into
mainfrom
singleAttBugFixes

Conversation

@eric-pSAP

Copy link
Copy Markdown
Contributor

Addressing issues #470 and #471

@hyperspace-pr-bot hyperspace-pr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR introduces two substantive issues: the reordering of onPrepareAttachment before the MIME-type validation in plugin.js is the more critical one — it breaks MIME validation for stream PUT requests because mimeType hasn't been derived from the filename yet at the time validateAttachmentMimeType runs. Additionally, the new filename-fetching block in finalizePrepareAttachment duplicates logic that already exists in resolveFilename, creating a maintenance risk.

PR Bot Information

Version: 1.26.11

  • Correlation ID: 1b371dd1-c957-41f8-a37d-487d99c81d8e
  • Event Trigger: pull_request.opened
  • LLM: anthropic--claude-4.6-sonnet
  • File Content Strategy: Full file content

Comment thread lib/plugin.js
Comment thread lib/generic-handlers.js
@eric-pSAP eric-pSAP marked this pull request as ready for review July 3, 2026 07:38
@eric-pSAP eric-pSAP requested a review from a team as a code owner July 3, 2026 07:38
@hyperspace-pr-bot

Copy link
Copy Markdown
Contributor

Summary

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


Fix Single Attachment Bugs: UI Facet Injection and MediaType Validation Crash

Bug Fix

🐛 Resolves two bugs affecting inline (Attachment) single-attachment usage in CAP applications:

  1. UI facet not injected when only sap.attachments.Attachment (inline) is used and no Attachments composition exists anywhere in the app.
  2. Upload crash with @Core.AcceptableMediaTypes annotation on inline attachments — a TypeError: Cannot read properties of undefined (reading 'split') occurred because the filename was not yet resolved when MIME type validation ran.

Changes

  • lib/plugin.js:

    • Fixed the unfoldModel early-return guard to also check for sap.attachments.Attachment (not only sap.attachments.Attachments), so UI facets are correctly injected when only inline attachments are used.
    • Moved the onPrepareAttachment(req) call to run before validateAttachmentMimeType, and placed it only in the stream PUT path (removed duplicate call). This ensures the filename is resolved before MIME type validation is attempted.
  • lib/generic-handlers.js: Added logic to resolve the filename from the DB record during a stream PUT when attachmentData.filename is missing, preventing a undefined dereference in checkMimeTypeMatch.

  • tests/integration/attachments.test.js: Added integration tests covering:

    • Upload accepted when file type matches @Core.AcceptableMediaTypes on an inline attachment.
    • Upload rejected (HTTP 400) when file type does not match.
    • unfoldModel correctly adds @UI.FieldGroup and @UI.Facet when only sap.attachments.Attachment is defined.
    • No spurious facets added when the entity has no inline attachment fields.
  • CHANGELOG.md: Added release notes for version 3.13.2.

GitHub Issues

  • #470: No UI enhancement for inline attachments without attachment compositions
  • #471: Upload fails for inline attachments with @Core.AcceptableMediaTypes annotation

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

Version: 1.26.14

@hyperspace-pr-bot hyperspace-pr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR contains a real ordering bug: moving onPrepareAttachment before the MIME-type and size validations was meant to populate mimeType for inline stream PUTs, but validateAttachmentMimeType finds the inline prefix by searching req.data[${p}_content], which is always falsy when req.data is the raw binary body — causing the guard if (!validateAttachmentMimeType(req)) return to abort the upload entirely, a regression for all inline attachment stream PUTs. The other changes (the unfoldModel guard fix and the filename DB-fallback lookup) look correct.

PR Bot Information

Version: 1.26.14

  • Correlation ID: 24bae380-76b2-11f1-9af1-dc60f6d0660c
  • File Content Strategy: Full file content
  • LLM: anthropic--claude-4.6-sonnet
  • Event Trigger: pull_request.ready_for_review

Comment thread lib/plugin.js
Comment on lines +605 to 606
await onPrepareAttachment(req)
if (!validateAttachmentMimeType(req)) return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Logic Error: For stream PUT requests (when req.subject.ref.length > 1), req.data is the raw binary body, not an object with ${prefix}_content keys. validateAttachmentMimeType finds the inline prefix by checking req.data[${p}_content] — which is always falsy in this path — so it returns false, and the if (!validateAttachmentMimeType(req)) return guard then aborts the upload prematurely (a regression).

The original ordering called onPrepareAttachment after both validations, so this wasn't a problem before. In the stream PUT path, either the validation needs to be skipped when no content-bearing req.data key is present, or mimeType needs to be populated differently before validation runs.


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

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.

1 participant