Skip to content

Commit 055573d

Browse files
jacalataclaude
andcommitted
Address Copilot findings: whitespace-only schedule_id and typo
Copilot's remaining suppressed comments (2026-09-18 Lite review) on this PR: 1. schedule_id validation in Subscriptions.create(), Subscriptions.update(), and RequestFactory.Subscription.create_req() relied on truthiness, which lets a whitespace-only id (e.g. " ") through to the wire layer and produces the same confusing error the None/empty checks exist to avoid. All three call sites now reject blank-after-strip ids explicitly. 2. Typo: "Susbcription" -> "Subscription" in create()'s no-item error. The other two Copilot comments (ScheduleItem.from_element list-unwrap in _parse_element, and the inline-schedule-with-no-id fallback) were already fixed in a prior commit (1a423a9) on this branch before this pass started. Adds regression tests for the whitespace-only schedule_id case at both the endpoint and RequestFactory layers. Co-Authored-By: Claude Code <noreply@anthropic.com>
1 parent 1a423a9 commit 055573d

3 files changed

Lines changed: 47 additions & 6 deletions

File tree

‎tableauserverclient/server/endpoint/subscriptions_endpoint.py‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,15 @@ def get_by_id(self, subscription_id: str) -> SubscriptionItem:
4141
@api(version="2.3")
4242
def create(self, subscription_item: SubscriptionItem) -> SubscriptionItem:
4343
if not subscription_item:
44-
error = "No Susbcription provided"
44+
error = "No Subscription provided"
4545
raise ValueError(error)
46-
if not subscription_item.schedule_id:
46+
if not (subscription_item.schedule_id or "").strip():
4747
# See tableau/server-client-python#1658: users trying to create an
4848
# "On Extract Refresh" subscription pass schedule_id=None and hit
49-
# a confusing wire-layer error. Point them at the factory.
49+
# a confusing wire-layer error. A whitespace-only id is the same
50+
# failure in disguise -- it would still serialize and hit the
51+
# same wire-layer error -- so reject it here too, not just None.
52+
# Point them at the factory.
5053
raise ValueError(
5154
"schedule_id is required; for on-extract-refresh subscriptions "
5255
"use SubscriptionItem.on_extract_refresh(...)"
@@ -84,7 +87,7 @@ def update(self, subscription_item: SubscriptionItem) -> SubscriptionItem:
8487
schedule_id = subscription_item.schedule.id
8588
if schedule_id is not None:
8689
subscription_item.schedule_id = schedule_id
87-
if not schedule_id:
90+
if not (schedule_id or "").strip():
8891
raise ValueError(
8992
"schedule_id is required to update a subscription. On Tableau "
9093
"Cloud, subscriptions parsed from a GET response may inline the "

‎tableauserverclient/server/request_factory.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1356,14 +1356,15 @@ def create_req(self, xml_request: ET.Element, subscription_item: "SubscriptionIt
13561356
# actionable error rather than the TypeError ElementTree raises when
13571357
# attrib["id"] is set to None, and so python -O doesn't strip the
13581358
# check entirely.
1359-
if subscription_item.schedule_id is None:
1359+
schedule_id = subscription_item.schedule_id
1360+
if schedule_id is None or not schedule_id.strip():
13601361
raise ValueError(
13611362
"schedule_id is required to build a subscription create "
13621363
"request; for on-extract-refresh subscriptions use "
13631364
"SubscriptionItem.on_extract_refresh(...)"
13641365
)
13651366
schedule_element = ET.SubElement(subscription_element, "schedule")
1366-
schedule_element.attrib["id"] = subscription_item.schedule_id
1367+
schedule_element.attrib["id"] = schedule_id
13671368

13681369
# User element
13691370
user_element = ET.SubElement(subscription_element, "user")

‎test/test_subscription.py‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,30 @@ def test_create_rejects_empty_schedule_id(server: TSC.Server) -> None:
132132
server.subscriptions.create(sub)
133133

134134

135+
def test_create_rejects_whitespace_only_schedule_id(server: TSC.Server) -> None:
136+
"""A whitespace-only schedule_id is the same failure mode as None/empty:
137+
truthiness alone lets it through, but it would still serialize as
138+
<schedule id=" "/> and hit the same confusing wire-layer error.
139+
"""
140+
target = TSC.Target("view-id", "view")
141+
sub = TSC.SubscriptionItem("subject", " ", "user-id", target)
142+
with pytest.raises(ValueError, match="on_extract_refresh"):
143+
server.subscriptions.create(sub)
144+
145+
146+
def test_create_req_rejects_whitespace_only_schedule_id(server: TSC.Server) -> None:
147+
"""RequestFactory.Subscription.create_req must reject a whitespace-only
148+
schedule_id too, for callers who bypass Subscriptions.create() and use
149+
the factory directly.
150+
"""
151+
from tableauserverclient.server.request_factory import RequestFactory
152+
153+
target = TSC.Target("view-id", "view")
154+
sub = TSC.SubscriptionItem("subject", " ", "user-id", target)
155+
with pytest.raises(ValueError, match="schedule_id is required"):
156+
RequestFactory.Subscription.create_req(sub)
157+
158+
135159
def test_subscription_defaults_refresh_extract_triggered_false(server: TSC.Server) -> None:
136160
"""A default SubscriptionItem does not opt into extract-refresh triggering."""
137161
target = TSC.Target("view-id", "view")
@@ -223,6 +247,19 @@ def test_update_rejects_missing_schedule_id(server: TSC.Server) -> None:
223247
server.subscriptions.update(sub)
224248

225249

250+
def test_update_rejects_whitespace_only_schedule_id(server: TSC.Server) -> None:
251+
"""Same failure mode as None: a whitespace-only schedule_id would pass a
252+
truthiness check and reach RequestFactory, still producing a confusing
253+
wire-layer error. update() must reject it explicitly.
254+
"""
255+
target = TSC.Target("view-id", "view")
256+
sub = TSC.SubscriptionItem("subject", "sched-id", "user-id", target)
257+
sub._id = "existing-sub-id" # type: ignore[assignment]
258+
sub.schedule_id = " "
259+
with pytest.raises(ValueError, match="schedule_id is required"):
260+
server.subscriptions.update(sub)
261+
262+
226263
def test_update_req_emits_refresh_extract_triggered_when_true(server: TSC.Server) -> None:
227264
"""When the flag is True, update_req must emit refreshExtractTriggered='true'."""
228265
from tableauserverclient.server.request_factory import RequestFactory

0 commit comments

Comments
 (0)