feat: replace getinstancesbybusinesskey with a general getinstances method#106
Conversation
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR is a solid refactor replacing the narrow getInstancesByBusinessKey with a general-purpose getInstances method. The changes are broad and consistent across most files, but there are a few correctness issues: the local store's rootInstanceId/parentInstanceId filters incorrectly match against i.id, the API URL is built with an unconditional trailing ? when no params are provided, status values are not URL-encoded unlike all other query params, and the sample service CDS files are incomplete (missing new ProcessInstance fields and most of the new getInstances parameters) compared to all other updated generated files in the PR.
PR Bot Information
Version: 1.20.43
- Event Trigger:
pull_request.opened - Correlation ID:
4d21e4c9-9854-4a43-b66b-679149569720 - LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
| for (const s of params.status ?? []) { | ||
| queryParts.push(`status=${s}`); |
There was a problem hiding this comment.
Bug: The status values are appended to the query string without encodeURIComponent
All other scalar params are encoded via encodeURIComponent(String(value)), but the status loop appends raw enum values directly. While current WorkflowStatus values are simple ASCII strings, keeping encoding consistent prevents breakage if status values ever change and avoids the inconsistency.
Consider:
for (const s of params.status ?? []) {
queryParts.push(`status=${encodeURIComponent(s)}`);
}| for (const s of params.status ?? []) { | |
| queryParts.push(`status=${s}`); | |
| for (const s of params.status ?? []) { | |
| queryParts.push(`status=${encodeURIComponent(s)}`); | |
| } |
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 getInstances( | ||
| businessKey : String, | ||
| status : many String, | ||
| definitionId : String, | ||
| definitionVersion : String, | ||
| startedAt : String, | ||
| completedAt : String, | ||
| startedBy : String, | ||
| subject : String | ||
| ) returns ProcessInstances; |
There was a problem hiding this comment.
Inconsistency: The getInstances function signature in the sample .cds files is missing many parameters present in the main BTPProcessService.cds and the generated hybrid test .cds files
The hybrid-test generated CDS files expose the full parameter set (including id, startedFrom, startedUpTo, completedFrom, completedUpTo, containsText, rootInstanceId, parentInstanceId, top, skip, orderBy, inlinecount), but the sample service CDS files (authorVerificationProcess.cds, bookApprovalProcess.cds) only expose a partial subset (8 params). This means callers using the sample service cannot pass the new filtering, pagination, or ordering params. If these files are regenerated by the import process, they should be consistent; if maintained manually, they need to be brought in line with the full parameter set.
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
| orderBy?: string | null; | ||
| top?: number | null; | ||
| skip?: number | null; | ||
| inlinecount?: string | null; |
There was a problem hiding this comment.
I am not sure if we should 1:1 mirror the wfApi here, I think it makes it more complicated to include those orderBy / top / skip / inlinecount
There was a problem hiding this comment.
I mean we try to mirror the sbpa workflow api as much as possible, if you think they are to noise I can remove orderBy and inlinecount since they are sbpa specific
There was a problem hiding this comment.
I would remove them at least as they, from my POV dont bring any benefit
| if (status) { | ||
| const params = req.data as GetInstancesParams; | ||
|
|
||
| if (params.status) { |
There was a problem hiding this comment.
we validate status here, but what about other params? Are they not validated?
There was a problem hiding this comment.
Because it is the only thing we know upfront the rest are strings / numbers / dates which we can not validate other then for the type
There was a problem hiding this comment.
do those strings have any character limited defined in sbpa?
…ith-a-general-getinstances-method
3 participants
Replace
getInstancesByBusinessKeywith a GeneralgetInstancesMethodNew Feature / Refactor
♻️ Replaced the narrowly scoped
getInstancesByBusinessKeymethod (which requiredbusinessKeyas a mandatory parameter and only supported filtering by status) with a more flexible, general-purposegetInstancesmethod. The new method accepts a rich set of optional filter parameters, enabling querying by instance ID, definition, timestamps, text search, pagination, sorting, and more — aligning with the full capabilities of the underlying workflow API.Changes
lib/api/workflow-client.ts: IntroducedGetInstancesParamsinterface with ~17 optional filter fields, addedINSTANCES_PARAMS_SKIP_KEYSandINSTANCES_PARAM_KEY_MAPconstants for API query building, added thegetInstancesfunction, and extendedIWorkflowInstanceClient. Also enriched theWorkflowInstancetype withdefinitionVersion,startedAt,completedAt,startedBy, andsubjectfields.lib/api/local-workflow-store.ts: ImplementedgetInstances(params)on the local store supporting all filter params (date ranges, text search, pagination, etc.), replacinggetInstancesByBusinessKey.lib/api/index.ts: Exported new types and constants (GetInstancesParams,INSTANCES_PARAMS_SKIP_KEYS,INSTANCES_PARAM_KEY_MAP,getInstances).lib/handlers/processService.ts: Renamed handler fromgetInstancesByBusinessKeytogetInstances, switched toGetInstancesParams, removed the mandatorybusinessKeyvalidation.lib/processImport/csnBuilder.ts: UpdatedgetInstancesCSN definition with all new optional params; addedid,subject,businessKey,completedAttoProcessInstancetype; fixedstartedAttype toTimestamp.srv/BTPProcessService.cds: ReplacedgetInstancesByBusinessKeyfunction signature with the newgetInstancesfunction accepting all optional filter params.srv/BTPProcessService.ts/srv/localProcessService.ts: Updated event handlers to usegetInstanceswithGetInstancesParams.tests/bookshop/srv/annotation-hybrid-service.cds/.ts: Updated action definitions and handler registrations fromgetInstancesByBusinessKeytogetInstances.tests/bookshop/srv/programmatic-service.cds/.ts: Renamed actions and handlers accordingly.tests/bookshop/srv/external/*.cds/tests/hybrid/importedCDS/*.cds: Regenerated CDS files reflecting the newgetInstancessignature, updatedProcessInstancetype withid,completedAt(Timestamp),subject,businessKey.tests/sample/status-management/srv/authors-service.js/books-service.js: Updated calls fromgetInstancesByBusinessKey(id, statuses)togetInstances({ businessKey, status }).tests/hybrid/annotationApproach.test.ts/programmaticApproach.test.ts: Updated test endpoint paths to usegetInstances.tests/bookshop/srv/workflows/*.json: Updated test workflow fixture files with minor JSON schema cleanup.PR Bot Information
Version:
1.20.43pull_request.openedanthropic--claude-4.6-sonnet4d21e4c9-9854-4a43-b66b-679149569720