Upgrade Firefly III API schema to v6.4.16 with validation#67
Conversation
- Add `destination_id` to `CreateWithdrawalRequest` to fix issue #62. - Add `source_id` to `CreateDepositRequest` for symmetric API support. - Configure all 18 request models with `extra='forbid'` to prevent silent failure of misspelled or unknown fields. - Implement mutual exclusivity validation for account ID and account name pairs using Pydantic model validators. - Update `TransactionService` to pass the new ID fields to the Firefly III API. - Fix a latent bug in `test_get_budget_summary` integration test discovered by the new strict validation. - Add comprehensive unit and integration tests for new fields and validation logic.
- Implement update-schema to automate Firefly III API model regeneration - Implement lint script for combined code formatting and auto-fixing - Register scripts as package entry points in pyproject.toml
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughBumps Firefly III OpenAPI to v6.4.16 and regenerates models; tightens many request models to forbid extras and adds mutual-exclusivity validators; adds destination_id/source_id handling in transaction payloads; introduces maintenance scripts (update_schema, format); updates tests and fixtures to cover new fields and behaviors. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant UpdateScript as update_schema.py
participant DocsAPI as Firefly Docs API
participant FS as Filesystem
participant Codegen as datamodel-codegen
participant Ruff as Ruff Formatter
User->>UpdateScript: run update-schema
UpdateScript->>DocsAPI: GET versions HTML
DocsAPI-->>UpdateScript: versions HTML
UpdateScript->>UpdateScript: parse & select latest stable
UpdateScript->>DocsAPI: GET /openapi.yaml for selected version
DocsAPI-->>UpdateScript: OpenAPI YAML bytes
UpdateScript->>FS: write firefly-iii-<v>-v1.yaml
UpdateScript->>FS: update pyproject.toml
UpdateScript->>Codegen: invoke datamodel-codegen (generate models)
Codegen-->>FS: write generated models
UpdateScript->>Ruff: run formatting
Ruff-->>FS: format files
UpdateScript->>FS: remove old schema (optional)
UpdateScript-->>User: exit status / report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
changed the title
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
- Coverage 99.81% 97.53% -2.28%
==========================================
Files 15 17 +2
Lines 2661 2841 +180
==========================================
+ Hits 2656 2771 +115
- Misses 5 70 +65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lampyrid/scripts/format.py`:
- Around line 7-21: main currently runs ruff with cwd='.' which is
caller-dependent; change main to resolve the repository/project root (e.g., via
pathlib on __file__ or git metadata) and pass that path as cwd to both
subprocess.run calls (the ones invoking ruff format and ruff check --fix) so
formatting is anchored to the repo root; ensure the resolved root is reused for
both calls and keep existing exception handling (CalledProcessError,
FileNotFoundError) intact.
In `@src/lampyrid/scripts/update_schema.py`:
- Around line 128-152: update_pyproject_toml currently silently no-ops if
neither the exact old_filename nor the regex matches; change it to return a
boolean success flag and surface failures: read the original content via
PYPROJECT_PATH.read_text(), compute the modified content (as currently done),
then compare original vs modified content and only call
PYPROJECT_PATH.write_text(content) if they differ; return True on change and
False otherwise so callers (the code that prints success) can check the boolean
and report an error when the update didn't occur. Ensure function signature
update_pyproject_toml(old_version: str | None, new_version: str) -> bool and
update any caller logic to handle a False result.
In `@tests/unit/test_models.py`:
- Around line 108-118: Update the docstrings in the test functions so they
reflect the actual assertions: change the docstring in
test_create_withdrawal_request_allows_neither_destination to state that
destination_id and destination_name can be omitted (not "cannot"), and make the
analogous docstring edit for the other test at the 177-185 range; ensure the
wording matches the test intent (e.g., "Test that destination_id and
destination_name may be omitted").
🧹 Nitpick comments (2)
tests/conftest.py (1)
281-290: Avoid fixed index access in fixtures; lookup by account name instead.Index-based access makes these fixtures brittle if setup order changes or more accounts are added. Consider resolving by name to keep tests stable.
♻️ Proposed refactor
def test_expense_account_obj() -> Account: @@ - if _cached_test_accounts is None or len(_cached_test_accounts) < 3: + if not _cached_test_accounts: raise RuntimeError('Test accounts not initialized. Check if _setup_test_data ran.') - return _cached_test_accounts[2] # Index 2 is expense account + account = next( + (acct for acct in _cached_test_accounts if acct.name == 'Test Expense'), + None, + ) + if account is None: + raise RuntimeError('Test Expense account not found in cached test accounts.') + return account @@ def test_revenue_account_obj() -> Account: @@ - if _cached_test_accounts is None or len(_cached_test_accounts) < 5: + if not _cached_test_accounts: raise RuntimeError('Test accounts not initialized. Check if _setup_test_data ran.') - return _cached_test_accounts[4] # Index 4 is revenue account + account = next( + (acct for acct in _cached_test_accounts if acct.name == 'Test Revenue'), + None, + ) + if account is None: + raise RuntimeError('Test Revenue account not found in cached test accounts.') + return accountAlso applies to: 314-323
tests/fixtures/transactions.py (1)
22-54: Consider using keyword-only parameters to establish a safer API contract.These new optional parameters appear to have no current call sites. While this means there's no immediate risk of positional-argument breakage, adding the
*separator to make these parameters keyword-only would improve the fixture API design and prevent future misuse. This is particularly valuable for test fixtures that may be used in multiple test files over time.♻️ Suggested refactor
def make_create_withdrawal_request( amount: float, description: str, source_id: str, + *, destination_name: str | None = None, destination_id: str | None = None, budget_id: str | None = None, date: datetime | None = None, ) -> CreateWithdrawalRequest:def make_create_deposit_request( amount: float, description: str, destination_id: str, + *, source_name: str | None = None, source_id: str | None = None, date: datetime | None = None, ) -> CreateDepositRequest:
RadCod3
force-pushed
the
feat/62-add-destinationid
branch
from
ad224ae to
3d64134
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/conftest.py`:
- Around line 321-337: The fixture test_revenue_account_obj currently requires
an exact name match which can conflict with the case-insensitive substring
selection used in _setup_test_data; update test_revenue_account_obj to first try
an exact match on acct.name == 'Test Revenue' against _cached_test_accounts, and
if that yields None, fall back to a case-insensitive substring search (e.g.,
'Test Revenue' in acct.name.lower()) to find accounts like "Test Revenue 2",
then raise the RuntimeError only if both lookups fail.
In `@tests/unit/test_scripts.py`:
- Around line 3-10: Remove the unused imports sys, pytest, and httpx from the
top import block and reorder the remaining imports into standard groups (stdlib,
third-party, local) in sorted order; keep only Path from pathlib and MagicMock,
patch from unittest.mock, then the local imports from lampyrid.scripts (format,
update_schema) so the import block is clean and CI-friendly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lampyrid/scripts/update_schema.py`:
- Around line 263-282: The script currently prints a warning when
regenerate_models() returns False but still exits with 0; update the end of the
regeneration block so that if regenerate_models() returns False the script
terminates with a non-zero status (e.g., call sys.exit(1) or raise SystemExit)
and include a clear message referencing MODELS_OUTPUT; also consider treating
formatting failures (the subprocess.run call that invokes 'uv run ruff format'
and the except block catching subprocess.CalledProcessError) as fatal in CI by
exiting non-zero there as well to avoid masking failures.
🧹 Nitpick comments (5)
tests/unit/test_format.py (1)
1-19: Duplicate test class exists intests/unit/test_scripts.py.This entire file duplicates the
TestFormatScriptclass already present intests/unit/test_scripts.py(lines 11-74), which contains more comprehensive tests including error handling scenarios. Consider removing this file to avoid test duplication and maintenance overhead.#!/bin/bash # Description: Verify the duplicate TestFormatScript classes in both files echo "=== tests/unit/test_format.py ===" rg -n "class TestFormatScript" tests/unit/test_format.py echo "" echo "=== tests/unit/test_scripts.py ===" rg -n "class TestFormatScript" tests/unit/test_scripts.py echo "" echo "=== Test method counts ===" echo "test_format.py methods:" rg -c "def test_" tests/unit/test_format.py echo "test_scripts.py TestFormatScript methods:" ast-grep --pattern $'class TestFormatScript { $$$ def test_$_($$$) { $$$ } $$$ }'tests/unit/test_scripts.py (2)
26-36: Fragile assertion assumes specific project directory name.The assertion
kwargs.get('cwd').name == 'LamPyrid'(line 31) assumes the project directory is always named "LamPyrid". This could break if the repository is cloned to a differently named directory.Consider asserting that
cwdis a validPathand exists, or remove this specific name check.♻️ Suggested fix
args, kwargs = mock_run.call_args_list[0] assert args[0] == ['ruff', 'format', '.'] assert kwargs.get('check') is True assert isinstance(kwargs.get('cwd'), Path) - assert kwargs.get('cwd').name == 'LamPyrid' + # Verify cwd is set to project root (contains pyproject.toml) + assert (kwargs.get('cwd') / 'pyproject.toml').exists() or True # Mock doesn't check filesystemOr simply remove the name check since the important assertion is that
cwdis aPath:- assert kwargs.get('cwd').name == 'LamPyrid'
243-251: Test name doesn't match test behavior.
test_update_pyproject_toml_regex_fallbackactually testsget_current_schema_version(), notupdate_pyproject_toml(). The test should either be renamed to reflect what it's testing or be updated to actually test the regex fallback behavior inupdate_pyproject_toml.♻️ Suggested fix - rename to match behavior
`@patch`('pathlib.Path.read_text') - `@patch`('pathlib.Path.write_text') - def test_update_pyproject_toml_regex_fallback(self, mock_write, mock_read): - """Test update_pyproject_toml when direct string replacement fails.""" + def test_get_current_schema_version_parses_version(self, mock_read): + """Test get_current_schema_version extracts version from pyproject.toml.""" mock_read.return_value = 'input = "firefly-iii-6.4.14-v1.yaml"' result = update_schema.get_current_schema_version() assert result == '6.4.14'tests/unit/test_models.py (2)
54-91: Missing mutual exclusivity test forCreateWithdrawalRequest.The
TestCreateDepositRequestclass includes a test for mutual exclusivity validation (test_create_deposit_request_mutual_exclusivity), butTestCreateWithdrawalRequestis missing an equivalent test. According to the model definition,CreateWithdrawalRequestalso has a validator that raises an error when bothdestination_idanddestination_nameare provided.🧪 Suggested test to add
def test_create_withdrawal_request_mutual_exclusivity(self): """Test that destination_id and destination_name cannot both be provided.""" with pytest.raises(ValidationError) as exc_info: CreateWithdrawalRequest( amount=25.50, description='Test withdrawal', source_id='1', destination_id='5', destination_name='Groceries', ) errors = exc_info.value.errors() assert len(errors) == 1 assert 'Cannot specify both destination_id and destination_name' in str(errors[0]['msg'])
147-164: Consider adding test coverage for transaction type validation.The
CreateBulkTransactionsRequestmodel has a validator that only allowswithdrawal,deposit, andtransfertransaction types. Consider adding a test to verify that invalid transaction types (likereconciliationoropening_balance) raise aValidationError.🧪 Suggested test to add
def test_create_bulk_transactions_request_rejects_invalid_type(self): """Test that CreateBulkTransactionsRequest rejects invalid transaction types.""" transaction = Transaction( type=TransactionTypeProperty.reconciliation, # Invalid for bulk create amount=25.50, description='Test reconciliation', date=utc_now(), source_id='1', destination_id='2', ) with pytest.raises(ValidationError) as exc_info: CreateBulkTransactionsRequest(transactions=[transaction]) errors = exc_info.value.errors() assert 'Invalid transaction type' in str(errors[0]['msg'])
2 participants
This PR does the following,