UN-2190 [FEAT] Auto-capture execution ID in the API deployment Postman collection#2031
UN-2190 [FEAT] Auto-capture execution ID in the API deployment Postman collection#2031athul-rs wants to merge 3 commits into
Conversation
Add a post-response script to the 'Process document' request that stores message.execution_id into a collection variable, and point the 'Execution status' request's execution_id query param at that variable. Users no longer copy-paste execution IDs between requests (mirrors the LLMWhisperer collection's whisper_hash pattern). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdds constants and DTOs for Postman script events and collection variables, extends collection/item models, seeds collection variables via APIBase, and wires a test event on the execute request to capture execution_id for use in status requests. ChangesPostman Execution ID Variable Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| backend/api_v2/postman_collection/dto.py | Core change: adds new dataclasses, attaches a try/catch-guarded Postman test script to the execute item, seeds a collection variable with a fallback default, and strips null event blocks in to_dict(). Logic is sound after previous-round fixes. |
| backend/api_v2/serializers.py | Adds execution_id = CharField() to APIExecutionResponseSerializer; additive and non-breaking since ExecutionResponse.execution_id is always a non-null UUID string. |
| backend/api_v2/postman_collection/constants.py | Adds two constants (EXEC_ID_VARIABLE_NAME and STATUS_EXEC_ID_VARIABLE) used to avoid magic strings; straightforward and correct. |
Sequence Diagram
sequenceDiagram
participant User as Postman User
participant Execute as Process Document Request
participant Script as Post-Response Script
participant CollVar as Collection Variable (execution_id)
participant Status as Execution Status Request
participant API as Unstract API
User->>Execute: "POST /api/{name} (multipart file)"
Execute->>API: HTTP POST
API-->>Execute: "{"message": {"execution_id": "uuid", "execution_status": "..."}}"
Execute->>Script: Run test script
Script->>Script: "try { pm.response.json() }"
Script->>CollVar: pm.collectionVariables.set("execution_id", uuid)
User->>Status: "GET /api/{name}?execution_id={{execution_id}}"
Status->>CollVar: "Resolve {{execution_id}} to uuid"
Status->>API: HTTP GET with resolved UUID
API-->>User: Execution status response
Reviews (3): Last reviewed commit: "UN-2190 Expose execution_id in the API e..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…PI deployments - Wrap pm.response.json() in try/catch so error pages (non-JSON) don't surface a Postman test error - Move collection variables behind APIBase.get_collection_variables() so Pipeline collections (no status request) stay variable-free Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
athul-rs
requested review from
chandrasekharan-zipstack,
hari-kuriakose and
jaseemjaskp
The ExecutionResponse DTO carries execution_id but APIExecutionResponseSerializer dropped it, so the Postman capture script (and any API consumer) had to parse it out of status_api. Add it as a first-class response field; the collection script's message.execution_id lookup now matches the real payload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Unstract test resultsPer-group results
Critical paths
|
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
Automated PR review (PR Review Toolkit: code-reviewer, silent-failure-hunter, type-design-analyzer, pr-test-analyzer, comment-analyzer, code-simplifier).
The change is correct and well-scoped — the capture script's response.message.execution_id path is validated by the new serializer field and the {"message": ...} response envelope, and urlencode(safe="{}") correctly preserves {{execution_id}}. The already-fixed items (serializer dropping execution_id, pipeline collection variables, non-JSON try/catch) are not re-raised.
The findings below are all new. The only substantive one is the stale-variable risk (P1); the rest are P2/P3 hardening, type-design, comment, and test-coverage suggestions.
| "} catch (error) {", | ||
| " // Non-JSON response (e.g. gateway error); nothing to capture", | ||
| "}", | ||
| "if (response && response.message && response.message.execution_id) {", # noqa: E501 |
There was a problem hiding this comment.
P1 — silent reuse of a stale execution_id. This guard has no else branch, and it's distinct from the non-JSON try/catch already fixed below: here the response parses fine but carries no message.execution_id — e.g. a 422 validation error or 500 ({"message": {"error": ...}}), auth/rate-limit rejection, or any 2xx with an unexpected shape. In that case the script does nothing and {{execution_id}} keeps whatever it held — most dangerously the id captured from a previous successful run. The user re-runs Execute, it fails, hits Execution status, and Postman silently polls the old execution and shows its COMPLETED result as if it belonged to the failed run. No signal that capture was skipped.
Suggest making the miss explicit and preventing cross-run staleness:
if (response && response.message && response.message.execution_id) {
pm.collectionVariables.set("execution_id", response.message.execution_id);
} else {
pm.collectionVariables.set("execution_id", "REPLACE_WITH_EXECUTION_ID");
console.warn("No execution_id in execute response; not reusing a stale value. Status:", pm.response.code);
}Optionally gate capture on pm.response.code 2xx so a 200-with-unexpected-shape is surfaced too.
| class PostmanItem: | ||
| name: str | ||
| request: RequestItem | ||
| event: list[EventItem] | None = None |
There was a problem hiding this comment.
P2 — event: list[EventItem] | None = None creates a None/[] tri-state where only "some events" / "no events" are meaningful, and it's the sole reason to_dict() (lines 301-306) has to post-process and pop null event keys. Other "maybe-empty" fields on these DTOs already use field(default_factory=list) (PostmanCollection.item/variable, lines 251-252). Aligning here:
event: list[EventItem] = field(default_factory=list)lets you drop the strip loop and reduce to_dict to return asdict(self). Trade-off: items then serialize "event": [] instead of omitting the key — both are valid Postman v2.1 (empty array is the schema-canonical form). If preserving the exact "key absent" output is a hard requirement, keep the loop but make it robust to the new default by testing if not item.get("event") rather than is None.
|
|
||
| @dataclass | ||
| class EventItem: | ||
| listen: str |
There was a problem hiding this comment.
P2 (type safety) — constrain the closed enums. EventItem.listen accepts any str, but Postman v2.1 only recognizes two hooks; a typo like "tests" produces a silently-ignored event with no error anywhere in this pipeline. Narrow it to Literal["prerequest", "test"]. Same idea for ScriptItem.type (line 24): only "text/javascript" is meaningful, so Literal["text/javascript"] (or keep the default and type it as such). Zero runtime cost, caught by mypy, self-documenting.
| ] | ||
|
|
||
| def _get_execute_capture_event(self) -> EventItem: | ||
| """Post-response script that stores the execution_id from the execute |
There was a problem hiding this comment.
P3 (comment) — docstring omits the load-bearing response-shape coupling. The script silently no-ops unless the response is {message: {execution_id}}; if that envelope ever changes (e.g. message renamed or execution_id relocated) capture breaks quietly. Add a line so a future API change flags this script, e.g. "Assumes the execute response has shape {message: {execution_id}}; if the body is non-JSON or lacks that field, nothing is captured." Minor: "Post-response script" matches Postman's current UI label but the code emits listen="test" — a half-sentence tying the two ("post-response, i.e. Postman's test hook") avoids a maintainer double-take.
| def get_form_data_items(self) -> list[FormDataItem]: | ||
| pass | ||
|
|
||
| def get_collection_variables(self) -> list["VariableItem"]: |
There was a problem hiding this comment.
P3 (nit) — unnecessary string forward-reference. VariableItem is already defined above (line 33), so the quotes in -> list["VariableItem"] aren't needed; the override at line 175 already uses the bare list[VariableItem]. Use the bare type for consistency.
| EXECUTE_PIPELINE_API_KEY = "Process pipeline" | ||
| STATUS_API_KEY = "Execution status" | ||
| STATUS_EXEC_ID_DEFAULT = "REPLACE_WITH_EXECUTION_ID" | ||
| EXEC_ID_VARIABLE_NAME = "execution_id" |
There was a problem hiding this comment.
P3 — unenforced coupling between the variable name and its {{...}} reference. EXEC_ID_VARIABLE_NAME = "execution_id" and STATUS_EXEC_ID_VARIABLE = "{{execution_id}}" must agree (the status URL references {{execution_id}}, the capture JS sets "execution_id", and the collection variable keys on it). If one is renamed and the other isn't, the collection breaks silently — the status request would query a never-set variable. Derive one from the other so they can't drift:
STATUS_EXEC_ID_VARIABLE = f"{{{{{EXEC_ID_VARIABLE_NAME}}}}}" # -> "{{execution_id}}"| dict[str, Any]: PostmanCollection as a dict | ||
| """ | ||
| collection_dict = asdict(self) | ||
| # Drop null event blocks; Postman expects "event" to be a list |
There was a problem hiding this comment.
P2 (test coverage) — backend/api_v2/postman_collection/ has no tests at all, and this PR's trickiest logic is exactly the kind that fails only at Postman-import time, not in CI. to_dict() (and create()) are pure given settings.WEB_APP_ORIGIN_URL + an api_key, so this is cheap to lock down. Suggest adding backend/api_v2/postman_collection/tests/test_dto.py covering: (a) to_dict strips null event yet keeps the populated execute event and emits the variable array; (b) regression guard that PipelineDto collections carry no event key and variable == [] while APIDeploymentDto carries both; (c) the variable key, the {{execution_id}} in the status URL, and the pm.collectionVariables.set(...) in the JS all reference the same CollectionKey.EXEC_ID_VARIABLE_NAME (assert via the constant, not a hardcoded literal, so a partial rename fails); (d) the status URL keeps execution_id={{execution_id}} literal (no %7B). Assert load-bearing substrings, not the whole exec array, to stay robust to comment/wording edits.
3 participants
What
execution_idfrom its response into a collection variable via a post-response script, and the Execution status request reads{{execution_id}}from that variable. No more manual copy-pasting of execution IDs.Why
UN-2190 — minor UX gap: after executing a document asynchronously, users had to copy the execution ID from the response and paste it into the status request's
REPLACE_WITH_EXECUTION_IDplaceholder. The LLMWhisperer Postman collection already does this withwhisper_hash; this applies the same pattern.How
postman_collection/dto.py: newScriptItem/EventItem/VariableItemdataclasses;PostmanItemgets an optionalevent; the execute request carries a post-response script (pm.collectionVariables.set("execution_id", ...), guarded so non-JSON/error responses are ignored); the status URL uses{{execution_id}}(urlencode withsafe="{}"so Postman's braces survive); collection-levelvariableblock added with aREPLACE_WITH_EXECUTION_IDdefault so manual use still works.to_dict()stripsevent: nullfrom items that have no scripts.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
REPLACE_WITH_EXECUTION_IDdefault — identical to today's placeholder. The script is defensive (checksresponse.message.execution_idexists before setting).Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Notes on Testing
execution_id={{execution_id}}unescaped, and the collection-level variable is present.Screenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code