fix(storage): handle PIT + metadata filter on aggregated balances when ACCOUNT_METADATA_HISTORY is disabled (v2.3)#1423
Conversation
…n ACCOUNT_METADATA_HISTORY is disabled The aggregated balances PIT path unconditionally joined the `accounts_metadata` history table when filtering by metadata, returning an empty result on ledgers configured with `ACCOUNT_METADATA_HISTORY: DISABLED` because that table is never populated under this setting. Gate the history join on `FeatureAccountMetadataHistory=SYNC` and fall back to the current `accounts.metadata` column otherwise, mirroring the pattern already used in `resource_accounts.go`. This restores the expected behaviour and matches what `/accounts` returns under the same configuration. Fixes #1416
The previous commit introduced a second lateral join aliased as `accounts` in the metadata fallback branch, which collided with the existing address-segments lateral when a PIT aggregate query combined a metadata filter with a partial address filter on a ledger configured with `ACCOUNT_METADATA_HISTORY: DISABLED`, producing `ERROR: table name "accounts" specified more than once`. Rename the fallback alias to `accounts_metadata` for symmetry with the SYNC branch, and extend the regression test with the combined metadata + partial address filter scenario.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughWhen ChangesAggregate Balances PIT + Metadata Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The production change appears aligned with the intended fallback, but the added integration test references an undefined type and will not compile when the it test suite is run.
| }) | ||
|
|
||
| t.Run("with pit and partial address filter", func(t *testing.T) { | ||
| ret, err := store.AggregatedVolumes().GetOne(ctx, common.ResourceQuery[ledger.GetAggregatedVolumesOptions]{ |
There was a problem hiding this comment.
🔴 [blocker] Use ledgerstore options in the new query
In this new subtest, ledger is the domain package (github.com/formancehq/ledger/internal), which does not define GetAggregatedVolumesOptions; the options type lives in internal/storage/ledger, imported above as ledgerstore. With the it tag enabled this file will fail to compile with undefined: ledger.GetAggregatedVolumesOptions, so the regression suite cannot run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/storage/ledger/balances_test.go`:
- Line 441: The ResourceQuery generic type parameter in the
AggregatedVolumes().GetOne() method call on line 441 is using the incorrect type
ledger.GetAggregatedVolumesOptions when it should be using
ledgerstore.GetAggregatedVolumesOptions to match the storage contract and be
consistent with the surrounding tests. Replace
ledger.GetAggregatedVolumesOptions with ledgerstore.GetAggregatedVolumesOptions
in the ResourceQuery type parameter.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e35796e5-ea1d-48a6-8bbf-33319fb0b998
📒 Files selected for processing (2)
internal/storage/ledger/balances_test.gointernal/storage/ledger/resource_aggregated_balances.go
v2.3's CommitTransaction handles account upsert internally, so the `commitTransactionAndUpsertAccounts` helper from main is unnecessary.
gfyrag
force-pushed
the
hotfix/v2.3/aggregate-balances-pit-metadata-history-disabled
branch
from
1be6728 to
8f46247
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
✅ Approve — automated review
The change correctly gates the PIT metadata history join on the account metadata history feature and falls back to current account metadata when disabled. The added regression test covers the described aggregate-balance scenarios, and no introduced correctness issues were identified.
No findings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v2.3 #1423 +/- ##
================================================
+ Coverage 81.54% 81.58% +0.03%
================================================
Files 187 187
Lines 9257 9266 +9
================================================
+ Hits 7549 7560 +11
- Misses 1253 1254 +1
+ Partials 455 452 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gfyrag
deleted the
hotfix/v2.3/aggregate-balances-pit-metadata-history-disabled
branch
4 participants
Summary
Backport of #1421 to
release/v2.3./aggregate/balances?pit=…with a metadata\$matchfilter silently returned{"data":{}}on ledgers configured withACCOUNT_METADATA_HISTORY: DISABLED, because the PIT path joined theaccounts_metadatahistory table that is never populated under that feature flag value.FeatureAccountMetadataHistory=SYNCand fall back to the currentaccounts.metadatacolumn otherwise — matching the pattern already used inresource_accounts.go.TestBalancesAggregatesPITWithMetadataHistoryDisabled) covering with-PIT, without-PIT, and the combined metadata + partial address filter case on a ledger configured withACCOUNT_METADATA_HISTORY: DISABLED.Notes on the backport:
balances_test.go.CommitTransactionalready upserts accounts internally, so thecommitTransactionAndUpsertAccountshelper from main is unnecessary — the regression test was adapted to callstore.CommitTransaction(ctx, &tx, nil)directly (separate adjustment commit).Fixes #1416
Test plan