From 3339c51a7dcc6beb32f17504615e9efd33f72c43 Mon Sep 17 00:00:00 2001 From: Ankush Kumar Garg Date: Wed, 5 Aug 2026 12:15:43 +0530 Subject: [PATCH 1/3] change srv.before to srv,on --- lib/sdm.js | 106 ++++++++++++++++++++++++++----------------- test/lib/sdm.test.js | 95 ++++++++++++++++++++++---------------- 2 files changed, 120 insertions(+), 81 deletions(-) diff --git a/lib/sdm.js b/lib/sdm.js index 6f1433d2..e97c7f34 100644 --- a/lib/sdm.js +++ b/lib/sdm.js @@ -572,9 +572,13 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; } } - async draftAttachmentUploadHandler(req) { + async draftAttachmentUploadHandler(req, next) { - if (req?.data?.content) { + // Not a content upload (e.g. metadata-only draft edit) -> pass control down the + // on-handler chain so the base attachments/default handler can run. + if (!req?.data?.content) return next(); + + { // Read actual file size from HTTP Content-Length header so the chunked // upload path can be selected before the stream is consumed. const rawContentLength = req.req?.headers?.['content-length'] || req.headers?.['content-length']; @@ -1676,9 +1680,10 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; * * @param {import('@sap/cds').Request} req - The request object */ - async nonDraftAttachmentCreateHandler(req) { - // Skip if no content or if this is a draft entity (handled by draftPutHandler) - if (!req.data.content || req.target.isDraft) return; + async nonDraftAttachmentCreateHandler(req, next) { + // Skip if no content or if this is a draft entity (handled by draftPutHandler). + // Chain to the next on-handler / default persistence. + if (!req.data.content || req.target.isDraft) return next(); LOG.info(`[INFO] [nonDraftAttachmentCreateHandler] event=${req.event} target=${req.target.name}`); const rawContentLength = req.req?.headers?.['content-length'] || req.headers?.['content-length']; @@ -1741,6 +1746,8 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; } req.data.content = null; + // Upload to DMS done. Chain to default handler to persist metadata to the DB. + return next(); } /** @@ -1748,20 +1755,20 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; * Called on PATCH/UPDATE of attachment entity directly (e.g., Projects.references) * @param {import('@sap/cds').Request} req - The request object */ - async nonDraftAttachmentUpdateHandler(req) { + async nonDraftAttachmentUpdateHandler(req, next) { // Skip if this is a draft entity if (req.target.isDraft) { - return; + return next(); } // Skip if this is a PUT /content operation (handled by nonDraftAttachmentCreateHandler) if (req.data.content) { - return; + return next(); } // Skip if filename is not being changed and no custom properties if (!('filename' in req.data) && Object.keys(req.data).length <= 1) { - return; + return next(); } LOG.info(`[INFO] [nonDraftAttachmentUpdateHandler] Updating attachment metadata target=${req.target.name} ID=${req.data.ID}`); @@ -1775,7 +1782,6 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; if (!currentAttachment) { return req.reject(404, 'Attachment not found'); } - // Merge request data with current attachment for validation const attachmentToUpdate = { ...currentAttachment, ...req.data }; @@ -1801,35 +1807,39 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; // Use the existing _updateAttachments method which handles all validation and SDM updates const failedReq = await this._updateAttachments(req, context); - // Handle errors using req.reject (direct operation pattern) + // Handle errors using req.reject (direct operation pattern). Explicit return after + // reject so the trailing next() is not reached on a rejected request. if (failedReq && failedReq.length > 0) { const error = failedReq[0]; - + if (error.typeOfError === 'restricted characters') { - req.reject(409, nameConstrainErr([error.name], "Update")); + return req.reject(409, nameConstrainErr([error.name], "Update")); } else if (error.typeOfError === 'empty name') { - req.reject(400, emptyFileNameErr); + return req.reject(400, emptyFileNameErr); } else if (error.typeOfError === 'duplicate') { - req.reject(409, duplicateFileErr([error.name])); + return req.reject(409, duplicateFileErr([error.name])); } else if (error.typeOfError === 'no sdm roles') { - req.reject(403, userNotAuthorisedError); + return req.reject(403, userNotAuthorisedError); } else if (error.typeOfError === 'not found') { - req.reject(404, renameFileErr([error.name], getStatusCondition(404))); + return req.reject(404, renameFileErr([error.name], getStatusCondition(404))); } else if (error.typeOfError === 'unsupported properties') { // Parse CMIS property IDs from error.details (comma-separated string) const cmisPropertyIds = error.details.split(',').map(name => name.trim()); - + // For unsupported properties, we warn but don't reject (matches draft behavior) // The properties couldn't be updated, but the operation should still succeed const warningMessage = unsupportedPropertiesErrorMessage(cmisPropertyIds); req.warn(warningMessage); - // Continue - don't reject, just warn + // Continue - don't reject, just warn (falls through to next() below) } else if (error.typeOfError === 'bad request') { - req.reject(500, error.message || renameOtherFilesErr([error.name], ['Update failed'])); + return req.reject(500, error.message || renameOtherFilesErr([error.name], ['Update failed'])); } else { - req.reject(500, 'Update failed'); + return req.reject(500, 'Update failed'); } } + + // Metadata/rename update in DMS done. Chain to default handler to persist to DB. + return next(); } /** @@ -2223,34 +2233,48 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; srv.before("READ", targets, this.setRepository.bind(this)); srv.before("READ", targets, this.filterAttachments.bind(this)); - // Handle PUT for draft attachments + // Handle PUT for draft attachments. + // Registered as an `on` handler (prepended so it runs before the base + // @cap-js/attachments on-handler) instead of `before`. CAP only enters the + // `on` phase if no req.errors occurred during `before`, so a customer's own + // validation before-handler that rejects now prevents this DMS upload from + // starting at all -> avoids orphaned DMS files on rollback. if (target.drafts) { - srv.before( - "PUT", - target.drafts, - this.draftAttachmentUploadHandler.bind(this) + srv.prepend(() => + srv.on( + "PUT", + target.drafts, + this.draftAttachmentUploadHandler.bind(this) + ) ); } else { - // Handle PUT for non-draft attachments (content upload) - srv.before( - "PUT", - target, - this.nonDraftAttachmentCreateHandler.bind(this) + // Handle PUT for non-draft attachments (content upload) as an `on` handler + // so a customer validation reject in `before` prevents the DMS upload. + srv.prepend(() => + srv.on( + "PUT", + target, + this.nonDraftAttachmentCreateHandler.bind(this) + ) ); } - // Handle CREATE for non-draft attachments - srv.before( - "CREATE", - target, - this.nonDraftAttachmentCreateHandler.bind(this) + // Handle CREATE for non-draft attachments (on-phase, same rationale) + srv.prepend(() => + srv.on( + "CREATE", + target, + this.nonDraftAttachmentCreateHandler.bind(this) + ) ); - // Handle direct UPDATE/PATCH on non-draft attachment entity - srv.before( - "UPDATE", - target, - this.nonDraftAttachmentUpdateHandler.bind(this) + // Handle direct UPDATE/PATCH on non-draft attachment entity (on-phase) + srv.prepend(() => + srv.on( + "UPDATE", + target, + this.nonDraftAttachmentUpdateHandler.bind(this) + ) ); srv.after( diff --git a/test/lib/sdm.test.js b/test/lib/sdm.test.js index 8da8aa1c..0600f1d1 100644 --- a/test/lib/sdm.test.js +++ b/test/lib/sdm.test.js @@ -1999,6 +1999,7 @@ describe("SDMAttachmentsService", () => { before: jest.fn(), after: jest.fn(), on: jest.fn(), + prepend: jest.fn((cb) => cb()), entities: {} }; }); @@ -2259,6 +2260,7 @@ describe("SDMAttachmentsService", () => { before: jest.fn(), after: jest.fn(), on: jest.fn(), + prepend: jest.fn((cb) => cb()), entities: { TestEntity: { elements: { @@ -2297,7 +2299,10 @@ describe("SDMAttachmentsService", () => { mockSrv = { before: jest.fn(), after: jest.fn(), - on: jest.fn() + on: jest.fn(), + // prepend runs its callback immediately so the inner srv.on(...) registrations + // (e.g. the draft upload handler) are recorded on the mocks below. + prepend: jest.fn((cb) => cb()) }; entity = { @@ -2345,19 +2350,19 @@ describe("SDMAttachmentsService", () => { entity, expect.any(Function) ); - expect(mockSrv.before).toHaveBeenCalledWith( - "PUT", - target.drafts, + expect(mockSrv.on).toHaveBeenCalledWith( + "PUT", + target.drafts, expect.any(Function) ); - expect(mockSrv.before).toHaveBeenCalledWith( - "CREATE", - target, + expect(mockSrv.on).toHaveBeenCalledWith( + "CREATE", + target, expect.any(Function) ); - expect(mockSrv.before).toHaveBeenCalledWith( - "UPDATE", - target, + expect(mockSrv.on).toHaveBeenCalledWith( + "UPDATE", + target, expect.any(Function) ); @@ -2383,8 +2388,8 @@ describe("SDMAttachmentsService", () => { const targetWithoutDrafts = {}; service.registerSDMHandlers(mockSrv, entity, targetWithoutDrafts); - // Verify PUT handler for non-draft target is called instead - const putCalls = mockSrv.before.mock.calls.filter(call => call[0] === 'PUT'); + // Verify PUT handler for non-draft target is registered as an `on` handler + const putCalls = mockSrv.on.mock.calls.filter(call => call[0] === 'PUT'); expect(putCalls.length).toBeGreaterThan(0); expect(putCalls[0][1]).toBe(targetWithoutDrafts); }); @@ -2448,13 +2453,18 @@ describe("SDMAttachmentsService", () => { it('should register all three custom action handlers', () => { service.registerSDMHandlers(mockSrv, entity, target); - + const actionNames = mockSrv.on.mock.calls.map(call => call[0]); - + expect(actionNames).toContain('openAttachment'); expect(actionNames).toContain('createLink'); expect(actionNames).toContain('editLink'); - expect(mockSrv.on).toHaveBeenCalledTimes(3); + // 3 custom action handlers + attachment handlers now registered as `on` (via + // prepend): draft PUT (target.drafts) + non-draft CREATE + non-draft UPDATE = 6 total. + expect(mockSrv.on).toHaveBeenCalledTimes(6); + expect(actionNames).toContain('PUT'); + expect(actionNames).toContain('CREATE'); + expect(actionNames).toContain('UPDATE'); }); it('should handle errors thrown by openAttachment method', async () => { @@ -2871,7 +2881,10 @@ describe("SDMAttachmentsService", () => { test('should skip when req.data.content is not provided', async () => { const req = { data: {} }; - await service.draftAttachmentUploadHandler(req); + const next = jest.fn(); + await service.draftAttachmentUploadHandler(req, next); + // No content -> hand control down the on-handler chain, do no SDM work. + expect(next).toHaveBeenCalled(); expect(service.checkRepositoryType).not.toHaveBeenCalled(); }); @@ -2974,9 +2987,11 @@ describe("SDMAttachmentsService", () => { getDraftAttachmentsForUpID.mockResolvedValue(attachment_val); req.data.content = null; // simulating content being reset to null after initial check - - await service.draftAttachmentUploadHandler(req); - + + const next = jest.fn(); + await service.draftAttachmentUploadHandler(req, next); + + expect(next).toHaveBeenCalled(); expect(service.isFileNameDuplicateInDrafts).not.toHaveBeenCalled(); expect(service.create).not.toHaveBeenCalled(); }); @@ -6387,7 +6402,7 @@ describe("SDMAttachmentsService", () => { event: 'CREATE' }; - await service.nonDraftAttachmentCreateHandler(mockReq); + await service.nonDraftAttachmentCreateHandler(mockReq, jest.fn()); expect(service.onCreate).not.toHaveBeenCalled(); }); @@ -6403,7 +6418,7 @@ describe("SDMAttachmentsService", () => { event: 'CREATE' }; - await service.nonDraftAttachmentCreateHandler(mockReq); + await service.nonDraftAttachmentCreateHandler(mockReq, jest.fn()); expect(service.onCreate).not.toHaveBeenCalled(); }); @@ -6434,7 +6449,7 @@ describe("SDMAttachmentsService", () => { }) }); - await service.nonDraftAttachmentCreateHandler(mockReq); + await service.nonDraftAttachmentCreateHandler(mockReq, jest.fn()); expect(service.onCreate).toHaveBeenCalledWith( expect.arrayContaining([ @@ -6491,7 +6506,7 @@ describe("SDMAttachmentsService", () => { }) }); - await service.nonDraftAttachmentCreateHandler(mockReq); + await service.nonDraftAttachmentCreateHandler(mockReq, jest.fn()); expect(service.onCreate).toHaveBeenCalledWith( expect.arrayContaining([ @@ -6569,7 +6584,7 @@ describe("SDMAttachmentsService", () => { where: jest.fn().mockResolvedValue(null) }); - await service.nonDraftAttachmentCreateHandler(mockReq); + await service.nonDraftAttachmentCreateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(404, 'Attachment not found'); expect(service.onCreate).not.toHaveBeenCalled(); @@ -6589,7 +6604,7 @@ describe("SDMAttachmentsService", () => { isRestrictedCharactersInName.mockReturnValue(true); - await service.nonDraftAttachmentCreateHandler(mockReq); + await service.nonDraftAttachmentCreateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(409, expect.stringContaining('invalid/file.pdf')); expect(service.onCreate).not.toHaveBeenCalled(); @@ -6609,7 +6624,7 @@ describe("SDMAttachmentsService", () => { isRestrictedCharactersInName.mockReturnValue(false); - await service.nonDraftAttachmentCreateHandler(mockReq); + await service.nonDraftAttachmentCreateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(400, expect.stringContaining('empty')); expect(service.onCreate).not.toHaveBeenCalled(); @@ -6635,7 +6650,7 @@ describe("SDMAttachmentsService", () => { target: { name: 'Orders.references.drafts', isDraft: true } }; - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(service._updateAttachments).not.toHaveBeenCalled(); }); @@ -6649,7 +6664,7 @@ describe("SDMAttachmentsService", () => { target: { name: 'Orders.references', isDraft: false } }; - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(service._updateAttachments).not.toHaveBeenCalled(); }); @@ -6660,7 +6675,7 @@ describe("SDMAttachmentsService", () => { target: { name: 'Orders.references', isDraft: false } }; - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(service._updateAttachments).not.toHaveBeenCalled(); }); @@ -6685,7 +6700,7 @@ describe("SDMAttachmentsService", () => { where: jest.fn().mockResolvedValue(mockCurrentAttachment) }); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(service._updateAttachments).toHaveBeenCalledWith( mockReq, @@ -6715,7 +6730,7 @@ describe("SDMAttachmentsService", () => { where: jest.fn().mockResolvedValue(null) }); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(404, 'Attachment not found'); }); @@ -6736,7 +6751,7 @@ describe("SDMAttachmentsService", () => { typeOfError: 'restricted characters' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(409, expect.stringContaining('invalid/name.pdf')); }); @@ -6757,7 +6772,7 @@ describe("SDMAttachmentsService", () => { typeOfError: 'empty name' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(400, expect.stringContaining('empty')); }); @@ -6778,7 +6793,7 @@ describe("SDMAttachmentsService", () => { typeOfError: 'duplicate' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(409, expect.stringContaining('duplicate.pdf')); }); @@ -6799,7 +6814,7 @@ describe("SDMAttachmentsService", () => { typeOfError: 'no sdm roles' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(403, expect.stringContaining('permissions')); }); @@ -6826,7 +6841,7 @@ describe("SDMAttachmentsService", () => { details: 'cmis:prop1,cmis:prop2' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.warn).toHaveBeenCalled(); expect(mockReq.reject).not.toHaveBeenCalled(); @@ -6848,7 +6863,7 @@ describe("SDMAttachmentsService", () => { typeOfError: 'not found' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(404, expect.stringContaining('notfound.pdf')); }); @@ -6871,7 +6886,7 @@ describe("SDMAttachmentsService", () => { message: customErrorMessage }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(500, customErrorMessage); }); @@ -6892,7 +6907,7 @@ describe("SDMAttachmentsService", () => { typeOfError: 'bad request' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(500, expect.stringContaining('invalid.pdf')); expect(mockReq.reject).toHaveBeenCalledWith(500, expect.stringContaining('Update failed')); @@ -6914,7 +6929,7 @@ describe("SDMAttachmentsService", () => { typeOfError: 'unknown error type' }]); - await service.nonDraftAttachmentUpdateHandler(mockReq); + await service.nonDraftAttachmentUpdateHandler(mockReq, jest.fn()); expect(mockReq.reject).toHaveBeenCalledWith(500, 'Update failed'); }); From 03624f63a13838c81a9b8b76bdb59d2a980e61bc Mon Sep 17 00:00:00 2001 From: Ankush Kumar Garg Date: Thu, 6 Aug 2026 10:30:25 +0530 Subject: [PATCH 2/3] change more handlers to on --- lib/sdm.js | 36 +++++++++----- test/lib/sdm.test.js | 109 ++++++++++++++++++++++++------------------- 2 files changed, 86 insertions(+), 59 deletions(-) diff --git a/lib/sdm.js b/lib/sdm.js index e97c7f34..70470808 100644 --- a/lib/sdm.js +++ b/lib/sdm.js @@ -217,7 +217,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; return attachmentCompositions; } - async draftEntityRenameHandler(req) { + async draftEntityRenameHandler(req, next) { const { repositoryId } = getConfigurations(); const attachmentCompositions = this.getAttachmentCompositions(req.target); LOG.debug(`[DEBUG] [draftEntityRenameHandler] entity=${req.target.name} compositions=${attachmentCompositions.length}`); @@ -225,6 +225,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; for (const composition of attachmentCompositions) { await this.processCompositionRename(req, composition, repositoryId); } + return next(); } async processCompositionRename(req, compositionName, repositoryId) { @@ -1033,7 +1034,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; req.parentId.push(folderId); } - async attachURLsToDeleteFromAttachmentsDraft(req) { + async attachURLsToDeleteFromAttachmentsDraft(req, next) { let draftAttachments = cds.model.definitions[req.target.name]; if(draftAttachments) { const attachmentsToDeleteFromDraft = await getURLToDeleteFromDraftAttachments(req.data.ID, draftAttachments); @@ -1045,6 +1046,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; await this.deleteAttachmentsWithKeys(req.attachmentsToDelete, req); } } + return next(); } async deleteAttachmentsWithKeys(records, req) { @@ -1578,12 +1580,12 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; } } - async handleDraftDiscardForLinks(req) { + async handleDraftDiscardForLinks(req, next) { let parentId = req.data.ID; const baseEntityName = req.target.name.replace(/\.drafts$/, ""); const baseEntity = cds.model.definitions[baseEntityName]; if (!baseEntity) { - return; + return next(); } LOG.debug(`[DEBUG] [handleDraftDiscardForLinks] parentId=${parentId} entity=${baseEntityName}`); const attachmentCompositions = this.getAttachmentCompositions({ name: baseEntityName }); @@ -1618,6 +1620,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; } } } + return next(); } async revertLinkInSDM(draftAttachment, originalLinkUrl, req, attachmentsEntity) { @@ -1851,20 +1854,21 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; * * @param {import('@sap/cds').Request} req - The request object */ - async nonDraftEntityRenameHandler(req) { + async nonDraftEntityRenameHandler(req, next) { const { repositoryId } = getConfigurations(); const attachmentsEntity = cds.model.definitions[req.target.name + ".attachments"]; - - if (!attachmentsEntity) return; + + if (!attachmentsEntity) return next(); LOG.debug(`[DEBUG] [nonDraftEntityRenameHandler] entity=${req.target.name} repositoryId=${repositoryId}`); const updatedAttachments = await this._getUpdatedAttachments(req); - if (!updatedAttachments || updatedAttachments.length === 0) return; + if (!updatedAttachments || updatedAttachments.length === 0) return next(); const validationContext = this._prepareValidationContext(attachmentsEntity, updatedAttachments[0]); const allErrors = await this._processAttachmentUpdates(req, updatedAttachments, attachmentsEntity, validationContext); this._handleUpdateResults(req, repositoryId, allErrors, validationContext.propertyTitles); + return next(); } /** @@ -2192,17 +2196,23 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; // Draft-specific handlers if (entity.drafts) { - srv.before("DELETE", entity.drafts, this.handleDraftDiscardForLinks.bind(this)); + srv.prepend(() => + srv.on("DELETE", entity.drafts, this.handleDraftDiscardForLinks.bind(this)) + ); // Snapshot existing attachment IDs BEFORE activation so the after-SAVE // stamp can target only freshly activated rows. Registered before the // rename handler so req._sdmSaveSnapshot is populated for everything // downstream that runs in the SAVE flow. srv.before("SAVE", entity, this.captureSaveSnapshot.bind(this)); srv.after("SAVE", entity, this.handleDraftSaveForLinks.bind(this)); - srv.before("SAVE", entity, this.draftEntityRenameHandler.bind(this)); + srv.prepend(() => + srv.on("SAVE", entity, this.draftEntityRenameHandler.bind(this)) + ); } else { // Non-draft rename/update handler - srv.before("UPDATE", entity, this.nonDraftEntityRenameHandler.bind(this)); + srv.prepend(() => + srv.on("UPDATE", entity, this.nonDraftEntityRenameHandler.bind(this)) + ); } srv.after( @@ -2218,7 +2228,9 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; // Handle DELETE on attachment entity (draft and non-draft) if (target.drafts) { - srv.before(["DELETE"], target.drafts, this.attachURLsToDeleteFromAttachmentsDraft.bind(this)); + srv.prepend(() => + srv.on(["DELETE"], target.drafts, this.attachURLsToDeleteFromAttachmentsDraft.bind(this)) + ); } // Handle direct DELETE on attachment entity diff --git a/test/lib/sdm.test.js b/test/lib/sdm.test.js index 0600f1d1..e340f4ff 100644 --- a/test/lib/sdm.test.js +++ b/test/lib/sdm.test.js @@ -922,7 +922,7 @@ describe("SDMAttachmentsService", () => { setupDestinationMocks(); getDraftAttachments.mockResolvedValue([]); - await service.draftEntityRenameHandler(req); + await service.draftEntityRenameHandler(req, jest.fn()); expect(service.isFileNameDuplicateInDrafts).not.toHaveBeenCalled(); expect(getDraftAttachments).toHaveBeenCalledWith(cds.model.definitions['sampleTarget.references'], req, 'repo123'); @@ -954,7 +954,7 @@ describe("SDMAttachmentsService", () => { getSecondaryPropertiesWithInvalidDefinition.mockReturnValue({ invalidProperty: "value" }); getSecondaryTypeProperties.mockReturnValue(new Map([["property1", "value1"], ["property2", "value2"]])); - await service.draftEntityRenameHandler(req); + await service.draftEntityRenameHandler(req, jest.fn()); expect(service.isFileNameDuplicateInDrafts).toHaveBeenCalledWith(allAttachments, req); expect(service.updateDraftAttachments).toHaveBeenCalledTimes(2); @@ -987,7 +987,7 @@ describe("SDMAttachmentsService", () => { setupDestinationMocks(); getDraftAttachments.mockResolvedValue(allAttachments); - await service.draftEntityRenameHandler(req); + await service.draftEntityRenameHandler(req, jest.fn()); expect(service.isFileNameDuplicateInDrafts).toHaveBeenCalledWith(allAttachments, req); expect(service.updateDraftAttachments).toHaveBeenCalledTimes(1); @@ -1024,7 +1024,7 @@ describe("SDMAttachmentsService", () => { getSecondaryPropertiesWithInvalidDefinition.mockReturnValue({}); getSecondaryTypeProperties.mockReturnValue(new Map()); - await expect(service.draftEntityRenameHandler(req)).rejects.toThrow('Draft update failed'); + await expect(service.draftEntityRenameHandler(req, jest.fn())).rejects.toThrow('Draft update failed'); expect(service.isFileNameDuplicateInDrafts).toHaveBeenCalledWith(allAttachments, req); expect(service.updateNonDraftAttachments).not.toHaveBeenCalled(); @@ -1055,7 +1055,7 @@ describe("SDMAttachmentsService", () => { getSecondaryPropertiesWithInvalidDefinition.mockReturnValue({ invalidProperty: "value" }); getSecondaryTypeProperties.mockReturnValue(new Map([["property1", "value1"], ["property2", "value2"]])); - await service.draftEntityRenameHandler(req); + await service.draftEntityRenameHandler(req, jest.fn()); expect(getDraftAttachments).toHaveBeenCalledWith(referencesEntity, req, 'repo123'); expect(service.isFileNameDuplicateInDrafts).toHaveBeenCalledWith(allReferences, req); @@ -1106,7 +1106,7 @@ describe("SDMAttachmentsService", () => { getSecondaryPropertiesWithInvalidDefinition.mockReturnValue({}); getSecondaryTypeProperties.mockReturnValue(new Map()); - await service.draftEntityRenameHandler(req); + await service.draftEntityRenameHandler(req, jest.fn()); // Should be called 3 times, once for each composition expect(getDraftAttachments).toHaveBeenCalledTimes(3); @@ -2330,24 +2330,36 @@ describe("SDMAttachmentsService", () => { entity.drafts, expect.any(Function) ); - expect(mockSrv.before).toHaveBeenCalledWith( - ["DELETE"], - target.drafts, + expect(mockSrv.on).toHaveBeenCalledWith( + ["DELETE"], + target.drafts, expect.any(Function) ); expect(mockSrv.before).toHaveBeenCalledWith( - "DELETE", - target, + "DELETE", + target, expect.any(Function) ); expect(mockSrv.before).toHaveBeenCalledWith( - "READ", - [target, target.drafts], + "READ", + [target, target.drafts], expect.any(Function) ); expect(mockSrv.before).toHaveBeenCalledWith( - "SAVE", - entity, + "SAVE", + entity, + expect.any(Function) + ); + // draftEntityRenameHandler (SAVE) and handleDraftDiscardForLinks (DELETE on + // entity.drafts) are now `on` handlers. + expect(mockSrv.on).toHaveBeenCalledWith( + "SAVE", + entity, + expect.any(Function) + ); + expect(mockSrv.on).toHaveBeenCalledWith( + "DELETE", + entity.drafts, expect.any(Function) ); expect(mockSrv.on).toHaveBeenCalledWith( @@ -2459,12 +2471,15 @@ describe("SDMAttachmentsService", () => { expect(actionNames).toContain('openAttachment'); expect(actionNames).toContain('createLink'); expect(actionNames).toContain('editLink'); - // 3 custom action handlers + attachment handlers now registered as `on` (via - // prepend): draft PUT (target.drafts) + non-draft CREATE + non-draft UPDATE = 6 total. - expect(mockSrv.on).toHaveBeenCalledTimes(6); + // 3 custom action handlers + attachment handlers now registered as `on` (via prepend): + // draft PUT + non-draft CREATE + non-draft UPDATE + SAVE (draft rename) + + // DELETE (draft discard, entity.drafts) + ["DELETE"] (draft attachment) = 9 total. + expect(mockSrv.on).toHaveBeenCalledTimes(9); expect(actionNames).toContain('PUT'); expect(actionNames).toContain('CREATE'); expect(actionNames).toContain('UPDATE'); + expect(actionNames).toContain('SAVE'); + expect(actionNames).toContain('DELETE'); }); it('should handle errors thrown by openAttachment method', async () => { @@ -3476,7 +3491,7 @@ describe("SDMAttachmentsService", () => { const deleteAttachmentsSpy = jest.spyOn(service, 'deleteAttachmentsWithKeys'); // Call the method - await service.attachURLsToDeleteFromAttachmentsDraft(req); + await service.attachURLsToDeleteFromAttachmentsDraft(req, jest.fn()); expect(req.attachmentsToDelete).toEqual([{ url: 'http://example.com/attachment1', ID: '1' }]); @@ -3499,7 +3514,7 @@ describe("SDMAttachmentsService", () => { }; const deleteAttachmentsSpy = jest.spyOn(service, 'deleteAttachmentsWithKeys'); - await service.attachURLsToDeleteFromAttachmentsDraft(req); + await service.attachURLsToDeleteFromAttachmentsDraft(req, jest.fn()); expect(req.attachmentsToDelete).toBeUndefined(); expect(deleteAttachmentsSpy).not.toHaveBeenCalled(); @@ -4643,7 +4658,7 @@ describe("SDMAttachmentsService", () => { global.SELECT.where.mockResolvedValue(draftAttachments); - await service.handleDraftDiscardForLinks(req); + await service.handleDraftDiscardForLinks(req, jest.fn()); expect(service.revertLinkInSDM).toHaveBeenCalledWith( draftAttachments[0], @@ -4665,7 +4680,7 @@ describe("SDMAttachmentsService", () => { global.SELECT.where.mockResolvedValue(draftAttachments); - await service.handleDraftDiscardForLinks(req); + await service.handleDraftDiscardForLinks(req, jest.fn()); expect(service.revertLinkInSDM).not.toHaveBeenCalled(); }); @@ -4681,7 +4696,7 @@ describe("SDMAttachmentsService", () => { global.SELECT.where.mockResolvedValue(draftAttachments); - await service.handleDraftDiscardForLinks(req); + await service.handleDraftDiscardForLinks(req, jest.fn()); expect(service.revertLinkInSDM).not.toHaveBeenCalled(); }); @@ -4704,7 +4719,7 @@ describe("SDMAttachmentsService", () => { global.SELECT.where.mockResolvedValue([]); // Should execute without crashing - await service.handleDraftDiscardForLinks(req); + await service.handleDraftDiscardForLinks(req, jest.fn()); expect(service.revertLinkInSDM).not.toHaveBeenCalled(); }); @@ -6030,7 +6045,7 @@ describe("SDMAttachmentsService", () => { setupDestinationMocks(); - await service.handleDraftDiscardForLinks(req); + await service.handleDraftDiscardForLinks(req, jest.fn()); // Should not throw }); @@ -6057,7 +6072,7 @@ describe("SDMAttachmentsService", () => { // Ensure entity doesn't exist delete cds.model.definitions['NonExistent.Entity']; - await service.handleDraftDiscardForLinks(req); + await service.handleDraftDiscardForLinks(req, jest.fn()); // Should not throw and should not call any SDM operations }); @@ -6559,7 +6574,7 @@ describe("SDMAttachmentsService", () => { isRestrictedCharactersInName.mockReturnValue(false); // Should not throw — should fall back to req.data.ID when req.req is absent - await expect(service.nonDraftAttachmentCreateHandler(mockReq)).resolves.not.toThrow(); + await expect(service.nonDraftAttachmentCreateHandler(mockReq, jest.fn())).resolves.not.toThrow(); expect(service.onCreate).toHaveBeenCalledWith( expect.arrayContaining([ @@ -6956,7 +6971,7 @@ describe("SDMAttachmentsService", () => { // Ensure no attachments composition exists cds.model.definitions['Orders.attachments'] = undefined; - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(mockReq.diff).not.toHaveBeenCalled(); }); @@ -6972,7 +6987,7 @@ describe("SDMAttachmentsService", () => { includes: ['sap.attachments.Attachments'] }; - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(service._updateAttachments).not.toHaveBeenCalled(); }); @@ -6993,7 +7008,7 @@ describe("SDMAttachmentsService", () => { includes: ['sap.attachments.Attachments'] }; - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(service._updateAttachments).not.toHaveBeenCalled(); }); @@ -7035,7 +7050,7 @@ describe("SDMAttachmentsService", () => { setupDestinationMocks(); updateAttachment.mockResolvedValue(200); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(updateAttachment).toHaveBeenCalled(); expect(mockReq.reject).not.toHaveBeenCalled(); @@ -7073,7 +7088,7 @@ describe("SDMAttachmentsService", () => { isRestrictedCharactersInName.mockReturnValue(true); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(mockReq.warn).toHaveBeenCalled(); expect(mockReq.reject).not.toHaveBeenCalled(); @@ -7111,7 +7126,7 @@ describe("SDMAttachmentsService", () => { isRestrictedCharactersInName.mockReturnValue(false); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); // Empty filename falls back to original filename, so no update needed expect(mockReq.warn).not.toHaveBeenCalled(); @@ -7156,7 +7171,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Access denied'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'no sdm roles', name: 'renamed.pdf' }], @@ -7203,7 +7218,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Duplicate file'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'duplicate', name: 'duplicate.pdf' }], @@ -7250,7 +7265,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('File not found'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'not found', name: 'notfound.pdf' }], @@ -7300,7 +7315,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Unsupported properties warning'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'unsupported properties', details: 'customProp is not supported' }], @@ -7349,7 +7364,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Bad request error'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'bad request', name: 'file.pdf', message: 'Network error occurred' }], @@ -7413,7 +7428,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Multiple errors'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [ @@ -7464,7 +7479,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue(''); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith([], {}); expect(mockReq.warn).not.toHaveBeenCalled(); @@ -7505,7 +7520,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Empty filename error'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'empty name', name: null }], @@ -7549,7 +7564,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Empty filename error'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'empty name', name: ' ' }], @@ -7593,7 +7608,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Empty filename error'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [{ typeOfError: 'empty name', name: '\t\n ' }], @@ -7631,7 +7646,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue(''); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); // Should not process this attachment expect(getPropertiesForID).not.toHaveBeenCalled(); @@ -7669,7 +7684,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue(''); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); // Should not process this attachment expect(getPropertiesForID).not.toHaveBeenCalled(); @@ -7725,7 +7740,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue(''); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); // Should only process the valid attachment expect(updateAttachment).toHaveBeenCalledTimes(1); @@ -7789,7 +7804,7 @@ describe("SDMAttachmentsService", () => { const handleWarningSpy = jest.spyOn(service, 'handleWarning').mockReturnValue('Multiple validation errors'); - await service.nonDraftEntityRenameHandler(mockReq); + await service.nonDraftEntityRenameHandler(mockReq, jest.fn()); expect(handleWarningSpy).toHaveBeenCalledWith( [ From 28ed7e91fb6ae2c15f8a44cfe2d9ee4acceebec3 Mon Sep 17 00:00:00 2001 From: Ankush Kumar Garg Date: Tue, 11 Aug 2026 16:27:22 +0530 Subject: [PATCH 3/3] flag based approach --- lib/sdm.js | 114 +++++++++++++++++++------------------------ test/lib/sdm.test.js | 11 ++++- 2 files changed, 60 insertions(+), 65 deletions(-) diff --git a/lib/sdm.js b/lib/sdm.js index 70470808..a23caa7e 100644 --- a/lib/sdm.js +++ b/lib/sdm.js @@ -225,7 +225,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; for (const composition of attachmentCompositions) { await this.processCompositionRename(req, composition, repositoryId); } - return next(); + return next?.(); } async processCompositionRename(req, compositionName, repositoryId) { @@ -577,7 +577,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; // Not a content upload (e.g. metadata-only draft edit) -> pass control down the // on-handler chain so the base attachments/default handler can run. - if (!req?.data?.content) return next(); + if (!req?.data?.content) return next?.(); { // Read actual file size from HTTP Content-Length header so the chunked @@ -1046,7 +1046,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; await this.deleteAttachmentsWithKeys(req.attachmentsToDelete, req); } } - return next(); + return next?.(); } async deleteAttachmentsWithKeys(records, req) { @@ -1585,7 +1585,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; const baseEntityName = req.target.name.replace(/\.drafts$/, ""); const baseEntity = cds.model.definitions[baseEntityName]; if (!baseEntity) { - return next(); + return next?.(); } LOG.debug(`[DEBUG] [handleDraftDiscardForLinks] parentId=${parentId} entity=${baseEntityName}`); const attachmentCompositions = this.getAttachmentCompositions({ name: baseEntityName }); @@ -1620,7 +1620,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; } } } - return next(); + return next?.(); } async revertLinkInSDM(draftAttachment, originalLinkUrl, req, attachmentsEntity) { @@ -1686,7 +1686,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; async nonDraftAttachmentCreateHandler(req, next) { // Skip if no content or if this is a draft entity (handled by draftPutHandler). // Chain to the next on-handler / default persistence. - if (!req.data.content || req.target.isDraft) return next(); + if (!req.data.content || req.target.isDraft) return next?.(); LOG.info(`[INFO] [nonDraftAttachmentCreateHandler] event=${req.event} target=${req.target.name}`); const rawContentLength = req.req?.headers?.['content-length'] || req.headers?.['content-length']; @@ -1750,7 +1750,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; req.data.content = null; // Upload to DMS done. Chain to default handler to persist metadata to the DB. - return next(); + return next?.(); } /** @@ -1761,17 +1761,17 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; async nonDraftAttachmentUpdateHandler(req, next) { // Skip if this is a draft entity if (req.target.isDraft) { - return next(); + return next?.(); } // Skip if this is a PUT /content operation (handled by nonDraftAttachmentCreateHandler) if (req.data.content) { - return next(); + return next?.(); } // Skip if filename is not being changed and no custom properties if (!('filename' in req.data) && Object.keys(req.data).length <= 1) { - return next(); + return next?.(); } LOG.info(`[INFO] [nonDraftAttachmentUpdateHandler] Updating attachment metadata target=${req.target.name} ID=${req.data.ID}`); @@ -1842,7 +1842,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; } // Metadata/rename update in DMS done. Chain to default handler to persist to DB. - return next(); + return next?.(); } /** @@ -1858,17 +1858,17 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; const { repositoryId } = getConfigurations(); const attachmentsEntity = cds.model.definitions[req.target.name + ".attachments"]; - if (!attachmentsEntity) return next(); + if (!attachmentsEntity) return next?.(); LOG.debug(`[DEBUG] [nonDraftEntityRenameHandler] entity=${req.target.name} repositoryId=${repositoryId}`); const updatedAttachments = await this._getUpdatedAttachments(req); - if (!updatedAttachments || updatedAttachments.length === 0) return next(); + if (!updatedAttachments || updatedAttachments.length === 0) return next?.(); const validationContext = this._prepareValidationContext(attachmentsEntity, updatedAttachments[0]); const allErrors = await this._processAttachmentUpdates(req, updatedAttachments, attachmentsEntity, validationContext); this._handleUpdateResults(req, repositoryId, allErrors, validationContext.propertyTitles); - return next(); + return next?.(); } /** @@ -2154,6 +2154,27 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; registerSDMHandlers(srv, entity, target) { + // Feature flag: when settings.uploadInOnPhase (or env SDM_UPLOAD_IN_ON_PHASE) is + // true, the DMS-mutating handlers are registered in the `on` phase (prepended) so a + // customer validation reject in `before` prevents the DMS operation -> no orphaned + // files. Default is false, which keeps the original `before`-phase behavior for full + // backward compatibility. Opt-in only. + const useOnPhase = + process.env.SDM_UPLOAD_IN_ON_PHASE === "true" || + cds.env?.requires?.["sdm"]?.settings?.uploadInOnPhase === true; + LOG.info(`[INFO] [registerSDMHandlers] DMS handler phase = ${useOnPhase ? "on (opt-in)" : "before (default)"}`); + + // Helper: register a DMS-mutating handler in `on` (prepended) when the flag is set, + // otherwise in `before`. The handlers accept (req, next) and use next?.() so they run + // correctly in either phase. + const registerDmsHandler = (events, ent, handler) => { + if (useOnPhase) { + srv.prepend(() => srv.on(events, ent, handler)); + } else { + srv.before(events, ent, handler); + } + }; + // When @SDM.useClientCredential is set, override createdBy/modifiedBy with // the SDM technical user clientid so the plugin DB matches DMS/DI. Run // these first so they win against later managed-aspect defaults. @@ -2196,23 +2217,17 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; // Draft-specific handlers if (entity.drafts) { - srv.prepend(() => - srv.on("DELETE", entity.drafts, this.handleDraftDiscardForLinks.bind(this)) - ); + registerDmsHandler("DELETE", entity.drafts, this.handleDraftDiscardForLinks.bind(this)); // Snapshot existing attachment IDs BEFORE activation so the after-SAVE // stamp can target only freshly activated rows. Registered before the // rename handler so req._sdmSaveSnapshot is populated for everything // downstream that runs in the SAVE flow. srv.before("SAVE", entity, this.captureSaveSnapshot.bind(this)); srv.after("SAVE", entity, this.handleDraftSaveForLinks.bind(this)); - srv.prepend(() => - srv.on("SAVE", entity, this.draftEntityRenameHandler.bind(this)) - ); + registerDmsHandler("SAVE", entity, this.draftEntityRenameHandler.bind(this)); } else { // Non-draft rename/update handler - srv.prepend(() => - srv.on("UPDATE", entity, this.nonDraftEntityRenameHandler.bind(this)) - ); + registerDmsHandler("UPDATE", entity, this.nonDraftEntityRenameHandler.bind(this)); } srv.after( @@ -2228,9 +2243,7 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; // Handle DELETE on attachment entity (draft and non-draft) if (target.drafts) { - srv.prepend(() => - srv.on(["DELETE"], target.drafts, this.attachURLsToDeleteFromAttachmentsDraft.bind(this)) - ); + registerDmsHandler(["DELETE"], target.drafts, this.attachURLsToDeleteFromAttachmentsDraft.bind(this)); } // Handle direct DELETE on attachment entity @@ -2245,49 +2258,22 @@ let subdomain = cds.context?.user?.authInfo?.token?.payload?.ext_attr?.zdn; srv.before("READ", targets, this.setRepository.bind(this)); srv.before("READ", targets, this.filterAttachments.bind(this)); - // Handle PUT for draft attachments. - // Registered as an `on` handler (prepended so it runs before the base - // @cap-js/attachments on-handler) instead of `before`. CAP only enters the - // `on` phase if no req.errors occurred during `before`, so a customer's own - // validation before-handler that rejects now prevents this DMS upload from - // starting at all -> avoids orphaned DMS files on rollback. + // Handle PUT for draft/non-draft attachment content upload. + // When the opt-in flag is set, these run in the `on` phase (prepended, so they + // run before the base @cap-js/attachments on-handler); CAP only enters `on` if no + // req.errors occurred in `before`, so a customer validation reject prevents the DMS + // upload -> no orphaned files. Default (flag off) keeps the original `before` phase. if (target.drafts) { - srv.prepend(() => - srv.on( - "PUT", - target.drafts, - this.draftAttachmentUploadHandler.bind(this) - ) - ); + registerDmsHandler("PUT", target.drafts, this.draftAttachmentUploadHandler.bind(this)); } else { - // Handle PUT for non-draft attachments (content upload) as an `on` handler - // so a customer validation reject in `before` prevents the DMS upload. - srv.prepend(() => - srv.on( - "PUT", - target, - this.nonDraftAttachmentCreateHandler.bind(this) - ) - ); + registerDmsHandler("PUT", target, this.nonDraftAttachmentCreateHandler.bind(this)); } - // Handle CREATE for non-draft attachments (on-phase, same rationale) - srv.prepend(() => - srv.on( - "CREATE", - target, - this.nonDraftAttachmentCreateHandler.bind(this) - ) - ); + // Handle CREATE for non-draft attachments (same phase rationale) + registerDmsHandler("CREATE", target, this.nonDraftAttachmentCreateHandler.bind(this)); - // Handle direct UPDATE/PATCH on non-draft attachment entity (on-phase) - srv.prepend(() => - srv.on( - "UPDATE", - target, - this.nonDraftAttachmentUpdateHandler.bind(this) - ) - ); + // Handle direct UPDATE/PATCH on non-draft attachment entity (same phase rationale) + registerDmsHandler("UPDATE", target, this.nonDraftAttachmentUpdateHandler.bind(this)); srv.after( "DELETE", diff --git a/test/lib/sdm.test.js b/test/lib/sdm.test.js index e340f4ff..e92fd16d 100644 --- a/test/lib/sdm.test.js +++ b/test/lib/sdm.test.js @@ -2288,14 +2288,19 @@ describe("SDMAttachmentsService", () => { let mockSrv; let entity; let target; + let origEnvReg; beforeEach(() => { jest.clearAllMocks(); + // Enable the opt-in flag so DMS handlers register in the `on` phase (the path + // these assertions verify). Default (flag off) registers them as `before`. + origEnvReg = cds.env; + cds.env = { ...cds.env, requires: { ...(cds.env?.requires), sdm: { settings: { uploadInOnPhase: true } } } }; service = new SDMAttachmentsService(); service._registeredEntityHandlers = new Set(); service._registeredTargetHandlers = new Set(); service._registeredGlobalActionHandlers = false; - + mockSrv = { before: jest.fn(), after: jest.fn(), @@ -2316,6 +2321,10 @@ describe("SDMAttachmentsService", () => { }; }); + afterEach(() => { + cds.env = origEnvReg; + }); + it('should register all handlers correctly', () => { service.registerSDMHandlers(mockSrv, entity, target);