Fix(attachments): Improve Error Handling in Upload Lifecycle and Non-draft Upload Flow

README.md

@@ -130,6 +130,17 @@ entity Incidents {
 ```
 In this example, the `@attachments.disable_facet` is set to `true`, which means the plugin will be hidden by default.
 
+## Non-Draft Upload Example
+
+For scenarios where the entity is not draft-enabled, see [`tests/non-draft-request.http`](./tests/non-draft-request.http) for sample `.http` requests to perform metadata creation and content upload.
+
+The typical sequence includes:
+
+1. **POST** to create attachment metadata  
+2. **PUT** to upload file content using the ID returned
+
+> This is useful for non-draft-enabled entity sets. Make sure to replace `{{host}}`, `{{auth}}`, and IDs accordingly.
+
 ## Multitenancy
 
 The plugin supports multitenancy scenarios, allowing both shared and tenant-specific object store instances.

lib/aws-s3.js

@@ -71,7 +71,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") {
       DEBUG?.(`Created S3 client for tenant ${tenantID}`);
     } catch (error) {
       // eslint-disable-next-line no-console
-      console.error(`Creation of S3 client for tenant ${tenantID} failed`, error);
+      cds.log('attachments').error(`Creation of S3 client for tenant ${tenantID} failed`, error);
     }
   }
 
@@ -99,12 +99,19 @@ module.exports = class AWSAttachmentsService extends require("./basic") {
         client: this.client,
         params: input,
       });
-
-      const stored = super.put(attachments, metadata, null, isDraftEnabled);
-      await Promise.all([stored, multipartUpload.done()]);
-      if (this.kind === 's3') scanRequest(attachments, { ID: metadata.ID }, req)
+
+      await super.put(attachments, metadata, null, isDraftEnabled);
+      await multipartUpload.done();
+
+      if (this.kind === 's3') {
+        // Call scanRequest but catch errors to prevent upload failure
+        scanRequest(attachments, { ID: metadata.ID }, req).catch(err => {
+          cds.log('attachments').error('[SCAN][Error]', err);
+        });
+      }
     } catch (err) {
-      console.error(err); // eslint-disable-line no-console
+      cds.log('attachments').error('[PUT][UploadError]', err);
+      req?.error?.(500, 'Attachment upload failed.');
     }
   }
 
@@ -163,12 +170,13 @@ module.exports = class AWSAttachmentsService extends require("./basic") {
   }
 
   async updateContentHandler(req, next) {
+  try {
     // Check separate object store instances
     if (separateObjectStore) {
       const tenantID = req.tenant;
       await this.createClientS3(tenantID);
     }
-
+    
     if (req?.data?.content) {
       const response = await SELECT.from(req.target, { ID: req.data.ID }).columns("url");
       if (response?.url) {
@@ -182,20 +190,24 @@ module.exports = class AWSAttachmentsService extends require("./basic") {
           client: this.client,
           params: input,
         });
-        // const stored = super.put (Attachments, metadata)
-        await Promise.all([multipartUpload.done()]);
-
-        const keys = { ID: req.data.ID }
-        scanRequest(req.target, keys, req)
+        await multipartUpload.done();
+        const keys = { ID: req.data.ID };
+        // Call scanRequest async, handle errors to avoid unhandled rejections
+        scanRequest(req.target, keys, req).catch(err => {
+          cds.log('attachments').error('[SCAN][Error]', err);
+        });
       }
     } else if (req?.data?.note) {
       const key = { ID: req.data.ID };
       await super.update(req.target, key, { note: req.data.note });
     } else {
-      next();
+      return next();
     }
+  } catch (err) {
+    cds.log('attachments').error('[UPDATE_CONTENT_HANDLER][Error]', err);
+    req?.error?.(500, 'Failed to update attachment content.');
   }
-
+}
   async getAttachmentsToDelete({ draftEntity, activeEntity, id }) {
     const [draftAttachments, activeAttachments] = await Promise.all([
       SELECT.from(draftEntity).columns("url").where(id),
@@ -279,11 +291,16 @@ module.exports = class AWSAttachmentsService extends require("./basic") {
     });
   }
 
-  async nonDraftHandler(attachments, data) {
+  async nonDraftHandler(attachments, data, req) {
     const isDraftEnabled = false;
+    try {
     const response = await SELECT.from(attachments, { ID: data.ID }).columns("url");
     if (response?.url) data.url = response.url;
-    return this.put(attachments, [data], isDraftEnabled);
+    return await this.put(attachments, [data], isDraftEnabled, data.content, req);
+  } catch (error) {
+    cds.log('attachments').error('[NonDraftHandlerError]', error);
+    req?.error?.(500, 'Failed to process non-draft attachment upload.');
+  }
   }
 
   async delete(Key, req) {

lib/basic.js

@@ -5,69 +5,100 @@ const { scanRequest } = require('./malwareScanner')
 
 module.exports = class AttachmentsService extends cds.Service {
 
-  async put(attachments, data, _content, isDraftEnabled=true) {
+  async put(attachments, data, _content, isDraftEnabled = true) {
     if (!Array.isArray(data)) {
       if (_content) data.content = _content;
       data = [data];
     }
-    DEBUG?.(
-      "Uploading attachments for",
-      attachments.name,
-      data.map?.((d) => d.filename)
-    );
-
-    let res;
-    if (isDraftEnabled) {
-      res = await Promise.all(
-        data.map(async (d) => {
-          return await UPSERT(d).into(attachments);
-        })
-      );
-    }
 
-    if(this.kind === 'db') data.map((d) => { scanRequest(attachments, { ID: d.ID })})
+    DEBUG?.("Uploading attachments for", attachments.name, data.map?.(d => d.filename));
+
+    try {
+      let res;
+      if (isDraftEnabled) {
+        res = await Promise.all(data.map(async (d) => {
+          try {
+            return await UPSERT(d).into(attachments);
+          } catch (err) {
+            cds.log('attachments').error('[PUT][UpsertError]', err);
+            throw err;
+          }
+        }));
+      }
+
+      if (this.kind === 'db') {
+        for (const d of data) {
+          try {
+            scanRequest(attachments, { ID: d.ID });
+          } catch (err) {
+            cds.log('attachments').error('[PUT][ScanRequestError]', err);
+          }
+        }
+      }
 
-    return res;
+      return res;
+    } catch (err) {
+      cds.log('attachments').error('[PUT][UploadError]', err);
+      throw err;
+    }
   }
 
   // eslint-disable-next-line no-unused-vars
   async get(attachments, keys, req = {}) {
-    if (attachments.isDraft) {
-      attachments = attachments.actives;
-    }
+    if (attachments.isDraft) attachments = attachments.actives;
     DEBUG?.("Downloading attachment for", attachments.name, keys);
-    const result = await SELECT.from(attachments, keys).columns("content");
-    return (result?.content)? result.content : null;
+    try {
+      const result = await SELECT.from(attachments, keys).columns("content");
+      return result?.content || null;
+    } catch (err) {
+      cds.log('attachments').error('[GET][DownloadError]', err);
+      throw err;
+    }
   }
 
   /**
    * Returns a handler to copy updated attachments content from draft to active / object store
    */
   draftSaveHandler(attachments) {
     const queryFields = this.getFields(attachments);
-
-
+
     return async (_, req) => {
-      // The below query loads the attachments into streams
-      const cqn = SELECT(queryFields)
-        .from(attachments.drafts)
-        .where([
-          ...req.subject.ref[0].where.map((x) =>
-            x.ref ? { ref: ["up_", ...x.ref] } : x
-          )
-          // NOTE: needs skip LargeBinary fix to Lean Draft
-        ]);
-        cqn.where({content: {'!=': null }})
-        const draftAttachments = await cqn
-
-      if (draftAttachments.length)
-        await this.put(attachments, draftAttachments);
+      try {
+        // Build WHERE clause based on primary key mappings (e.g., up_)
+        const baseWhere = req.subject.ref[0].where.map((x) =>
+          x.ref ? { ref: ["up_", ...x.ref] } : x
+        );
+
+        // Construct SELECT CQN to fetch draft attachments with non-null content
+        const cqn = SELECT(queryFields)
+          .from(attachments.drafts)
+          .where(baseWhere);
+
+        // Add filter to exclude drafts with empty or null content
+        cqn.where({ content: { '!=': null } });
+
+        const draftAttachments = await cqn;
+
+        // Upload fetched attachments (if any)
+        if (draftAttachments.length) {
+          await this.put(attachments, draftAttachments);
+        }
+      } catch (err) {
+        const logger = cds.log('attachments');
+        logger.error('[DRAFT_SAVE_HANDLER]', err);
+        req?.error?.(500, 'Failed to process draft attachments.');
+      }
     };
   }
 
   async nonDraftHandler(attachments, data) {
     const isDraftEnabled = false;
-    return this.put(attachments, [data], null, isDraftEnabled);
+    try {
+      return await this.put(attachments, [data], null, isDraftEnabled);
+    } catch (err) {
+      cds.log('attachments').error('[NON_DRAFT][UploadError]', err);
+      throw err;
+    }
   }
 
   getFields(attachments) {
@@ -82,21 +113,39 @@ module.exports = class AttachmentsService extends cds.Service {
   }
 
   async registerUpdateHandlers(srv, entity, target) {
-    srv.after("SAVE", entity, this.draftSaveHandler(target));
-    return;
+    try {
+      srv.after("SAVE", entity, this.draftSaveHandler(target));
+    } catch (err) {
+      cds.log('attachments').error('[REGISTER_UPDATE_HANDLERS][Error]', err);
+    }
   }
 
   async update(Attachments, key, data) {
-    DEBUG?.("Updating attachment for", Attachments.name, key)
-    return await UPDATE(Attachments, key).with(data)
+    DEBUG?.("Updating attachment for", Attachments.name, key);
+    try {
+      return await UPDATE(Attachments, key).with(data);
+    } catch (err) {
+      cds.log('attachments').error('[UPDATE][Error]', err);
+      throw err;
+    }
   }
 
   async getStatus(Attachments, key) {
-    const result = await SELECT.from(Attachments, key).columns('status')
-    return result?.status;
+    try {
+      const result = await SELECT.from(Attachments, key).columns('status');
+      return result?.status;
+    } catch (err) {
+      cds.log('attachments').error('[GET_STATUS][Error]', err);
+      throw err;
+    }
   }
-  
+
   async deleteInfectedAttachment(Attachments, key) {
-    return await UPDATE(Attachments, key).with({ content: null})
+    try {
+      return await UPDATE(Attachments, key).with({ content: null });
+    } catch (err) {
+      cds.log('attachments').error('[DELETE_INFECTED][Error]', err);
+      throw err;
+    }
   }
 };

lib/plugin.js

@@ -56,15 +56,20 @@ cds.once("served", async function registerPluginHandlers () {
 
           const op = isDraft ? "NEW" : "CREATE";
           srv.before(op, putTarget, (req) => {
-            req.data.url = cds.utils.uuid()
-            const isMultitenacyEnabled = !!cds.env.requires.multitenancy;
-            const objectStoreKind = cds.env.requires?.attachments?.objectStore?.kind;
-            if (isMultitenacyEnabled && objectStoreKind === "shared") {
-              req.data.url = `${req.tenant}_${req.data.url}`;
+            try {
+              req.data.url = cds.utils.uuid()
+              const isMultitenacyEnabled = !!cds.env.requires.multitenancy;
+              const objectStoreKind = cds.env.requires?.attachments?.objectStore?.kind;
+              if (isMultitenacyEnabled && objectStoreKind === "shared") {
+                req.data.url = `${req.tenant}_${req.data.url}`;
+              }
+              req.data.ID = cds.utils.uuid()
+              let ext = extname(req.data.filename).toLowerCase().slice(1)
+              req.data.mimeType = Ext2MimeTyes[ext] || "application/octet-stream"
+            } catch (err) {
+              LOG.error('[PUT_BEFORE_ERROR]', err);
+              req.reject(500, 'Attachment initialization failed.')
             }
-            req.data.ID = cds.utils.uuid()
-            let ext = extname(req.data.filename).toLowerCase().slice(1)
-            req.data.mimeType = Ext2MimeTyes[ext] || "application/octet-stream"
           });
 
           if (isDraft) {
@@ -106,11 +111,16 @@ cds.once("served", async function registerPluginHandlers () {
   }
 
   async function nonDraftUpload(req, target) {
-		if (req?.content?.url?.endsWith("/content")) {
-			const attachmentID = req.content.url.match(attachmentIDRegex)[1];
-			AttachmentsSrv.nonDraftHandler(target, { ID: attachmentID, content: req.content });
-		}
-	}
+  try {
+    if (req?.content?.url?.endsWith("/content")) {
+      const attachmentID = req.content.url.match(attachmentIDRegex)[1];
+      await AttachmentsSrv.nonDraftHandler(target, { ID: attachmentID, content: req.content });
+    }
+  } catch (err) {
+    LOG.error('[NON_DRAFT_UPLOAD_ERROR]', err);
+    req.reject(500, 'Non-draft attachment upload failed.');
+  }
+}
 })
 
 function validateAttachmentSize (req) {