Skip to content

Commit 8eba015

Browse files
jacalataclaude
andcommitted
Guard against empty webhook update, verify PUT body on wire
Two fresh-eyes findings on #1806: 1. `WebhookRequest.update_req` used to happily serialize `<tsRequest><webhook/></tsRequest>` when every updatable field on the WebhookItem was None. The server rejected that with a generic 400 the caller couldn't act on. Now raises ValueError up front with a message naming the four fields that would resolve it. 2. `test_update` verified the response-derived WebhookItem but never inspected the PUT body, so removing the `update_req` call in the endpoint would keep the test green. Added `m.last_request` assertions on the wire: name, isEnabled, url, and event tag all present in the PUT payload. This is the load-bearing endpoint<->factory contract check. Also added test_update_rejects_empty_payload confirming the new guard fires when nothing is set on the item. 28 passed on test_webhook.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c2a4c1f commit 8eba015

2 files changed

Lines changed: 41 additions & 0 deletions

File tree

tableauserverclient/server/request_factory.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1414,6 +1414,21 @@ def create_req(self, xml_request: ET.Element, webhook_item: "WebhookItem") -> by
14141414

14151415
@_tsrequest_wrapped
14161416
def update_req(self, xml_request: ET.Element, webhook_item: "WebhookItem") -> bytes:
1417+
# Reject a no-op update up front. Without at least one updatable
1418+
# attribute set the payload is <tsRequest><webhook/></tsRequest>,
1419+
# which the server rejects with a generic 400 that gives the caller
1420+
# no idea what happened. Raise here with an actionable message.
1421+
if (
1422+
webhook_item.name is None
1423+
and webhook_item.is_enabled is None
1424+
and webhook_item._event is None
1425+
and webhook_item.url is None
1426+
):
1427+
raise ValueError(
1428+
"WebhookItem has no updatable fields set; "
1429+
"at least one of name, is_enabled, event, or url must be provided."
1430+
)
1431+
14171432
webhook = ET.SubElement(xml_request, "webhook")
14181433
if webhook_item.name is not None:
14191434
webhook.attrib["name"] = webhook_item.name

test/test_webhook.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,19 @@ def test_update(server: TSC.Server) -> None:
197197

198198
updated_webhook = server.webhooks.update(webhook_item)
199199

200+
# Verify the actual PUT body reached the wire with the caller's values.
201+
# Without this, deleting the update_req call in the endpoint would still
202+
# pass every response-derived assertion below (response is mocked and
203+
# comes back verbatim). This is the load-bearing endpoint <-> factory
204+
# contract check.
205+
assert m.last_request is not None
206+
assert m.last_request.method == "PUT"
207+
body = m.last_request.text or ""
208+
assert 'name="webhook-name-updated"' in body, body
209+
assert 'isEnabled="true"' in body, body
210+
assert 'url="https://updated-url.example.com/hook"' in body, body
211+
assert "datasource-created" in body, body
212+
200213
assert updated_webhook.id == "webhook-id"
201214
assert updated_webhook.name == "webhook-name-updated"
202215
assert updated_webhook.url == "https://updated-url.example.com/hook"
@@ -210,6 +223,19 @@ def test_update_missing_id(server: TSC.Server) -> None:
210223
server.webhooks.update(webhook_item)
211224

212225

226+
def test_update_rejects_empty_payload() -> None:
227+
# A WebhookItem with no updatable fields set should raise up front rather
228+
# than serializing to <tsRequest><webhook/></tsRequest> and letting the
229+
# server return a generic 400. Uses update_req directly so the guard is
230+
# exercised even before the endpoint sees the item.
231+
from tableauserverclient.server.request_factory import RequestFactory
232+
233+
webhook_item = WebhookItem()
234+
webhook_item._id = "webhook-id" # has an id but nothing to update
235+
with pytest.raises(ValueError, match="no updatable fields"):
236+
RequestFactory.Webhook.update_req(webhook_item)
237+
238+
213239
def test_update_preserves_locally_set_fields_omitted_by_server(server: TSC.Server) -> None:
214240
"""update should preserve locally-set fields that the server response omits.
215241

0 commit comments

Comments
 (0)