Skip to content

Commit 4c64ba7

Browse files
fix(review): address PR #168 Copilot review comments
- assets.py replace(): use per-request header copy (no Content-Type) instead of mutating shared client headers dict; wrap file open in context manager to prevent FD leak - assets.py update(): use per-request header copy with explicit Content-Type: application/json, robust against prior upload/replace calls - capture.py retry: close old FD before reopening on transient retry; use tuple(val[2:]) to handle both 2-tuple and 3-tuple file specs without crashing on missing index - setup.py: remove references to non-existent .env.example in docstring and runtime error message - test_assets_unit.py: fix scan-status unit tests to use correct query param name (include_asset_scan_status) and lowercase string value (true), matching the integration suite and CMA API wire format
1 parent d34dde2 commit 4c64ba7

5 files changed

Lines changed: 44 additions & 26 deletions

File tree

.talismanrc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,3 +406,7 @@ fileignoreconfig:
406406
- filename: tests/mock/assets/test_assets_mock.py
407407
checksum: 12c9091bb88c0c12712f046b29fb4a9dce3b95ecc45f4ea46bbc3fd4529742a0
408408
version: "1.0"
409+
fileignoreconfig:
410+
- filename: tests/integration/framework/capture.py
411+
checksum: c9680c4ee3e1def0765fd89767fa1383aee5d195222fa021a19618607373047c
412+
version: "1.0"

contentstack_management/assets/assets.py

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -174,12 +174,11 @@ def replace(self, file_path):
174174
"""
175175

176176
url = f"assets/{self.asset_uid}"
177-
# Let requests build the multipart body and set Content-Type WITH a boundary.
178-
# Setting a bare "multipart/form-data" (no boundary) makes the API reject the
179-
# upload with 422 "Please send a valid multipart/form-data payload".
180-
self.client.headers.pop("Content-Type", None)
181-
files = {"asset": open(f"{file_path}",'rb')}
182-
return self.client.put(url, headers = self.client.headers, params = self.params, files = files)
177+
# Omit Content-Type so requests can set it with the correct multipart boundary.
178+
# Use a per-request header copy to avoid mutating the shared client headers dict.
179+
request_headers = {k: v for k, v in self.client.headers.items() if k.lower() != "content-type"}
180+
with open(file_path, 'rb') as fh:
181+
return self.client.put(url, headers=request_headers, params=self.params, files={"asset": fh})
183182

184183
def generate(self, data):
185184
"""
@@ -410,10 +409,11 @@ def update(self, data):
410409
if self.asset_uid is None or '':
411410
raise Exception(ASSET_UID_REQUIRED)
412411
url = f"assets/{self.asset_uid}"
413-
# Updating an asset's title/description sends a JSON body, so it must use
414-
# application/json. Forcing multipart/form-data here makes the API reject
415-
# the request with 422 "Please send a valid multipart/form-data payload".
416-
return self.client.put(url, headers = self.client.headers, params = self.params, data = data)
412+
# Use a per-request header copy with Content-Type explicitly set to application/json
413+
# so this call is not affected by whatever a preceding upload/replace left in
414+
# the shared headers dict.
415+
request_headers = {**self.client.headers, "Content-Type": "application/json"}
416+
return self.client.put(url, headers=request_headers, params=self.params, data=data)
417417

418418
def publish(self, data):
419419
"""

tests/integration/framework/capture.py

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,23 @@ def _request_with_retry(method, url, kwargs):
136136
files = kwargs.get("files")
137137
if files:
138138
for key, val in list(files.items()):
139-
name = getattr(val[1] if isinstance(val, (tuple, list)) else val, "name", None)
140-
if name and os.path.exists(name):
141-
kwargs["files"][key] = (val[0], open(name, "rb"), val[2]) if isinstance(val, (tuple, list)) else open(name, "rb")
139+
if isinstance(val, (tuple, list)):
140+
fh = val[1]
141+
name = getattr(fh, "name", None)
142+
if name and os.path.exists(name):
143+
try:
144+
fh.close()
145+
except Exception:
146+
pass
147+
kwargs["files"][key] = (val[0], open(name, "rb")) + tuple(val[2:])
148+
else:
149+
name = getattr(val, "name", None)
150+
if name and os.path.exists(name):
151+
try:
152+
val.close()
153+
except Exception:
154+
pass
155+
kwargs["files"][key] = open(name, "rb")
142156
raise last_exc
143157

144158

tests/integration/framework/setup.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
All calls go through the SDK so the request-capture layer records them. Uses the
1212
SDK exactly as a customer would — exercising the real artifact under test.
1313
14-
Env vars (see .env.example):
14+
Env vars (set in tests/integration/.env):
1515
Required: EMAIL, PASSWORD, HOST, ORGANIZATION
1616
Optional: MFA_SECRET, DELETE_DYNAMIC_RESOURCES (default 'true')
1717
"""
@@ -47,7 +47,7 @@ def validate_environment() -> None:
4747
if missing:
4848
raise RuntimeError(
4949
f"Missing required environment variables: {', '.join(missing)}. "
50-
f"See tests/integration/.env.example."
50+
f"Create tests/integration/.env with these variables set."
5151
)
5252

5353

tests/unit/assets/test_assets_unit.py

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -261,49 +261,49 @@ def test_delete_folder(self):
261261

262262
def test_fetch_includes_scan_status_param(self):
263263
asset = self.client.stack(api_key).assets(asset_uid)
264-
asset.add_param("_asset_scan_status", True)
264+
asset.add_param("include_asset_scan_status", "true")
265265
response = asset.fetch()
266-
self.assertIn("_asset_scan_status=True", response.request.url)
266+
self.assertIn("include_asset_scan_status=true", response.request.url)
267267
self.assertEqual(response.request.method, "GET")
268268

269269
def test_find_includes_scan_status_param(self):
270270
asset = self.client.stack(api_key).assets()
271-
asset.add_param("_asset_scan_status", True)
271+
asset.add_param("include_asset_scan_status", "true")
272272
response = asset.find()
273-
self.assertIn("_asset_scan_status=True", response.request.url)
273+
self.assertIn("include_asset_scan_status=true", response.request.url)
274274
self.assertEqual(response.request.method, "GET")
275275

276276
def test_upload_includes_scan_status_param(self):
277277
file_path = "tests/resources/mock_assets/chaat.jpeg"
278278
asset = self.client.stack(api_key).assets()
279-
asset.add_param("_asset_scan_status", True)
279+
asset.add_param("include_asset_scan_status", "true")
280280
response = asset.upload(file_path)
281-
self.assertIn("_asset_scan_status=True", response.request.url)
281+
self.assertIn("include_asset_scan_status=true", response.request.url)
282282
self.assertEqual(response.request.method, "POST")
283283

284284
def test_fetch_without_scan_status_param_field_absent(self):
285285
response = self.client.stack(api_key).assets(asset_uid).fetch()
286-
self.assertNotIn("_asset_scan_status", response.request.url)
286+
self.assertNotIn("include_asset_scan_status", response.request.url)
287287
self.assertEqual(response.request.method, "GET")
288288

289289
def test_find_without_scan_status_param_field_absent(self):
290290
response = self.client.stack(api_key).assets().find()
291-
self.assertNotIn("_asset_scan_status", response.request.url)
291+
self.assertNotIn("include_asset_scan_status", response.request.url)
292292
self.assertEqual(response.request.method, "GET")
293293

294294
def test_upload_without_scan_status_param_field_absent(self):
295295
file_path = "tests/resources/mock_assets/chaat.jpeg"
296296
response = self.client.stack(api_key).assets().upload(file_path)
297-
self.assertNotIn("_asset_scan_status", response.request.url)
297+
self.assertNotIn("include_asset_scan_status", response.request.url)
298298
self.assertEqual(response.request.method, "POST")
299299

300300
def test_scan_status_param_coexists_with_other_params(self):
301301
asset = self.client.stack(api_key).assets(asset_uid)
302302
asset.add_param("locale", "en-us")
303-
asset.add_param("_asset_scan_status", True)
303+
asset.add_param("include_asset_scan_status", "true")
304304
response = asset.fetch()
305305
self.assertIn("locale=en-us", response.request.url)
306-
self.assertIn("_asset_scan_status=True", response.request.url)
306+
self.assertIn("include_asset_scan_status=true", response.request.url)
307307
self.assertEqual(response.request.method, "GET")
308308

309309
def test_publish_includes_api_version_header(self):

0 commit comments

Comments
 (0)