From 714d774a40b27301fe4af73b29246da6fdeba860 Mon Sep 17 00:00:00 2001 From: sam-hokipoki Date: Sat, 12 Sep 2026 15:42:46 +0800 Subject: [PATCH] Fix patching any cell that contains a date Patching a cell whose current value is a date fails outright, before any work is done. The before-snapshot feeds openpyxl's datetime into PatchValue, whose value union is str | int | float | None: 3 validation errors for PatchValue value.str Input should be a valid string input_value=datetime.datetime(2025, 1, 1, 0, 0) Any workbook with a date column is therefore partly unpatchable. Widen the union to a shared PatchScalar alias covering datetime/date/time/ timedelta, which is the full set openpyxl returns for date- and duration-formatted cells. Widening alone leaves the undo path corrupting data, though. Inverse ops are serialized to JSON, where a datetime survives only as an ISO string, and a string replays as a string -- so undoing an edit silently rewrites a date cell as text (data_type 'd' becomes 's'), keeping the number format so it still looks like a date in Excel. So add an explicit value_type hint ("auto" | "date") on set_value and set_value_if. The inverse-op builder sets it for date-like before-values, which makes undo lossless. It also lets a caller write a real date cell for the first time: JSON has no date literal, so until now set_value could only ever produce text there. Both model copies carry the change, since edit/internal.py does the work and edit/engine/openpyxl_engine.py re-validates the result into edit/models.py. Tests cover the crash, the JSON round-trip through undo, writing a date via value_type, and that "auto" still leaves an ISO-looking string alone. --- src/exstruct/edit/internal.py | 41 +++++++---- src/exstruct/edit/models.py | 18 +++-- src/exstruct/edit/op_schema.py | 10 ++- src/exstruct/edit/types.py | 32 ++++++++- tests/edit/test_datetime_cells.py | 116 ++++++++++++++++++++++++++++++ 5 files changed, 196 insertions(+), 21 deletions(-) create mode 100644 tests/edit/test_datetime_cells.py diff --git a/src/exstruct/edit/internal.py b/src/exstruct/edit/internal.py index a991fde..b8a7791 100644 --- a/src/exstruct/edit/internal.py +++ b/src/exstruct/edit/internal.py @@ -5,6 +5,7 @@ from collections.abc import Callable, Iterator from contextlib import contextmanager from copy import copy +from datetime import date, datetime, time from pathlib import Path import re from typing import Any, Protocol, cast, runtime_checkable @@ -40,9 +41,12 @@ PatchBackend, PatchEngine, PatchOpType, + PatchScalar, PatchStatus, PatchValueKind, + PatchValueType, VerticalAlignType, + coerce_patch_scalar, ) _ALLOWED_EXTENSIONS = {".xlsx", ".xlsm", ".xls"} @@ -165,7 +169,7 @@ class DesignSnapshot(BaseModel): class OpenpyxlCellProtocol(Protocol): """Protocol for openpyxl cell access used by patch runner.""" - value: str | int | float | None + value: PatchScalar data_type: str | None font: OpenpyxlFontProtocol fill: OpenpyxlFillProtocol @@ -523,18 +527,22 @@ class PatchOp(BaseModel): default=None, description="Base cell for formula translation in fill_formula (e.g. 'C2').", ) - expected: str | int | float | None = Field( + expected: PatchScalar = Field( default=None, description="Expected current value for conditional ops (set_value_if, set_formula_if). Operation is skipped if mismatch.", ) - value: str | int | float | None = Field( + value: PatchScalar = Field( default=None, description="Value to set. Use null to clear a cell. For set_value and set_value_if.", ) - values: list[list[str | int | float | None]] | None = Field( + values: list[list[PatchScalar]] | None = Field( default=None, description="2D list of values for set_range_values. Shape must match the range dimensions.", ) + value_type: PatchValueType = Field( + default="auto", + description="Interpretation hint for value. 'date' parses an ISO string into a real date/time cell instead of text (JSON has no date literal). For set_value and set_value_if.", + ) formula: str | None = Field( default=None, description="Formula string starting with '=' (e.g. '=SUM(A1:A10)'). For set_formula, set_formula_if, fill_formula.", @@ -818,6 +826,7 @@ def _validate_optional_positive_width(cls, value: float | None) -> float | None: @model_validator(mode="after") def _validate_op(self) -> PatchOp: + self.value = coerce_patch_scalar(self.value, self.value_type) validator = _validator_for_op(self.op) if validator is None: return self @@ -1523,7 +1532,7 @@ class PatchValue(BaseModel): """Normalized before/after value in patch diff.""" kind: PatchValueKind - value: str | int | float | None + value: PatchScalar class PatchDiffItem(BaseModel): @@ -2865,7 +2874,7 @@ def _apply_openpyxl_cell_op( def _set_cell_value( cell: OpenpyxlCellProtocol, - value: str | int | float | None, + value: PatchScalar, auto_formula: bool, *, op_name: str, @@ -3129,7 +3138,7 @@ def _build_merge_value_loss_warning( ) -def _has_non_empty_cell_value(value: str | int | float | None) -> bool: +def _has_non_empty_cell_value(value: PatchScalar) -> bool: """Return True when cell has a non-empty value.""" if value is None: return False @@ -3520,7 +3529,7 @@ def _translate_formula(formula: str, origin: str, target: str) -> str: return str(translated) -def _patch_value_to_primitive(value: PatchValue | None) -> str | int | float | None: +def _patch_value_to_primitive(value: PatchValue | None) -> PatchScalar: """Convert PatchValue into primitive value for condition checks.""" if value is None: return None @@ -3528,8 +3537,8 @@ def _patch_value_to_primitive(value: PatchValue | None) -> str | int | float | N def _values_equal_for_condition( - current: str | int | float | None, - expected: str | int | float | None, + current: PatchScalar, + expected: PatchScalar, ) -> bool: """Compare values for conditional update checks.""" return current == expected @@ -3552,7 +3561,15 @@ def _build_inverse_cell_op( cell=cell_ref, formula=str(before.value), ) - return PatchOp(op="set_value", sheet=op.sheet, cell=cell_ref, value=before.value) + return PatchOp( + op="set_value", + sheet=op.sheet, + cell=cell_ref, + value=before.value, + # Without the hint the ISO string this serializes to replays as text, + # silently downgrading a date cell on undo. + value_type="date" if isinstance(before.value, datetime | date | time) else "auto", + ) def _collect_formula_issues_openpyxl( @@ -4621,7 +4638,7 @@ def _apply_xlwings_cell_op( def _set_xlwings_cell_value( cell: XlwingsRangeProtocol, - value: str | int | float | None, + value: PatchScalar, auto_formula: bool, *, op_name: str, diff --git a/src/exstruct/edit/models.py b/src/exstruct/edit/models.py index 3267e76..572fbb3 100644 --- a/src/exstruct/edit/models.py +++ b/src/exstruct/edit/models.py @@ -24,9 +24,12 @@ PatchBackend, PatchEngine, PatchOpType, + PatchScalar, PatchStatus, PatchValueKind, + PatchValueType, VerticalAlignType, + coerce_patch_scalar, ) _A1_PATTERN = re.compile(r"^[A-Za-z]{1,3}[1-9][0-9]*$") @@ -121,7 +124,7 @@ class DesignSnapshot(BaseModel): class OpenpyxlCellProtocol(Protocol): """Protocol for openpyxl cell access used by patch runner.""" - value: str | int | float | None + value: PatchScalar data_type: str | None font: OpenpyxlFontProtocol fill: OpenpyxlFillProtocol @@ -422,18 +425,22 @@ class PatchOp(BaseModel): default=None, description="Base cell for formula translation in fill_formula (e.g. 'C2').", ) - expected: str | int | float | None = Field( + expected: PatchScalar = Field( default=None, description="Expected current value for conditional ops (set_value_if, set_formula_if). Operation is skipped if mismatch.", ) - value: str | int | float | None = Field( + value: PatchScalar = Field( default=None, description="Value to set. Use null to clear a cell. For set_value and set_value_if.", ) - values: list[list[str | int | float | None]] | None = Field( + values: list[list[PatchScalar]] | None = Field( default=None, description="2D list of values for set_range_values. Shape must match the range dimensions.", ) + value_type: PatchValueType = Field( + default="auto", + description="Interpretation hint for value. 'date' parses an ISO string into a real date/time cell instead of text (JSON has no date literal). For set_value and set_value_if.", + ) formula: str | None = Field( default=None, description="Formula string starting with '=' (e.g. '=SUM(A1:A10)'). For set_formula, set_formula_if, fill_formula.", @@ -717,6 +724,7 @@ def _validate_optional_positive_width(cls, value: float | None) -> float | None: @model_validator(mode="after") def _validate_op(self) -> PatchOp: + self.value = coerce_patch_scalar(self.value, self.value_type) validator = _validator_for_op(self.op) if validator is None: return self @@ -1422,7 +1430,7 @@ class PatchValue(BaseModel): """Normalized before/after value in patch diff.""" kind: PatchValueKind - value: str | int | float | None + value: PatchScalar class PatchDiffItem(BaseModel): diff --git a/src/exstruct/edit/op_schema.py b/src/exstruct/edit/op_schema.py index 25f771f..d079727 100644 --- a/src/exstruct/edit/op_schema.py +++ b/src/exstruct/edit/op_schema.py @@ -115,10 +115,11 @@ def schema_with_sheet_resolution_rules(schema: PatchOpSchema) -> PatchOpSchema: op="set_value", description="Set a scalar value to one cell.", required=["sheet", "cell", "value"], - optional=[], + optional=["value_type"], constraints=[ "cell target only", "use auto_formula=true to allow values starting with '='", + "value_type='date' parses an ISO value into a real date cell (default 'auto' writes it as text)", ], example={"op": "set_value", "sheet": "Sheet1", "cell": "A1", "value": "Hello"}, ), @@ -175,8 +176,11 @@ def schema_with_sheet_resolution_rules(schema: PatchOpSchema) -> PatchOpSchema: op="set_value_if", description="Set value when current value matches expected.", required=["sheet", "cell", "expected", "value"], - optional=[], - constraints=["no-op when expected mismatch"], + optional=["value_type"], + constraints=[ + "no-op when expected mismatch", + "value_type='date' parses an ISO value into a real date cell (default 'auto' writes it as text)", + ], example={ "op": "set_value_if", "sheet": "Sheet1", diff --git a/src/exstruct/edit/types.py b/src/exstruct/edit/types.py index 01fa818..e21e48d 100644 --- a/src/exstruct/edit/types.py +++ b/src/exstruct/edit/types.py @@ -1,9 +1,17 @@ -"""Literal type aliases used by the public workbook editing contract.""" +"""Type aliases and scalar coercion for the public workbook editing contract.""" from __future__ import annotations +from datetime import date, datetime, time, timedelta from typing import Literal +# Scalar cell payloads. datetime/date/time/timedelta are what openpyxl hands back +# for date- and duration-formatted cells; without them a patch that merely reads +# such a cell for its before-snapshot fails validation before doing any work. +PatchScalar = str | int | float | datetime | date | time | timedelta | None + +PatchValueType = Literal["auto", "date"] + PatchOpType = Literal[ "set_value", "set_formula", @@ -63,7 +71,29 @@ "PatchBackend", "PatchEngine", "PatchOpType", + "PatchScalar", "PatchStatus", "PatchValueKind", + "PatchValueType", "VerticalAlignType", + "coerce_patch_scalar", ] + + +def coerce_patch_scalar(value: PatchScalar, value_type: PatchValueType) -> PatchScalar: + """Apply an explicit ``value_type`` hint to a scalar payload. + + JSON has no date literal, so a datetime survives serialization only as an ISO + string -- and a string round-trips back as a string, silently downgrading a + date cell to text. ``value_type="date"`` is how a caller (and the inverse-op + builder) says "this really is a date". + """ + + if value_type != "date" or not isinstance(value, str): + return value + for parse in (datetime.fromisoformat, date.fromisoformat, time.fromisoformat): + try: + return parse(value) + except ValueError: + continue + raise ValueError(f"value_type='date' but {value!r} is not an ISO date/time.") diff --git a/tests/edit/test_datetime_cells.py b/tests/edit/test_datetime_cells.py new file mode 100644 index 0000000..5581c08 --- /dev/null +++ b/tests/edit/test_datetime_cells.py @@ -0,0 +1,116 @@ +"""Date-bearing cells must survive patch, inverse-op serialization, and undo.""" + +from __future__ import annotations + +from datetime import datetime +import json +from pathlib import Path + +from openpyxl import Workbook, load_workbook +import pytest + +from exstruct.edit import PatchOp, PatchRequest, patch_workbook +from exstruct.edit.types import coerce_patch_scalar + + +def _workbook_with_date(path: Path) -> None: + workbook = Workbook() + sheet = workbook.active + assert sheet is not None + sheet.title = "Sheet1" + sheet["A1"] = datetime(2025, 1, 1) + workbook.save(path) + workbook.close() + + +def test_patching_a_date_cell_captures_the_date_as_before(tmp_path: Path) -> None: + """A date in the target cell used to fail PatchValue validation outright.""" + source = tmp_path / "book.xlsx" + _workbook_with_date(source) + + result = patch_workbook( + PatchRequest( + xlsx_path=source, + ops=[PatchOp(op="set_value", sheet="Sheet1", cell="A1", value="replaced")], + backend="openpyxl", + dry_run=True, + return_inverse_ops=True, + ) + ) + + assert result.error is None + assert result.patch_diff[0].before is not None + assert result.patch_diff[0].before.value == datetime(2025, 1, 1) + + +def test_inverse_op_restores_a_real_date_through_json(tmp_path: Path) -> None: + """The undo script is written to JSON, so the date hint must survive the trip.""" + source = tmp_path / "book.xlsx" + patched = tmp_path / "patched.xlsx" + undone = tmp_path / "undone.xlsx" + _workbook_with_date(source) + + forward = patch_workbook( + PatchRequest( + xlsx_path=source, + output_path=patched, + ops=[PatchOp(op="set_value", sheet="Sheet1", cell="A1", value="replaced")], + backend="openpyxl", + return_inverse_ops=True, + ) + ) + assert forward.error is None + + # Round-trip the inverse ops the way the CLI does: dump to JSON, read back. + serialized = json.loads(json.dumps([op.model_dump(mode="json") for op in forward.inverse_ops])) + assert forward.out_path is not None + reverse = patch_workbook( + PatchRequest( + xlsx_path=Path(forward.out_path), + output_path=undone, + ops=[PatchOp.model_validate(op) for op in serialized], + backend="openpyxl", + ) + ) + assert reverse.error is None + + assert reverse.out_path is not None + cell = load_workbook(reverse.out_path)["Sheet1"]["A1"] + assert cell.value == datetime(2025, 1, 1) + assert cell.data_type == "d", "undo downgraded the date cell to text" + + +def test_value_type_date_writes_a_date_not_a_string(tmp_path: Path) -> None: + source = tmp_path / "book.xlsx" + out = tmp_path / "out.xlsx" + _workbook_with_date(source) + + result = patch_workbook( + PatchRequest( + xlsx_path=source, + output_path=out, + ops=[ + PatchOp( + op="set_value", + sheet="Sheet1", + cell="A1", + value="2026-12-25T00:00:00", + value_type="date", + ) + ], + backend="openpyxl", + ) + ) + + assert result.error is None + assert result.out_path is not None + assert load_workbook(result.out_path)["Sheet1"]["A1"].value == datetime(2026, 12, 25) + + +def test_value_type_auto_leaves_an_iso_string_alone() -> None: + assert coerce_patch_scalar("2025-01-01", "auto") == "2025-01-01" + + +def test_value_type_date_rejects_non_iso_text() -> None: + with pytest.raises(ValueError, match="not an ISO date/time"): + coerce_patch_scalar("hello", "date")