Address review feedback on DOWNLOAD_CHUNK_SIZE_MB

- Validate the env var: max(1, int(...)) inside try/except ValueError, so
  TSC_DOWNLOAD_CHUNK_SIZE_MB=0 or non-numeric doesn't silently corrupt output
- Document that very large values (~thousands of MB) will OOM on constrained hosts
- Correct CHANGELOG: PDF/image are NOT unified; only CSV/Excel and file-backed downloads

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: jacalata

CHANGELOG.md

@@ -5,18 +5,21 @@
   hierarchy path (e.g. `"Marketing/Q1 Reports"`). The walk is performed level by
   level using the REST API name filter, so a path with *n* components issues *n*
   requests. Returns the matching `ProjectItem` or `None` if no project is found.
-* Unified streaming download chunk size across `views.populate_csv` /
-  `_pdf` / `_excel`, `custom_views.*`, `workbooks.download`,
-  `datasources.download`, and `flows.download`. Previously the mixed 1024-byte
-  and 10240-byte chunks caused multi-second latency for large view exports.
-  Downloads now use a dedicated `DOWNLOAD_CHUNK_SIZE_MB` config value
-  (default 1 MB, overridable via the `TSC_DOWNLOAD_CHUNK_SIZE_MB` env var).
-  Upload / chunked-publish continues to use `CHUNK_SIZE_MB` (default 50 MB,
-  overridable via `TSC_CHUNK_SIZE_MB`) -- they are separate knobs because a
-  large read chunk delays first-byte yield on slow connections while a large
-  write chunk reduces per-request overhead. Behavior note: callers that
-  previously streamed 1 KB at a time will now hold up to 1 MB resident per
-  chunk; memory-constrained callers can drop this via the env var.
+* Unified streaming download chunk size for the file-backed download paths --
+  `views.populate_csv` and `views.populate_excel`, the CSV/Excel branches of
+  `custom_views.*`, and `workbooks.download` / `datasources.download` /
+  `flows.download`. Previously the mixed 1024-byte and 10240-byte chunks caused
+  multi-second latency for large view exports. These paths now use a dedicated
+  `DOWNLOAD_CHUNK_SIZE_MB` config value (default 1 MB, overridable via the
+  `TSC_DOWNLOAD_CHUNK_SIZE_MB` env var). Upload / chunked-publish continues to
+  use `CHUNK_SIZE_MB` (default 50 MB, overridable via `TSC_CHUNK_SIZE_MB`) --
+  they are separate knobs because a large read chunk delays first-byte yield on
+  slow connections while a large write chunk reduces per-request overhead.
+  Behavior note: callers that previously streamed 1 KB at a time will now hold
+  up to 1 MB resident per chunk; memory-constrained callers can drop this via
+  the env var. Follow-up: `views.populate_pdf` and `views.populate_image` still
+  buffer the full response in memory via `server_response.content` and are not
+  covered by this change.
 * Preserve HTTP method and body across 3xx redirects. Previously `requests`
   followed 301/302/303 by converting POST to GET and dropping the body, so
   endpoints like `users.add`, `workbooks.publish`, and any write hitting a

tableauserverclient/config.py

@@ -19,17 +19,32 @@ def FILESIZE_LIMIT_MB(self):
     def CHUNK_SIZE_MB(self):
         return int(os.getenv("TSC_CHUNK_SIZE_MB", 5 * 10))  # 5MB felt too slow, upped it to 50
 
-    # Chunk size for streaming *downloads* (view CSV / Excel / PDF, workbook /
+    # Chunk size for streaming *downloads* (view CSV / Excel, workbook /
     # datasource / flow downloads). Kept separate from the upload knob because
     # a large read chunk delays the first-byte yield on slow connections --
     # requests.iter_content buffers up to chunk_size before yielding, so on a
     # 1 Mbps link a 50 MB chunk means ~7 minutes before the first yield.
     # 1 MB is empirically a reasonable balance between per-chunk overhead and
     # progressive-yield latency; callers who want a different tradeoff can
     # tune via TSC_DOWNLOAD_CHUNK_SIZE_MB.
+    #
+    # No upper bound is enforced, but very large values (thousands of MB) will
+    # OOM on constrained hosts because each chunk is buffered in memory before
+    # it is written or yielded. Keep this under ~100 MB unless the caller has
+    # specifically measured a benefit at higher values.
+    #
+    # Bounds: values <= 0 or non-numeric input are treated as invalid and fall
+    # back to the 1 MB default; 0 or a negative chunk size would silently
+    # corrupt the output because iter_content interprets it as "read all".
     @property
-    def DOWNLOAD_CHUNK_SIZE_MB(self):
-        return int(os.getenv("TSC_DOWNLOAD_CHUNK_SIZE_MB", 1))
+    def DOWNLOAD_CHUNK_SIZE_MB(self) -> int:
+        raw = os.getenv("TSC_DOWNLOAD_CHUNK_SIZE_MB", "1")
+        try:
+            value = int(raw)
+        except ValueError:
+            # invalid env value; fall back to default
+            value = 1
+        return max(1, value)
 
     # Default page size
     @property

test/test_view.py

@@ -340,7 +340,10 @@ def test_stream_content_uses_configured_chunk_size(server: TSC.Server, monkeypat
     def spy_iter_content(self, chunk_size=None, decode_unicode=False):
         captured.append(chunk_size)
         assert real_iter_content is not None
-        return real_iter_content(self, chunk_size, decode_unicode)
+        # Pass chunk_size / decode_unicode by name so a signature reorder in
+        # requests fails loudly here instead of silently binding to the wrong
+        # parameter.
+        return real_iter_content(self, chunk_size=chunk_size, decode_unicode=decode_unicode)
 
     import requests
 
@@ -360,6 +363,24 @@ def spy_iter_content(self, chunk_size=None, decode_unicode=False):
     assert captured[0] == config.DOWNLOAD_CHUNK_SIZE_MB * BYTES_PER_MB
 
 
+def test_download_chunk_size_clamps_zero(monkeypatch) -> None:
+    # TSC_DOWNLOAD_CHUNK_SIZE_MB=0 would cause requests.iter_content to read the
+    # entire response as a single chunk (defeating the streaming behavior); make
+    # sure the config clamps to the 1 MB default instead.
+    from tableauserverclient.config import config
+
+    monkeypatch.setenv("TSC_DOWNLOAD_CHUNK_SIZE_MB", "0")
+    assert config.DOWNLOAD_CHUNK_SIZE_MB == 1
+
+
+def test_download_chunk_size_rejects_non_numeric(monkeypatch) -> None:
+    # Non-numeric env values must not crash the download path; fall back to 1 MB.
+    from tableauserverclient.config import config
+
+    monkeypatch.setenv("TSC_DOWNLOAD_CHUNK_SIZE_MB", "abc")
+    assert config.DOWNLOAD_CHUNK_SIZE_MB == 1
+
+
 def test_populate_image_missing_id(server: TSC.Server) -> None:
     single_view = TSC.ViewItem()
     single_view._id = None

test/test_workbook.py

@@ -351,7 +351,10 @@ def test_download_uses_configured_chunk_size(server: TSC.Server, tmp_path: Path,
     def spy_iter_content(self, chunk_size=None, decode_unicode=False):
         captured.append(chunk_size)
         assert real_iter_content is not None
-        return real_iter_content(self, chunk_size, decode_unicode)
+        # Pass chunk_size / decode_unicode by name so a signature reorder in
+        # requests fails loudly here instead of silently binding to the wrong
+        # parameter.
+        return real_iter_content(self, chunk_size=chunk_size, decode_unicode=decode_unicode)
 
     import requests