From 11315592ed739cece25ff36edcf04cdf3ed497e6 Mon Sep 17 00:00:00 2001 From: Radith Samarakoon Date: Thu, 4 Jun 2026 01:58:28 +0530 Subject: [PATCH 1/2] fix: restore currency_id int->str coercion lost in model regen The Firefly III API (Issue #43) returns currency_id as an int in spent/pc_spent (ArrayEntryWithCurrencyAndSum) arrays, while the OpenAPI spec types it as a string. A BeforeValidator coercion was added in 346ce63 but wiped out when models were regenerated in 55cdb0b, causing budget endpoints (e.g. list_budget_limits) to fail with: data.0.attributes.pc_spent.0.currency_id Input should be a valid string [type=string_type, input_value=25] This restores the BeforeValidator on ArrayEntryWithCurrencyAndSum and adds an apply_manual_fixes() step to scripts/update_schema.py so the patch is re-injected idempotently after every regeneration, preventing the fix from being lost again. Co-Authored-By: Claude Opus 4.8 --- src/lampyrid/models/firefly_models.py | 23 ++++++-- src/lampyrid/scripts/update_schema.py | 77 +++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 3 deletions(-) diff --git a/src/lampyrid/models/firefly_models.py b/src/lampyrid/models/firefly_models.py index 2d69e3f..154ee3e 100644 --- a/src/lampyrid/models/firefly_models.py +++ b/src/lampyrid/models/firefly_models.py @@ -1,14 +1,28 @@ # generated by datamodel-codegen: # filename: firefly-iii-6.5.5-v1.yaml # timestamp: 2026-03-24T13:57:48+00:00 +# +# NOTE: Manual modifications have been made to this file to work around +# Firefly III API bugs. See comments marked with "MANUAL FIX" below. +# These fixes are re-applied automatically after regeneration by +# scripts/update_schema.py (apply_manual_fixes). from __future__ import annotations from datetime import date as date_aliased from enum import Enum -from typing import Any +from typing import Annotated, Any -from pydantic import AnyUrl, AwareDatetime, BaseModel, EmailStr, Field, RootModel +from pydantic import AnyUrl, AwareDatetime, BaseModel, BeforeValidator, EmailStr, Field, RootModel + + +# MANUAL FIX: Firefly III API bug (Issue #43) returns currency_id as int instead of str +# in spent/pc_spent and other ArrayEntryWithCurrencyAndSum arrays. This validator coerces +# int to str so model validation does not fail. +def _coerce_to_str(v: Any) -> str | None: + if v is None: + return None + return str(v) class AutocompleteAccount(BaseModel): @@ -521,7 +535,10 @@ class InsightTransferEntry(BaseModel): class ArrayEntryWithCurrencyAndSum(BaseModel): - currency_id: str | None = Field(None, examples=['5']) + # MANUAL FIX: Use Annotated with BeforeValidator to coerce int to str (Issue #43) + currency_id: Annotated[str | None, BeforeValidator(_coerce_to_str)] = Field( + None, examples=['5'] + ) currency_code: str | None = Field(None, examples=['USD']) currency_symbol: str | None = Field(None, examples=['$']) currency_decimal_places: int | None = Field( diff --git a/src/lampyrid/scripts/update_schema.py b/src/lampyrid/scripts/update_schema.py index b10e36f..a07da20 100644 --- a/src/lampyrid/scripts/update_schema.py +++ b/src/lampyrid/scripts/update_schema.py @@ -180,6 +180,76 @@ def regenerate_models() -> bool: return False +def apply_manual_fixes() -> bool: + """Re-apply manual patches to the freshly generated models file. + + datamodel-codegen overwrites firefly_models.py from the OpenAPI spec, which + drops any hand-written workarounds for known Firefly III API bugs. This + re-injects them idempotently so they survive every regeneration. + + Currently applied fixes: + - Issue #43: the API returns ``currency_id`` as an int in spent/pc_spent + (ArrayEntryWithCurrencyAndSum) arrays, but the spec types it as a string. + A ``BeforeValidator`` coerces int -> str so validation does not fail. + + Returns: + True if the file was modified, False if fixes were already present. + + """ + models_path = PROJECT_ROOT / MODELS_OUTPUT + if not models_path.exists(): + print(f' Warning: {MODELS_OUTPUT} not found, skipping manual fixes') + return False + + content = models_path.read_text() + + # Idempotency guard: if the marker is already present, do nothing. + if '_coerce_to_str' in content: + return False + + # 1. Add the imports needed by the validator. + content = content.replace( + 'from typing import Any\n', + 'from typing import Annotated, Any\n', + 1, + ) + content = content.replace( + 'from pydantic import AnyUrl, AwareDatetime, BaseModel, EmailStr, Field, RootModel\n', + 'from pydantic import (\n' + ' AnyUrl,\n' + ' AwareDatetime,\n' + ' BaseModel,\n' + ' BeforeValidator,\n' + ' EmailStr,\n' + ' Field,\n' + ' RootModel,\n' + ')\n' + '\n\n' + '# MANUAL FIX: Firefly III API bug (Issue #43) returns currency_id as int instead of\n' + '# str in spent/pc_spent (ArrayEntryWithCurrencyAndSum) arrays. Coerce int to str.\n' + 'def _coerce_to_str(v):\n' + ' if v is None:\n' + ' return None\n' + ' return str(v)\n', + 1, + ) + + # 2. Wrap ArrayEntryWithCurrencyAndSum.currency_id with the coercing validator. + content = content.replace( + 'class ArrayEntryWithCurrencyAndSum(BaseModel):\n' + " currency_id: str | None = Field(None, examples=['5'])\n", + 'class ArrayEntryWithCurrencyAndSum(BaseModel):\n' + ' # MANUAL FIX: coerce int to str (Issue #43)\n' + ' currency_id: Annotated[str | None, BeforeValidator(_coerce_to_str)] = Field(\n' + " None, examples=['5']\n" + ' )\n', + 1, + ) + + models_path.write_text(content) + return True + + def cleanup_old_schema(old_version: str | None, new_version: str) -> bool: """Remove the old schema file if it exists and differs from new. @@ -265,6 +335,13 @@ def main() -> int: if regenerate_models(): print(f'Models regenerated at {MODELS_OUTPUT}') + # Re-apply manual workarounds for known Firefly III API bugs + print('Applying manual fixes...') + if apply_manual_fixes(): + print('Manual fixes applied') + else: + print('Manual fixes already present (or models file missing)') + # Format the generated code print('Formatting generated code...') try: From f38e551529fbfd7f203f1fca11fdbfa9c8bd50a8 Mon Sep 17 00:00:00 2001 From: Radith Samarakoon Date: Thu, 4 Jun 2026 02:06:54 +0530 Subject: [PATCH 2/2] fix: address CodeRabbit review on manual model fixes - apply_manual_fixes() now fails loudly: raises RuntimeError if any patch anchor is missing (datamodel-codegen output drift) and FileNotFoundError if the models file is absent, instead of silently reporting success and reintroducing the BudgetArray/pc_spent validation failure. - Move the apply_manual_fixes() call into regenerate_models() so every caller of that helper (e.g. tests, direct invocations) gets a patched models file, not just the CLI main() path. - Add unit tests covering patch application, idempotency, missing-anchor and missing-file failures, and regenerate_models integration. Co-Authored-By: Claude Opus 4.8 --- src/lampyrid/scripts/update_schema.py | 154 +++++++++++++++----------- tests/unit/test_scripts.py | 78 +++++++++++++ 2 files changed, 165 insertions(+), 67 deletions(-) diff --git a/src/lampyrid/scripts/update_schema.py b/src/lampyrid/scripts/update_schema.py index a07da20..cee3ef4 100644 --- a/src/lampyrid/scripts/update_schema.py +++ b/src/lampyrid/scripts/update_schema.py @@ -159,61 +159,18 @@ def update_pyproject_toml(old_version: str | None, new_version: str) -> bool: return False -def regenerate_models() -> bool: - """Run datamodel-codegen to regenerate Pydantic models. - - Returns: - True if successful, False otherwise - - """ - try: - subprocess.run( - ['uv', 'run', 'datamodel-codegen'], - cwd=PROJECT_ROOT, - capture_output=True, - text=True, - check=True, - ) - return True - except subprocess.CalledProcessError as e: - print(f' Error running datamodel-codegen: {e.stderr}') - return False - - -def apply_manual_fixes() -> bool: - """Re-apply manual patches to the freshly generated models file. - - datamodel-codegen overwrites firefly_models.py from the OpenAPI spec, which - drops any hand-written workarounds for known Firefly III API bugs. This - re-injects them idempotently so they survive every regeneration. - - Currently applied fixes: - - Issue #43: the API returns ``currency_id`` as an int in spent/pc_spent - (ArrayEntryWithCurrencyAndSum) arrays, but the spec types it as a string. - A ``BeforeValidator`` coerces int -> str so validation does not fail. - - Returns: - True if the file was modified, False if fixes were already present. - - """ - models_path = PROJECT_ROOT / MODELS_OUTPUT - if not models_path.exists(): - print(f' Warning: {MODELS_OUTPUT} not found, skipping manual fixes') - return False - - content = models_path.read_text() - - # Idempotency guard: if the marker is already present, do nothing. - if '_coerce_to_str' in content: - return False - - # 1. Add the imports needed by the validator. - content = content.replace( +# Manual patches re-applied to the generated models file after every regeneration. +# Each entry is (anchor, replacement): ``anchor`` is the exact generated snippet that +# must be present, ``replacement`` is what it becomes. If an anchor is missing, +# datamodel-codegen's output has drifted and apply_manual_fixes() fails loudly rather +# than silently dropping a workaround for a known Firefly III API bug. +_MANUAL_FIXES: list[tuple[str, str]] = [ + # Issue #43: import the symbols the coercion validator needs and define it. + ( 'from typing import Any\n', 'from typing import Annotated, Any\n', - 1, - ) - content = content.replace( + ), + ( 'from pydantic import AnyUrl, AwareDatetime, BaseModel, EmailStr, Field, RootModel\n', 'from pydantic import (\n' ' AnyUrl,\n' @@ -231,11 +188,9 @@ def apply_manual_fixes() -> bool: ' if v is None:\n' ' return None\n' ' return str(v)\n', - 1, - ) - - # 2. Wrap ArrayEntryWithCurrencyAndSum.currency_id with the coercing validator. - content = content.replace( + ), + # Issue #43: wrap ArrayEntryWithCurrencyAndSum.currency_id with the coercing validator. + ( 'class ArrayEntryWithCurrencyAndSum(BaseModel):\n' " currency_id: str | None = Field(None, examples=['5'])\n", 'class ArrayEntryWithCurrencyAndSum(BaseModel):\n' @@ -243,13 +198,85 @@ def apply_manual_fixes() -> bool: ' currency_id: Annotated[str | None, BeforeValidator(_coerce_to_str)] = Field(\n' " None, examples=['5']\n" ' )\n', - 1, - ) + ), +] + +# Marker proving the patches are present, used as the idempotency guard. +_MANUAL_FIX_MARKER = '_coerce_to_str' + + +def apply_manual_fixes() -> bool: + """Re-apply manual patches to the freshly generated models file. + + datamodel-codegen overwrites firefly_models.py from the OpenAPI spec, which + drops any hand-written workarounds for known Firefly III API bugs. This + re-injects them idempotently so they survive every regeneration. + + Currently applied fixes: + - Issue #43: the API returns ``currency_id`` as an int in spent/pc_spent + (ArrayEntryWithCurrencyAndSum) arrays, but the spec types it as a string. + A ``BeforeValidator`` coerces int -> str so validation does not fail. + + Returns: + True if the file was patched, False if fixes were already present. + + Raises: + FileNotFoundError: if the generated models file does not exist. + RuntimeError: if a patch anchor is missing, meaning datamodel-codegen's + output drifted and the workaround would otherwise be silently lost. + + """ + models_path = PROJECT_ROOT / MODELS_OUTPUT + if not models_path.exists(): + raise FileNotFoundError(f'{MODELS_OUTPUT} not found, cannot apply manual fixes') + + content = models_path.read_text() + + # Idempotency guard: if the marker is already present, the file is patched. + if _MANUAL_FIX_MARKER in content: + return False + + for anchor, replacement in _MANUAL_FIXES: + if anchor not in content: + raise RuntimeError( + f'Manual fix anchor not found in {MODELS_OUTPUT}; datamodel-codegen ' + f'output likely changed. Missing anchor:\n{anchor!r}' + ) + content = content.replace(anchor, replacement, 1) models_path.write_text(content) return True +def regenerate_models() -> bool: + """Run datamodel-codegen to regenerate Pydantic models, then re-apply fixes. + + Manual workarounds for known Firefly III API bugs are re-applied here (not only + on the CLI path) so every caller of this helper gets a patched models file. + + Returns: + True if successful, False otherwise + + """ + try: + subprocess.run( + ['uv', 'run', 'datamodel-codegen'], + cwd=PROJECT_ROOT, + capture_output=True, + text=True, + check=True, + ) + if apply_manual_fixes(): + print(' Applied manual fixes for known Firefly III API bugs') + return True + except subprocess.CalledProcessError as e: + print(f' Error running datamodel-codegen: {e.stderr}') + return False + except (FileNotFoundError, RuntimeError) as e: + print(f' Error applying manual fixes: {e}') + return False + + def cleanup_old_schema(old_version: str | None, new_version: str) -> bool: """Remove the old schema file if it exists and differs from new. @@ -335,13 +362,6 @@ def main() -> int: if regenerate_models(): print(f'Models regenerated at {MODELS_OUTPUT}') - # Re-apply manual workarounds for known Firefly III API bugs - print('Applying manual fixes...') - if apply_manual_fixes(): - print('Manual fixes applied') - else: - print('Manual fixes already present (or models file missing)') - # Format the generated code print('Formatting generated code...') try: diff --git a/tests/unit/test_scripts.py b/tests/unit/test_scripts.py index aacf6d8..7234fcc 100644 --- a/tests/unit/test_scripts.py +++ b/tests/unit/test_scripts.py @@ -260,3 +260,81 @@ def test_regenerate_models_failure(self, mock_run): result = update_schema.regenerate_models() assert result is False + + # --- apply_manual_fixes ------------------------------------------------- + + # Minimal stand-in for freshly generated firefly_models.py containing the + # exact anchors apply_manual_fixes() patches. + _GENERATED_STUB = ( + 'from __future__ import annotations\n\n' + 'from typing import Any\n\n' + 'from pydantic import AnyUrl, AwareDatetime, BaseModel, EmailStr, Field, RootModel\n\n\n' + 'class ArrayEntryWithCurrencyAndSum(BaseModel):\n' + " currency_id: str | None = Field(None, examples=['5'])\n" + ) + + def _stub_models_file(self, tmp_path, monkeypatch, content): + """Point update_schema at a temp models file containing ``content``.""" + models_path = tmp_path / 'firefly_models.py' + models_path.write_text(content) + monkeypatch.setattr(update_schema, 'PROJECT_ROOT', tmp_path) + monkeypatch.setattr(update_schema, 'MODELS_OUTPUT', 'firefly_models.py') + return models_path + + def test_apply_manual_fixes_patches_generated_file(self, tmp_path, monkeypatch): + """Fresh generated content is patched with the int->str coercion.""" + models_path = self._stub_models_file(tmp_path, monkeypatch, self._GENERATED_STUB) + + assert update_schema.apply_manual_fixes() is True + + patched = models_path.read_text() + assert '_coerce_to_str' in patched + assert 'BeforeValidator' in patched + assert 'Annotated[str | None, BeforeValidator(_coerce_to_str)]' in patched + + def test_apply_manual_fixes_is_idempotent(self, tmp_path, monkeypatch): + """Already-patched content is left untouched and reported as no-op.""" + models_path = self._stub_models_file(tmp_path, monkeypatch, self._GENERATED_STUB) + update_schema.apply_manual_fixes() + patched = models_path.read_text() + + assert update_schema.apply_manual_fixes() is False + assert models_path.read_text() == patched + + def test_apply_manual_fixes_missing_anchor_raises(self, tmp_path, monkeypatch): + """A drifted generated file (anchor missing) fails loudly.""" + drifted = self._GENERATED_STUB.replace( + 'from typing import Any\n', 'from typing import List\n' + ) + self._stub_models_file(tmp_path, monkeypatch, drifted) + + with pytest.raises(RuntimeError, match='anchor not found'): + update_schema.apply_manual_fixes() + + def test_apply_manual_fixes_missing_file_raises(self, tmp_path, monkeypatch): + """Missing models file fails loudly rather than silently skipping.""" + monkeypatch.setattr(update_schema, 'PROJECT_ROOT', tmp_path) + monkeypatch.setattr(update_schema, 'MODELS_OUTPUT', 'does_not_exist.py') + + with pytest.raises(FileNotFoundError): + update_schema.apply_manual_fixes() + + @patch('subprocess.run') + def test_regenerate_models_applies_fixes(self, mock_run, tmp_path, monkeypatch): + """regenerate_models patches the file after a successful codegen run.""" + mock_run.return_value = MagicMock(returncode=0) + models_path = self._stub_models_file(tmp_path, monkeypatch, self._GENERATED_STUB) + + assert update_schema.regenerate_models() is True + assert '_coerce_to_str' in models_path.read_text() + + @patch('subprocess.run') + def test_regenerate_models_fails_loud_on_drift(self, mock_run, tmp_path, monkeypatch): + """regenerate_models returns False if manual fixes cannot be applied.""" + mock_run.return_value = MagicMock(returncode=0) + drifted = self._GENERATED_STUB.replace( + 'from typing import Any\n', 'from typing import List\n' + ) + self._stub_models_file(tmp_path, monkeypatch, drifted) + + assert update_schema.regenerate_models() is False