fix: restore currency_id int->str coercion lost in model regen#83
Conversation
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 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 51 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a Pydantic validator workaround for a Firefly III API schema defect where ChangesFirefly III currency_id validator workaround
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #83 +/- ##
==========================================
- Coverage 97.64% 97.59% -0.05%
==========================================
Files 22 22
Lines 3475 3499 +24
==========================================
+ Hits 3393 3415 +22
- Misses 82 84 +2
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:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lampyrid/scripts/update_schema.py`:
- Around line 206-250: The apply_manual_fixes function currently always returns
True even when its replace() calls made no changes; update apply_manual_fixes to
detect whether any replacement actually happened by comparing the original
content to the modified content (or checking for the presence of the inserted
marker '_coerce_to_str' and the modified ArrayEntryWithCurrencyAndSum
annotation) before writing with models_path.write_text and returning True; if no
changes were applied, do not write and return False so callers (e.g., the code
path that expects the manual coercion for BudgetArray) can fail loudly.
- Around line 338-343: The manual Firefly III API workaround is only run in
main(), so regenerate_models() can leave firefly_models.py unpatched when called
directly; modify regenerate_models() to call apply_manual_fixes() after the
models file is written (the same place main() prints "Applying manual
fixes..."), using the same success/failure handling and informational output as
in main() so tests and callers that invoke regenerate_models() directly get the
patched file; reference the regenerate_models() function and the
apply_manual_fixes() call/site to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6caf3c6-5afc-4266-b8dd-2404cdbfb984
📒 Files selected for processing (2)
src/lampyrid/models/firefly_models.pysrc/lampyrid/scripts/update_schema.py
- 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 <noreply@anthropic.com>
Problem
Budget endpoints (e.g.
list_budget_limits/get_budget_spending) fail with a Pydantic validation error:The Firefly III API (Issue #43) returns
currency_idas an int insidespent/pc_spent(ArrayEntryWithCurrencyAndSum) arrays, even though the OpenAPI spec types it as astring.Root cause
A
BeforeValidatorcoercion (int → str) was originally added in346ce63("fix: add temp fix for availble_budgets"), but it lived directly in the auto-generatedfirefly_models.pyand was silently wiped out when the models were regenerated in55cdb0b("Regen firefly models with latest openapi spec").Fix
BeforeValidator(_coerce_to_str)onArrayEntryWithCurrencyAndSum.currency_id.apply_manual_fixes()step toscripts/update_schema.py, invoked right afterdatamodel-codegenruns, so the patch is re-injected automatically after every regeneration — preventing this fix from being lost again.Testing
currency_id: 25(int) →'25'(str), including via the fullBudgetLimitArraypayload shape that was failing.uv run pytest tests/unit/test_models.py— 17 passed.ruff format/ruff checkclean.🤖 Generated with Claude Code