Skip to content

Commit bc4ded6

Browse files
chandrasekharan-zipstackclaude
chandrasekharan-zipstack
and
claude
committed
fix(migration): address greptile P1s on base + workflow_endpoint
base.build_post_payload: Previous `value not in (None, "")` dropped booleans False and numeric 0 along with None/"" — DRF BooleanField False and numeric defaults were silently stripped from POST payloads. Switch to explicit identity + equality checks. New test_base_helpers.py guards this. workflow_endpoint._patch_endpoint: When source endpoint had a connector but its remap entry is missing (e.g. connector phase skipped a row), we previously PATCHed the target endpoint with connector_instance_id=None — silently detaching it. Now skip the PATCH, increment result.skipped, and append an error entry so the operator sees the broken link in the report. Existing test rewritten to assert the new skip-and-flag behaviour. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent fe66b05 commit bc4ded6

4 files changed

Lines changed: 89 additions & 8 deletions

File tree

‎src/unstract/migration/phases/base.py‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,15 @@ def build_post_payload(
3939
and rejects on required fields).
4040
"""
4141
keys = writable - SERVER_MANAGED
42-
return {k: src[k] for k in keys if k in src and src[k] not in (None, "")}
42+
# Equality with `(None, "")` matched False and 0 too (Python: False == 0,
43+
# 0 in (None, "") is False, but `0 not in (...)` falsely returns True).
44+
# Explicit identity / equality checks preserve falsy-but-meaningful
45+
# values like ``BooleanField`` False and numeric defaults.
46+
return {
47+
k: src[k]
48+
for k in keys
49+
if k in src and src[k] is not None and src[k] != ""
50+
}
4351

4452

4553
class Phase(ABC):

‎src/unstract/migration/phases/workflow_endpoint.py‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,10 +120,21 @@ def _patch_endpoint(
120120
if src_conn_id:
121121
tgt_conn_id = self.ctx.remap.resolve("connector", src_conn_id)
122122
if not tgt_conn_id:
123+
# Source had a connector but it never made it through the
124+
# connector phase (e.g. redacted secrets, skipped row).
125+
# Patching the endpoint with connector=None would silently
126+
# detach it on target; skip + flag so the operator notices.
123127
logger.warning(
124-
"no connector remap for %s on %s endpoint %s — leaving unset",
125-
src_conn_id, etype, src_ep_id,
128+
"skipping %s endpoint src=%s tgt=%s — source connector %s "
129+
"has no target remap; would silently unset connector",
130+
etype, src_ep_id, tgt_ep_id, src_conn_id,
126131
)
132+
result.skipped += 1
133+
result.errors.append(
134+
f"unmapped connector on {etype} endpoint {src_ep_id}: "
135+
f"src_connector={src_conn_id}"
136+
)
137+
return
127138

128139
payload: dict[str, Any] = {
129140
"connection_type": src_ep.get("connection_type") or "",

‎tests/migration/test_base_helpers.py‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
"""Tests for ``unstract.migration.phases.base`` helpers."""
2+
3+
from __future__ import annotations
4+
5+
from unstract.migration.phases.base import SERVER_MANAGED, build_post_payload
6+
7+
8+
def test_preserves_false_and_zero_values():
9+
"""Booleans set to False and numeric 0 are legitimate field values.
10+
11+
Earlier ``value not in (None, "")`` worked for None/"" but dropped
12+
False and 0 too because of Python's ``False == 0 == in (None, "")``
13+
edge case. Regression guard.
14+
"""
15+
src = {
16+
"is_active": False,
17+
"retry_count": 0,
18+
"rate_limit": 0.0,
19+
"name": "demo",
20+
}
21+
writable = frozenset({"is_active", "retry_count", "rate_limit", "name"})
22+
23+
payload = build_post_payload(src, writable)
24+
25+
assert payload == {
26+
"is_active": False,
27+
"retry_count": 0,
28+
"rate_limit": 0.0,
29+
"name": "demo",
30+
}
31+
32+
33+
def test_strips_none_and_empty_string_but_keeps_zero():
34+
src = {"a": None, "b": "", "c": 0, "d": False, "e": "kept"}
35+
writable = frozenset({"a", "b", "c", "d", "e"})
36+
37+
payload = build_post_payload(src, writable)
38+
39+
assert payload == {"c": 0, "d": False, "e": "kept"}
40+
41+
42+
def test_drops_server_managed_keys_even_if_writable():
43+
src = {"id": "X", "name": "demo", "organization": "org", "created_by": "u"}
44+
# All four are nominally writable but SERVER_MANAGED should win.
45+
writable = frozenset(src.keys())
46+
47+
payload = build_post_payload(src, writable)
48+
49+
assert payload == {"name": "demo"}
50+
for key in SERVER_MANAGED & set(src.keys()):
51+
assert key not in payload
52+
53+
54+
def test_ignores_writable_keys_missing_from_src():
55+
src = {"present": 1}
56+
writable = frozenset({"present", "absent"})
57+
58+
assert build_post_payload(src, writable) == {"present": 1}

‎tests/migration/test_workflow_endpoint_phase.py‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,11 @@ def test_endpoint_without_source_connector_patches_with_null():
155155
assert payload["configuration"] == {"foo": "bar"}
156156

157157

158-
def test_unknown_connector_uuid_logs_but_does_not_fail():
158+
def test_unknown_connector_uuid_skips_endpoint_and_flags_error():
159+
"""Source had a connector but its remap is missing — patching with
160+
connector=None would silently detach the endpoint on target. Skip
161+
the PATCH and record an operator-visible error entry instead.
162+
"""
159163
src = FakeClient()
160164
src.endpoints[SRC_WF] = [
161165
_src_endpoint(
@@ -171,10 +175,10 @@ def test_unknown_connector_uuid_logs_but_does_not_fail():
171175

172176
result = WorkflowEndpointPhase(ctx).run(MigrationReport())
173177

174-
assert result.created == 1
175-
# No remap → connector_instance_id stays None instead of failing the PATCH.
176-
_, payload = tgt.patch_calls[0]
177-
assert payload["connector_instance_id"] is None
178+
assert result.created == 0
179+
assert result.skipped == 1
180+
assert tgt.patch_calls == []
181+
assert any("unmapped connector" in e for e in result.errors)
178182

179183

180184
def test_missing_target_endpoint_fails_loudly():

0 commit comments

Comments
 (0)