feat: update workflow instances based on instance id #108
Conversation
…nstances-based-on-instance-id-not-business-key
…nstances-based-on-instance-id-not-business-key
![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 updateInstanceStatus across all service layers (BTP, local, process service handler) and adds corresponding CDS type/function definitions and tests. The most critical issue is the misuse of function instead of action for a state-mutating operation, which appears consistently in both srv/BTPProcessService.cds and lib/processImport/csnBuilder.ts (and consequently in all generated .cds fixture files). Additionally, the local service logs unvalidated input before performing validation, and the test suite has reliability gaps with unguarded array accesses and fail()-based assertions that can silently pass.
PR Bot Information
Version: 1.20.47
- File Content Strategy: Full file content
- Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
8da34664-1273-4437-86f2-e4f4ff1ee8b3
| this.on('updateInstanceStatus', async (req: cds.Request) => { | ||
| const { instanceId, status } = req.data; | ||
| LOG.info('Updating instance status', instanceId, '->', status); | ||
|
|
||
| LOG.debug( | ||
| `==============================================================\n` + | ||
| `Update instance status for ${instanceId} to ${status}\n` + | ||
| `==============================================================`, | ||
| ); |
There was a problem hiding this comment.
Logic Error: Validation occurs after data is logged, allowing unsanitized/invalid data to be logged before input checks
LOG.info and LOG.debug at lines 218–224 log instanceId and status before the null-checks at lines 226–238 are performed. If instanceId or status are missing or contain malicious content, they will be emitted to logs before being rejected.
Consider moving the validation block above the log statements.
| this.on('updateInstanceStatus', async (req: cds.Request) => { | |
| const { instanceId, status } = req.data; | |
| LOG.info('Updating instance status', instanceId, '->', status); | |
| LOG.debug( | |
| `==============================================================\n` + | |
| `Update instance status for ${instanceId} to ${status}\n` + | |
| `==============================================================`, | |
| ); | |
| this.on('updateInstanceStatus', async (req: cds.Request) => { | |
| const { instanceId, status } = req.data; | |
| if (!instanceId) { | |
| return req.reject({ status: 400, message: 'Missing required parameter: instanceId' }); | |
| } | |
| if (!status) { | |
| return req.reject({ status: 400, message: 'Missing required parameter: status' }); | |
| } | |
| const validStatuses = Object.values(WorkflowStatus); | |
| if (!validStatuses.includes(status as WorkflowStatus)) { | |
| return req.reject({ | |
| status: 400, | |
| message: `Invalid status: ${status}. Valid values are: ${validStatuses.join(', ')}`, | |
| }); | |
| } | |
| LOG.info('Updating instance status', instanceId, '->', status); | |
| LOG.debug( | |
| `==============================================================\n` + | |
| `Update instance status for ${instanceId} to ${status}\n` + | |
| `==============================================================`, | |
| ); |
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
| it('should return 404 for a non-existent instance ID', async () => { | ||
| try { | ||
| await updateInstanceStatus('non-existent-id', 'SUSPENDED'); | ||
| fail('Expected request to be rejected'); | ||
| } catch (error: any) { | ||
| expect(error.response.status).toBe(404); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Logic Error: Test for 404 does not start a process first, but updateInstanceStatus routes through genericUpdateInstanceStatus → processService.send('updateInstanceStatus', ...) → the local service. Since the local service uses the in-memory localWorkflowStore, any prior test run that left state could affect this test; conversely, the store is never reset between tests. However, the larger issue is that fail(...) at line 288 (and line 278) is not reliable in all Jest configurations — if the POST unexpectedly resolves instead of throwing, fail may itself throw an unrecognised error rather than failing the assertion.
Should use Jest's expect.assertions(1) at the start of the test block to guarantee the catch branch is reached, rather than relying on a bare fail() call.
| it('should return 404 for a non-existent instance ID', async () => { | |
| try { | |
| await updateInstanceStatus('non-existent-id', 'SUSPENDED'); | |
| fail('Expected request to be rejected'); | |
| } catch (error: any) { | |
| expect(error.response.status).toBe(404); | |
| } | |
| }); | |
| it('should return 404 for a non-existent instance ID', async () => { | |
| expect.assertions(1); | |
| try { | |
| await updateInstanceStatus('non-existent-id', 'SUSPENDED'); | |
| } catch (error: any) { | |
| expect(error.response.status).toBe(404); | |
| } | |
| }); |
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
| try { | ||
| await updateInstanceStatus(instanceId, 'INVALID'); | ||
| fail('Expected request to be rejected'); | ||
| } catch (error: any) { | ||
| expect(error.response.status).toBe(400); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Logic Error: Same fail(...) reliability issue — if the POST call does not throw, the test silently passes without any assertion being evaluated.
Should use expect.assertions(1) to ensure the catch block is actually executed.
| try { | |
| await updateInstanceStatus(instanceId, 'INVALID'); | |
| fail('Expected request to be rejected'); | |
| } catch (error: any) { | |
| expect(error.response.status).toBe(400); | |
| } | |
| }); | |
| expect.assertions(1); | |
| try { | |
| await updateInstanceStatus(instanceId, 'INVALID'); | |
| } catch (error: any) { | |
| expect(error.response.status).toBe(400); | |
| } |
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
| async function getInstanceId(businessKey: string): Promise<string> { | ||
| const res = await POST('/odata/v4/programmatic/genericGetInstancesByBusinessKey', { | ||
| businessKey, | ||
| }); | ||
| return res.data.value[0].id; |
There was a problem hiding this comment.
Bug: getInstanceId accesses res.data.value[0].id without checking whether the array is non-empty. If the instance hasn't been flushed/stored yet (e.g., timing issues), res.data.value[0] will be undefined, causing a runtime error that masks the actual test failure.
Should add a guard or assert that the array is non-empty before accessing the first element.
| async function getInstanceId(businessKey: string): Promise<string> { | |
| const res = await POST('/odata/v4/programmatic/genericGetInstancesByBusinessKey', { | |
| businessKey, | |
| }); | |
| return res.data.value[0].id; | |
| async function getInstanceId(businessKey: string): Promise<string> { | |
| const res = await POST('/odata/v4/programmatic/genericGetInstancesByBusinessKey', { | |
| businessKey, | |
| }); | |
| expect(res.data.value.length).toBeGreaterThan(0); | |
| return res.data.value[0].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
| definitions[fqn(serviceName, 'updateInstanceStatus')] = { | ||
| kind: 'function', | ||
| name: fqn(serviceName, 'updateInstanceStatus'), | ||
| params: { | ||
| instanceId: { type: csn.CdsBuiltinType.String, notNull: true }, | ||
| status: { type: csn.CdsBuiltinType.String, notNull: true }, | ||
| cascade: { type: csn.CdsBuiltinType.Boolean }, | ||
| }, | ||
| returns: { type: updateStatusResultType }, | ||
| }; |
There was a problem hiding this comment.
Best Practices: The updateInstanceStatus operation modifies server-side state (it changes the status of a workflow instance) but is declared as a CDS function. In CDS/OData semantics, function maps to an HTTP GET (safe, read-only), while action maps to an HTTP POST (non-safe, state-changing). Declaring a state-mutating operation as a function violates OData protocol semantics and may cause caching or proxy issues.
Should use kind: 'action' instead of kind: 'function'.
| definitions[fqn(serviceName, 'updateInstanceStatus')] = { | |
| kind: 'function', | |
| name: fqn(serviceName, 'updateInstanceStatus'), | |
| params: { | |
| instanceId: { type: csn.CdsBuiltinType.String, notNull: true }, | |
| status: { type: csn.CdsBuiltinType.String, notNull: true }, | |
| cascade: { type: csn.CdsBuiltinType.Boolean }, | |
| }, | |
| returns: { type: updateStatusResultType }, | |
| }; | |
| definitions[fqn(serviceName, 'updateInstanceStatus')] = { | |
| kind: 'action', | |
| name: fqn(serviceName, 'updateInstanceStatus'), | |
| params: { | |
| instanceId: { type: csn.CdsBuiltinType.String, notNull: true }, | |
| status: { type: csn.CdsBuiltinType.String, notNull: true }, | |
| cascade: { type: csn.CdsBuiltinType.Boolean }, | |
| }, | |
| returns: { type: updateStatusResultType }, | |
| }; |
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
| function updateInstanceStatus( | ||
| @mandatory instanceId : String(256), | ||
| @mandatory status : String(256), | ||
| cascade : Boolean | ||
| ) returns UpdateStatusResult; |
There was a problem hiding this comment.
Best Practices: updateInstanceStatus modifies workflow state (it is not idempotent and not safe) but is declared as a CDS function. OData maps function to HTTP GET (safe/read-only) and action to HTTP POST (state-changing). Using function here is semantically incorrect and can cause incorrect caching behaviour.
Should be declared as action instead of function.
| function updateInstanceStatus( | |
| @mandatory instanceId : String(256), | |
| @mandatory status : String(256), | |
| cascade : Boolean | |
| ) returns UpdateStatusResult; | |
| action updateInstanceStatus( | |
| @mandatory instanceId : String(256), | |
| @mandatory status : String(256), | |
| cascade : Boolean | |
| ) returns UpdateStatusResult; |
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
1 participant
Add
updateInstanceStatusFunction to Workflow Process ServicesNew Feature
✨ Introduces an
updateInstanceStatusfunction that allows callers to update the status of a workflow instance directly by its instance ID. The feature supports an optionalcascadeparameter and validates the provided status against knownWorkflowStatusvalues, returning a structuredUpdateStatusResult({ id, success }).Changes
lib/handlers/processService.ts: Registered the newupdateInstanceStatusHandlerhandler, which validatesinstanceIdandstatusparameters, checks against validWorkflowStatusvalues, and delegates toPROCESS_SERVICE.lib/processImport/csnBuilder.ts: AddedUpdateStatusResulttype definition (withidandsuccessfields) and theupdateInstanceStatusfunction signature to the generated CSN definitions for every imported process service.srv/BTPProcessService.cds: Added theUpdateStatusResulttype andupdateInstanceStatusfunction declaration to the BTP process service definition.srv/BTPProcessService.ts: Implemented theupdateInstanceStatushandler in the BTP process service, delegating toworkflowInstanceClient.updateWorkflowStatus.srv/localProcessService.ts: Implemented theupdateInstanceStatushandler in the local/mock process service, updating the local workflow store and returning appropriate 404 or success results.tests/bookshop/srv/programmatic-service.cds: AddedUpdateStatusResulttype,genericUpdateInstanceStatus, andupdateInstanceStatusViaProcesstest actions to the bookshop programmatic service.tests/bookshop/srv/programmatic-service.ts: Wired upgenericUpdateInstanceStatusandupdateInstanceStatusViaProcesshandlers for integration testing.tests/bookshop/srv/external/*.cds&tests/hybrid/importedCDS/*.cds: Updated all generated external CDS files with the newUpdateStatusResulttype andupdateInstanceStatusfunction, along with refreshed checksums.tests/bookshop/srv/workflows/*.json: Updated test workflow JSON fixtures to align with the latest schema (field reordering, removed redundanttitlefields in type definitions).tests/integration/genericProgrammaticApproach.test.ts: Added integration tests covering success, status reflection, invalid status (400), and non-existent instance (404) scenarios forupdateInstanceStatus.tests/integration/programmaticApproach.test.ts: Added an integration test verifyingupdateInstanceStatusinvoked via an imported process service returns{ id, success: true }.PR Bot Information
Version:
1.20.47pull_request.opened8da34664-1273-4437-86f2-e4f4ff1ee8b3anthropic--claude-4.6-sonnet