[ISSUE] The ZPA empty-body update placeholder returns model defaults (e.g. enabled=True) that a caller cannot tell apart from real values
Environment
|
|
| SDK version |
zscaler-sdk-python 1.9.44 (editable install of master @ e7f5f7ef) |
| Python |
3.11.15 |
| OS |
macOS (arm64) |
| Affected product / resource |
ZPA — 59 update methods across 42 modules (list below) |
Found by reading the SDK source and confirmed with the offline reproduction below (a stub
request executor, so no credentials and no network are involved). All identifiers used are
synthetic.
This is deliberately narrower than #289 and #419 — please read this part first
#289 and #419 both asked why update_segment doesn't return the updated field values, and
both were answered: ZPA replies 204 No Content on updates, the SDK cannot populate a full
model, and callers should follow an update with a get_* call. That answer is clear and I'm
not relitigating it.
What neither issue covered, and what this one is about, is that the placeholder the SDK
substitutes is not empty — it carries the model's constructor defaults, and some of them
are affirmative. ApplicationSegments({"id": segment_id}) reports enabled=True, because
zscaler/zpa/models/application_segment.py:42 is:
self.enabled = config["enabled"] if "enabled" in config else True
So as_dict() on the placeholder emits {'id': ..., 'enabled': True, 'domain_names': [], 'tcp_port_ranges': [], 'udp_port_ranges': [], ...} — values the API never sent. A caller who
logs, diffs, caches, or asserts on the returned object is reading fabricated data, and since
err is None and the object is a real ApplicationSegments, nothing distinguishes it from a
response that genuinely said enabled=True with no domains. Both of the linked issues'
docstring-style usage (print(f"... {update_segment.as_dict()}")) hits exactly this.
I acknowledge a caller can disambiguate today via response.get_status()
(zscaler/oneapi_response.py:163) — that's a reasonable answer, and if it's the intended one
I'd be glad to send a docs-only PR saying so, since none of the 59 docstrings mention it.
Description
The pattern is:
# zscaler/zpa/application_segment.py:464-465
if response is None or not response.get_body():
return (ApplicationSegments({"id": segment_id}), response, None)
It appears 59 times across 42 modules, all of them under zscaler/zpa/, and nowhere else
in the SDK:
$ python - <<'PY'
import re, glob, collections
guard = re.compile(r'if (?:response is None or not response\.get_body\(\)'
r'|not response or not response\.get_body\(\))\s*:')
ret = re.compile(r'return \((\w+)\(\{"id": ([a-z_]+)\}\), response, None\)')
total, files = 0, collections.OrderedDict()
for f in sorted(glob.glob('zscaler/**/*.py', recursive=True)):
lines = open(f).read().split('\n')
for i, line in enumerate(lines):
if guard.search(line):
for j in range(i + 1, min(i + 4, len(lines))):
if ret.search(lines[j]):
total += 1
files.setdefault(f, []).append(i + 1)
break
print('placeholder returns:', total)
print('modules:', len(files))
print('by service:', dict(collections.Counter(f.split('/')[1] for f in files)))
PY
placeholder returns: 59
modules: 42
by service: {'zpa': 42}
Other services chose the opposite convention for the same situation, which is the part I find
hard to reconcile:
zscaler/zia/locations.py:565-570 — for a 204, returns (None, None) with the comment
"This is the expected case for 204 No Content". No object is fabricated.
zscaler/zcell/sim_handling.py:230-231, zscaler/zcell/anomaly_policy.py:208-209,
zscaler/zcell/customer_data_handling.py:125-126 — if not response or not response.get_body(): return (None, response, None). Same guard, honest return value.
- ZIA / ZTW / ZCC / ZDX / ZIdentity update methods have no such guard at all — e.g.
zscaler/zia/url_filtering.py update_rule parses the body directly.
So the "return a stub" choice is specific to ZPA, and within ZPA it returns defaults rather
than None. I'm raising the defaults part, not the "why isn't it populated" part.
Reproduction
Fully offline — no credentials, no network. The stub replies with an empty body, which is what
RequestExecutor.execute produces for a 204 (zscaler/request_executor.py:545-559
constructs a ZscalerAPIResponse with response_body={}).
from zscaler.zpa.application_segment import ApplicationSegmentAPI
SEGMENT_ID = "72058304855090128"
CONFIG = {"client": {"customerId": "72058304855015000"}}
class StubResponse:
def get_body(self):
return {} # what a 204 yields
def get_results(self):
return []
def get_status(self):
return 204
class StubExecutor:
"""Stands in for RequestExecutor."""
def create_request(self, method, endpoint, body=None, headers=None, params=None,
use_raw_data_for_body=False):
return {"method": method, "endpoint": endpoint.strip(),
"body": {} if body is None else body, "params": params or {}}, None
def execute(self, request, response_type=None, return_raw_response=False):
return StubResponse(), None
seg, resp, err = ApplicationSegmentAPI(StubExecutor(), CONFIG).update_segment(
SEGMENT_ID, description="new description")
print("err =", err)
print("type =", type(seg).__name__)
print("seg.enabled =", seg.enabled, " <- never sent by the API")
print("seg.domain_names =", seg.domain_names, " <- never sent by the API")
print("as_dict() =", seg.as_dict())
Expected behaviour
The returned value should not assert field values the API did not send. Any of these would
achieve that; I'm not assuming which you'd want.
Actual behaviour
err = None
type = ApplicationSegments
seg.enabled = True <- never sent by the API
seg.domain_names = [] <- never sent by the API
as_dict() = {'id': '72058304855090128', 'domain_names': [], 'server_groups': [], 'enabled': True, 'tcp_port_ranges': [], 'udp_port_ranges': [], 'tcp_port_range': [], 'udp_port_range': [], 'pra_apps': [], 'guest_details': []}
Root cause
| Location |
Code |
zscaler/zpa/application_segment.py:464-465 |
representative instance of the placeholder (58 more like it) |
zscaler/zpa/models/application_segment.py:42 |
self.enabled = ... else True — the affirmative default that leaks into the placeholder |
zscaler/zia/locations.py:565-570 |
the other convention in the same codebase, for the same 204 |
zscaler/zcell/sim_handling.py:230-231 |
ditto, (None, response, None) |
Suggested direction
Deliberately not prescribing, especially as the surrounding behaviour has already been
declared intentional:
- Leave the behaviour and document it — add a line to the ZPA update docstrings saying the
returned object carries only the id, that other fields are model defaults and not API
data, and that response.get_status() distinguishes the case. Smallest, non-breaking, and
I'd be happy to write it.
- Return
(None, response, None) on an empty body, matching zscaler/zcell/* and
zscaler/zia/locations.py. Honest, but breaking for anyone reading .id off the result.
- Keep returning an object but construct it so unknown fields are
None rather than
constructor defaults, so as_dict() (which already drops None) yields just the id.
Which of these fits your plans? I'm happy to open the PR once you've picked.
[ISSUE] The ZPA empty-body update placeholder returns model defaults (e.g.
enabled=True) that a caller cannot tell apart from real valuesEnvironment
zscaler-sdk-python1.9.44 (editable install ofmaster@e7f5f7ef)Found by reading the SDK source and confirmed with the offline reproduction below (a stub
request executor, so no credentials and no network are involved). All identifiers used are
synthetic.
This is deliberately narrower than #289 and #419 — please read this part first
#289 and #419 both asked why
update_segmentdoesn't return the updated field values, andboth were answered: ZPA replies
204 No Contenton updates, the SDK cannot populate a fullmodel, and callers should follow an update with a
get_*call. That answer is clear and I'mnot relitigating it.
What neither issue covered, and what this one is about, is that the placeholder the SDK
substitutes is not empty — it carries the model's constructor defaults, and some of them
are affirmative.
ApplicationSegments({"id": segment_id})reportsenabled=True, becausezscaler/zpa/models/application_segment.py:42is:So
as_dict()on the placeholder emits{'id': ..., 'enabled': True, 'domain_names': [], 'tcp_port_ranges': [], 'udp_port_ranges': [], ...}— values the API never sent. A caller whologs, diffs, caches, or asserts on the returned object is reading fabricated data, and since
err is Noneand the object is a realApplicationSegments, nothing distinguishes it from aresponse that genuinely said
enabled=Truewith no domains. Both of the linked issues'docstring-style usage (
print(f"... {update_segment.as_dict()}")) hits exactly this.I acknowledge a caller can disambiguate today via
response.get_status()(
zscaler/oneapi_response.py:163) — that's a reasonable answer, and if it's the intended oneI'd be glad to send a docs-only PR saying so, since none of the 59 docstrings mention it.
Description
The pattern is:
It appears 59 times across 42 modules, all of them under
zscaler/zpa/, and nowhere elsein the SDK:
Other services chose the opposite convention for the same situation, which is the part I find
hard to reconcile:
zscaler/zia/locations.py:565-570— for a204, returns(None, None)with the comment"This is the expected case for 204 No Content". No object is fabricated.
zscaler/zcell/sim_handling.py:230-231,zscaler/zcell/anomaly_policy.py:208-209,zscaler/zcell/customer_data_handling.py:125-126—if not response or not response.get_body(): return (None, response, None). Same guard, honest return value.zscaler/zia/url_filtering.pyupdate_ruleparses the body directly.So the "return a stub" choice is specific to ZPA, and within ZPA it returns defaults rather
than
None. I'm raising the defaults part, not the "why isn't it populated" part.Reproduction
Fully offline — no credentials, no network. The stub replies with an empty body, which is what
RequestExecutor.executeproduces for a204(zscaler/request_executor.py:545-559constructs a
ZscalerAPIResponsewithresponse_body={}).Expected behaviour
The returned value should not assert field values the API did not send. Any of these would
achieve that; I'm not assuming which you'd want.
Actual behaviour
Root cause
zscaler/zpa/application_segment.py:464-465zscaler/zpa/models/application_segment.py:42self.enabled = ... else True— the affirmative default that leaks into the placeholderzscaler/zia/locations.py:565-570204zscaler/zcell/sim_handling.py:230-231(None, response, None)Suggested direction
Deliberately not prescribing, especially as the surrounding behaviour has already been
declared intentional:
returned object carries only the
id, that other fields are model defaults and not APIdata, and that
response.get_status()distinguishes the case. Smallest, non-breaking, andI'd be happy to write it.
(None, response, None)on an empty body, matchingzscaler/zcell/*andzscaler/zia/locations.py. Honest, but breaking for anyone reading.idoff the result.Nonerather thanconstructor defaults, so
as_dict()(which already dropsNone) yields just theid.Which of these fits your plans? I'm happy to open the PR once you've picked.