Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f58573
docs(README): add non-draft upload example using .http file
RoshniNaveenaS Jul 24, 2025
8ddd8ba
fix(aws-s3.js): improve error handling for non-draft and updateConten…
RoshniNaveenaS Jul 24, 2025
b3cbf1f
refactor(aws-s3): replace console.error with cds.log().error for cons…
RoshniNaveenaS Jul 24, 2025
c72fdb2
feat(basic.js): add robust error handling and non-blocking malware sc…
RoshniNaveenaS Jul 24, 2025
74f3465
fix(attachments): add error handling for PUT before handlers and nonD…
RoshniNaveenaS Jul 24, 2025
485c63f
Fix lint issues in aws-s3.js and improve error context propagation
RoshniNaveenaS Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions README.md
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

How the non-draft and draft scenarios be distinguishable from the usage perspective?

Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
49 changes: 33 additions & 16 deletions lib/aws-s3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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.');
}
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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),
Expand Down Expand Up @@ -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) {
Expand Down
143 changes: 96 additions & 47 deletions lib/basic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}
}
};
36 changes: 23 additions & 13 deletions lib/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 });
Copy link

Choose a reason for hiding this comment

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

Doesn't the line 117 need to be executed only after the attachmentID is obtained ? I mean would there be a situation when AttachmentsSrv.nonDraftHandler is called when url.match is asynchronously being executed ?

}
} catch (err) {
LOG.error('[NON_DRAFT_UPLOAD_ERROR]', err);
req.reject(500, 'Non-draft attachment upload failed.');
}
}
})

function validateAttachmentSize (req) {
Expand Down
Loading