From 4b5ec09ae236aa041181c007a0b9028930ae2139 Mon Sep 17 00:00:00 2001 From: peter Date: Wed, 2 Sep 2026 16:29:39 +0100 Subject: [PATCH] Convert uploaded .xlsx files to csv artifacts server-side Uploading a .xlsx file no longer becomes a generic downloadable binary - it's read server-side with openpyxl and converted to a csv artifact, rendering in the same interactive grid as a native CSV post. Detected by content type or .xlsx extension (some clients send generic/incorrect content types for multipart uploads). Only the active sheet converts; formula cells use whatever cached result the spreadsheet software last saved (openpyxl doesn't evaluate formulas itself). Corrupt/unreadable files return 400. Since the existing posting script already forwards any non-text file via multipart with no special-casing, `daybook_post.py balance_sheet.xlsx` already works with zero script changes - verified live. Two real bugs found and fixed while building this, both now covered by regression tests: - {**a_querydict, ...} silently wrapped every value (including title) in a list - Python's ** spread on a dict subclass bypasses QueryDict's __getitem__ override and exposes its internal one-list-per-key storage directly. Fixed by building the delegated dict explicitly via .get(). - Pre-setting None for absent optional fields (e.g. source_prompt) defeated create_text_artifact's own data.get(field, default) calls, since .get() only falls through to its default when the key is *missing*, not when the value is None - caused a NOT NULL constraint failure. Fixed by only setting keys that were actually provided. - csv.writer's default "\r\n" terminator was getting doubled by Path.write_text()'s own newline translation on Windows, producing a blank line between every row. Fixed with lineterminator="\n". Co-Authored-By: Claude Sonnet 5 --- artifacts/views.py | 64 ++++++++++++++++++++++++-- docs/api.md | 26 ++++++++++- requirements.txt | 1 + skills/daybook-posting/SKILL.md | 13 ++++++ tests/api/test_artifacts.py | 74 ++++++++++++++++++++++++++++++ tests/unit/test_xlsx_conversion.py | 60 ++++++++++++++++++++++++ 6 files changed, 232 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_xlsx_conversion.py diff --git a/artifacts/views.py b/artifacts/views.py index 72aa35b..e7650f9 100644 --- a/artifacts/views.py +++ b/artifacts/views.py @@ -6,12 +6,12 @@ import json import mimetypes import re import secrets -import time from datetime import timedelta from pathlib import Path from urllib.parse import urlencode import markdown +import openpyxl from django.conf import settings from django.contrib import messages from django.contrib.auth import views as auth_views @@ -107,7 +107,7 @@ class ArtifactListCreateView(APIView): return HttpResponseForbidden("Invalid or missing API token") if request.content_type and request.content_type.startswith("multipart/"): - artifact = create_binary_artifact(request.data, user) + artifact = create_binary_artifact(request.data, request, user) if isinstance(artifact, Response): return artifact apply_tag_colors(request.data) @@ -616,7 +616,36 @@ def create_text_artifact(data, request, user): return artifact -def create_binary_artifact(data, user): +XLSX_CONTENT_TYPE = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" + + +def is_xlsx_upload(uploaded, content_type): + if content_type == XLSX_CONTENT_TYPE: + return True + return Path(uploaded.name).suffix.lower() == ".xlsx" + + +def xlsx_to_csv_text(uploaded): + """Converts the active sheet of an uploaded .xlsx workbook to CSV text. + + Only the active sheet is used - other sheets in a multi-sheet workbook + are silently dropped. data_only=True reads cached formula *results* + rather than formula source, since a results grid is what's useful here. + """ + workbook = openpyxl.load_workbook(io.BytesIO(uploaded.read()), data_only=True, read_only=True) + sheet = workbook.active + buffer = io.StringIO() + # lineterminator="\n": csv.writer defaults to "\r\n", which then gets a + # second newline translation from Path.write_text()'s own text-mode + # handling when this string is saved to disk, producing a blank line + # between every row on Windows. + writer = csv.writer(buffer, lineterminator="\n") + for row in sheet.iter_rows(values_only=True): + writer.writerow(["" if cell is None else cell for cell in row]) + return buffer.getvalue() + + +def create_binary_artifact(data, request, user): uploaded = data.get("file") required = ["source", "title"] missing = [field for field in required if not data.get(field)] @@ -628,11 +657,36 @@ def create_binary_artifact(data, user): if source_error: return source_error + title = data["title"] + content_type = getattr(uploaded, "content_type", "") or mimetypes.guess_type(uploaded.name)[0] or "" + + if is_xlsx_upload(uploaded, content_type): + try: + content = xlsx_to_csv_text(uploaded) + except Exception as exc: + return Response({"detail": f"Could not read .xlsx file: {exc}"}, status=400) + # Built as a plain dict via .get() rather than {**data, ...}: `data` is + # a QueryDict for multipart uploads, and spreading a QueryDict with ** + # bypasses its __getitem__ override and exposes its internal + # one-value-per-list storage directly, silently wrapping every value + # (title included) in a list. + text_data = {"source": data.get("source"), "title": data.get("title")} + for optional_field in ( + "tags", "source_type", "source_name", "source_id", "source_prompt", "metadata", + ): + # Only set if actually provided - create_text_artifact's own + # data.get(field, default) calls need the key absent, not None, + # to fall through to their intended defaults. + value = data.get(optional_field) + if value is not None: + text_data[optional_field] = value + text_data["kind"] = Artifact.CSV + text_data["content"] = content + return create_text_artifact(text_data, request, user) + now = timezone.localtime() artifact_id = generate_id() source = slugify(data["source"]) or "unknown" - title = data["title"] - content_type = getattr(uploaded, "content_type", "") or mimetypes.guess_type(uploaded.name)[0] or "" kind = binary_kind(content_type) directory = Path(source) / now.date().isoformat() diff --git a/docs/api.md b/docs/api.md index 57d5d21..e0b0036 100644 --- a/docs/api.md +++ b/docs/api.md @@ -226,14 +226,38 @@ Required fields: | `title` | Required. | | `file` | Required file part. | -File kind is inferred from content type: +File kind is inferred from content type (or file extension for `.xlsx`): | Content type | Artifact kind | |---|---| | `image/*` | `image` | | `application/pdf` | `pdf` | +| `.xlsx` (by content type or extension) | `csv` — converted server-side, see below | | Anything else | `binary` | +### `.xlsx` Upload → `csv` Kind + +Uploading a `.xlsx` file doesn't create a `binary` artifact — it's read +server-side (`openpyxl`) and converted into a `csv` artifact, so it renders +in the same interactive grid view as a native CSV post. Known limitations: + +- Only the workbook's **active sheet** is converted; other sheets in a + multi-sheet workbook are silently dropped. +- Formula cells use the **cached result** the spreadsheet application last + calculated and saved, not a live re-evaluation — a workbook that's never + actually been opened/calculated in real spreadsheet software (e.g. one + built by a script that never triggered calculation) will show those cells + blank. +- Legacy `.xls` (pre-2007 binary format) is **not** supported, only modern + `.xlsx`. +- Rich spreadsheet formatting (bold, colors, merged cells, multi-level + headers) is not preserved — like any `csv` artifact, it renders as a + plain sortable/searchable table. If preserving that formatting matters + more than interactivity, post as `html_fragment` with a hand-built + `` instead. + +A corrupt or unreadable `.xlsx` file returns `400`. + PowerShell/curl example: ```powershell diff --git a/requirements.txt b/requirements.txt index 9da4882..367b364 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,5 +2,6 @@ Django>=5.0,<6.0 djangorestframework>=3.15,<4.0 Markdown>=3.6,<4.0 Pygments>=2.17,<3.0 +openpyxl>=3.1,<4.0 gunicorn>=23.0,<24.0 whitenoise>=6.7,<7.0 diff --git a/skills/daybook-posting/SKILL.md b/skills/daybook-posting/SKILL.md index 17f6b60..579944a 100644 --- a/skills/daybook-posting/SKILL.md +++ b/skills/daybook-posting/SKILL.md @@ -120,6 +120,19 @@ python skills/daybook-posting/scripts/daybook_post.py results.csv python skills/daybook-posting/scripts/daybook_post.py --kind json --title "Run Summary" --content-file summary.json ``` +`.xlsx` spreadsheets work too, with no special flag needed — just `--file` +(or post it positionally, same as any other file): + +```bash +python skills/daybook-posting/scripts/daybook_post.py balance_sheet.xlsx +``` + +The server converts it to a `csv` artifact automatically (active sheet +only; see `docs/api.md` for the full list of conversion limitations — +notably, formulas need to have been calculated by real spreadsheet software +that saved the file, and rich formatting like bold/colors/merged cells +isn't preserved). + For other code that should render today, use `--file path --as-markdown` to post it as a Markdown fenced code block. Without `--as-markdown`, it uploads as a downloadable binary file. diff --git a/tests/api/test_artifacts.py b/tests/api/test_artifacts.py index bbd634e..0608707 100644 --- a/tests/api/test_artifacts.py +++ b/tests/api/test_artifacts.py @@ -1,8 +1,10 @@ import json import shutil import tempfile +from io import BytesIO from pathlib import Path +import openpyxl from django.contrib.auth import get_user_model from django.test import override_settings from django.urls import reverse @@ -455,3 +457,75 @@ class ArtifactAPITestCase(APITestCase): artifact = Artifact.objects.get(pk=artifact_id) self.assertEqual(artifact.source_name, "test-agent") # unchanged, not mangled + + # -- xlsx upload converts to csv kind ------------------------------------ + + def make_xlsx(self, rows, filename="sheet.xlsx"): + workbook = openpyxl.Workbook() + sheet = workbook.active + for row in rows: + sheet.append(row) + buffer = BytesIO() + workbook.save(buffer) + buffer.seek(0) + buffer.name = filename + return buffer + + def test_xlsx_upload_converts_to_csv_kind(self): + xlsx = self.make_xlsx([["Item", "Q1", "Q2"], ["Cash", 1000, 1200]]) + response = self.client.post( + reverse("artifact-api"), + {"source": "test-agent", "title": "Balance Sheet", "file": xlsx}, + format="multipart", + **self.auth_headers(self.token), + ) + self.assertEqual(response.status_code, 201) + self.assertEqual(response.data["kind"], "csv") + + artifact = Artifact.objects.get(pk=response.data["id"]) + raw_path = (self.artifacts_root / artifact.path).with_suffix(".csv") + # read_text() normalizes line endings, so this is OS-portable rather + # than asserting the exact on-disk bytes (which are "\r\n" on Windows). + self.assertEqual(raw_path.read_text(encoding="utf-8"), "Item,Q1,Q2\nCash,1000,1200\n") + + stored_html = (self.artifacts_root / artifact.path).read_text(encoding="utf-8") + self.assertIn('"columns": ["Item", "Q1", "Q2"]', stored_html) + self.assertIn('["Cash", "1000", "1200"]', stored_html) + + def test_xlsx_upload_does_not_mangle_fields(self): + """Regression test: {**a_querydict} previously wrapped every value + (including title) in a list - QueryDict's __getitem__ override is + bypassed by the ** spread, exposing its internal list-per-key + storage directly. Separately, pre-setting None for absent optional + fields defeated create_text_artifact's own .get(field, default) + calls (.get only falls through when the key is missing, not None), + which previously caused a NOT NULL constraint failure on + source_prompt.""" + xlsx = self.make_xlsx([["a", "b"]]) + response = self.client.post( + reverse("artifact-api"), + { + "source": "test-agent", + "title": "Untitled Sheet", + "tags": "finance,q2", + "file": xlsx, + }, + format="multipart", + **self.auth_headers(self.token), + ) + self.assertEqual(response.status_code, 201) + self.assertEqual(response.data["title"], "Untitled Sheet") # not ["Untitled Sheet"] + self.assertEqual(response.data["source_prompt"], "") # defaulted, not None + self.assertEqual([t["name"] for t in response.data["tags"]], ["finance", "q2"]) + + def test_xlsx_upload_rejects_corrupt_file(self): + buffer = BytesIO(b"not a real xlsx file") + buffer.name = "broken.xlsx" + response = self.client.post( + reverse("artifact-api"), + {"source": "test-agent", "title": "Broken", "file": buffer}, + format="multipart", + **self.auth_headers(self.token), + ) + self.assertEqual(response.status_code, 400) + diff --git a/tests/unit/test_xlsx_conversion.py b/tests/unit/test_xlsx_conversion.py new file mode 100644 index 0000000..e31687d --- /dev/null +++ b/tests/unit/test_xlsx_conversion.py @@ -0,0 +1,60 @@ +from io import BytesIO +from types import SimpleNamespace + +import openpyxl +from django.test import SimpleTestCase + +from artifacts.views import XLSX_CONTENT_TYPE, is_xlsx_upload, xlsx_to_csv_text + + +class IsXlsxUploadTestCase(SimpleTestCase): + def test_matches_exact_content_type(self): + upload = SimpleNamespace(name="whatever.bin") + self.assertTrue(is_xlsx_upload(upload, XLSX_CONTENT_TYPE)) + + def test_matches_by_extension_regardless_of_content_type(self): + upload = SimpleNamespace(name="sheet.xlsx") + self.assertTrue(is_xlsx_upload(upload, "application/octet-stream")) + self.assertTrue(is_xlsx_upload(upload, "")) + + def test_extension_match_is_case_insensitive(self): + upload = SimpleNamespace(name="SHEET.XLSX") + self.assertTrue(is_xlsx_upload(upload, "")) + + def test_rejects_other_files(self): + upload = SimpleNamespace(name="report.pdf") + self.assertFalse(is_xlsx_upload(upload, "application/pdf")) + + +class XlsxToCsvTextTestCase(SimpleTestCase): + def make_xlsx(self, rows): + workbook = openpyxl.Workbook() + sheet = workbook.active + for row in rows: + sheet.append(row) + buffer = BytesIO() + workbook.save(buffer) + buffer.seek(0) + return buffer + + def test_converts_active_sheet_to_csv(self): + upload = self.make_xlsx([["Item", "Q1", "Q2"], ["Cash", 1000, 1200]]) + content = xlsx_to_csv_text(upload) + self.assertEqual(content, "Item,Q1,Q2\nCash,1000,1200\n") + + def test_uses_single_line_terminator_not_doubled(self): + """Regression test: csv.writer's default "\\r\\n" gets a second + newline translation from Path.write_text()'s text-mode handling when + this string is later saved to disk, producing a blank line between + every row on Windows - lineterminator="\\n" avoids embedding "\\r\\n" + in the string in the first place, leaving the OS-level text-mode + write as the only place "\\n" becomes "\\r\\n".""" + upload = self.make_xlsx([["a"], ["b"], ["c"]]) + content = xlsx_to_csv_text(upload) + self.assertNotIn("\r", content) + self.assertEqual(content.count("\n"), 3) + + def test_empty_cells_become_empty_strings(self): + upload = self.make_xlsx([["a", "b"], ["x", None]]) + content = xlsx_to_csv_text(upload) + self.assertEqual(content, "a,b\nx,\n")