Claudio Schaad
f9084dc074
Every repository, service, Blazor page/dialog, and test now uses
async/await. Single atomic diff; the codebase does not compile in
intermediate states.
- BaseRepository: LoadListAsync/LoadAsync/SaveAsync return Task<T>;
per-file locks use SemaphoreSlim so waiters can await; Save
serialises to a MemoryStream sync (XmlSerializer has no async
form), then File.WriteAllBytesAsync + sync File.Move.
- RepositoryCache.GetOrLoadAsync takes a Func<Task<List<T>>>.
- All 7 repository interfaces + implementations async.
- All service interfaces + implementations async (except vendor
IFxService and stateless IFxConverter / SettingsService).
- Every Blazor OnInitializedAsync switches to await base.
- Test suite fully async, 42 tests pass.
AccountRepository.EnsureAccountsFile keeps two .GetAwaiter().GetResult()
bridges because it runs from the constructor.
Null-render guard follow-up (folded in):
Blazor now renders the component once with fields at their initial
values while OnInitializedAsync awaits — so fields declared `= null!`
are actually null on that first render and things like
`accounts.GroupBy(...)` throw ArgumentNullException. Fixed across
Transactions, BalanceReport, BalanceSheetReport, ProfitLossReport,
DetailReport, Assets, Spendings, SpendingsOverTime, TransactionDialog,
TransactionSplitDialog, and BookingRuleDialog:
- Collection fields initialise to [] so first-render loops are empty.
- Single-object data fields become nullable; the razor wraps
consumption in `@if (field is null) { <p>Lädt…</p> return; }`.
- <PlotlyChart> guarded behind a null check on config/layout/data so
Plotly.Blazor's @bind doesn't see nulls.
- Header/footer strings initialise to "" instead of null!.
Architectural hygiene on a single-user local Blazor Server app: the
observed win is one File.ReadAllBytesAsync and one
File.WriteAllBytesAsync per Load/Save, and after PR D each file is
loaded at most once per SignalR circuit.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
114 lines
16 KiB
Markdown
114 lines
16 KiB
Markdown
# AccountingNext — Technical Improvement Plan
|
||
|
||
Analysis and phased plan produced 2026-07-02. See conversation history for full context.
|
||
|
||
## Findings
|
||
|
||
### 1. Correctness bugs (must fix)
|
||
|
||
- **Shared mutable state across users.** `SettingsService` is registered as `Singleton` (`Schaad.Accounting.UI/Extensions.cs:21`) but holds mutable `year`/`mandator` fields (`Schaad.Accounting.Services/SettingsService.cs:13-14`). Any user switching year/mandator changes it for *all* connected Blazor Server users.
|
||
- **`TrySetYear` rollback is broken.** `SettingsService.cs:33` captures `var oldYear = year;` — that's the *parameter*, not `this.year`. The "rollback" restores the same value that just failed.
|
||
- **`ChartService` mutates a shared service.** `GetAccountExpensesPerMonth(accountId, year)` (`ChartService.cs:87`) calls `settingsService.TrySetYear(year)` then `SetYear(DateTime.Now.Year)` to reload prior-year data. Global mutation on a singleton — guaranteed to race in production.
|
||
- **`DummyFxService` has a typo.** `DummyFxService.cs:13` — `currencies.Contains(fromCurrency) && currencies.Contains(fromCurrency)` (both sides check `fromCurrency`). Also it silently returns the raw amount for supported currencies, so all "CHF conversions" are no-ops.
|
||
- **`TransactionRepository.GetTransaction` mutates the entity.** `TransactionRepository.cs:67-81` divides `Value` by `FxRate` on read, and `SaveTransaction` multiplies by `FxRate` on write. Also silently NREs when the id is not found (line 73 dereferences `transaction`).
|
||
- **`ViewService.GetTransactionViewList(accountId)` mutates transactions.** `ViewService.cs:172` flips `t.Value *= -1` on the loaded transaction. `FileService.GetTransactionListCsv` does the same (`FileService.cs:137`). These are the shared objects returned by the repository — later reads see wrong signs.
|
||
- **DI double-registration.** `Extensions.cs` registers `IChartService`, `IViewService`, `IFileService`, statement services, and `IFxService` in both `AddRepositories` and `AddServices`. Also `AddServices` registers `PdfParsingService` as its own key (`services.AddSingleton<PdfParsingService, PdfParsingService>()`) instead of `IPdfParsingService`.
|
||
- **Magic numbers instead of `ClassIds`.** `ProfitLossReport.razor.cs:25-26` uses `.Class == 3` / `.Class == 4`.
|
||
- **`ClassIds` doc/code mismatch.** `CLAUDE.md` states `Activa=1000, Passiva=2000, …`, but `ClassIds.cs` defines them as `1,2,3,4` (matches `Account.Class = Number / 1000`).
|
||
- **Non-atomic XML writes, no locking.** `BaseRepository.Save` writes directly to the target path. Interrupted writes corrupt data. Two concurrent saves interleave. No temp-file+rename, no `FileShare` lock.
|
||
- **`AccountRepository.EnsureFileExisits` calls `SaveAccount` in a loop.** Each call re-loads and re-serializes the whole account list.
|
||
- **`ProfitLossReport` / `BalanceSheetReport`** compute totals by re-summing balances that `ViewService` already computes — but with subtle differences. Two sources of truth for the same number.
|
||
|
||
### 2. Reliability / maintainability
|
||
|
||
- **No async I/O.** Every XML read/write is synchronous, blocking the SignalR hub thread in Blazor Server.
|
||
- **No caching.** A single page render calls `GetAccountList()` and `GetTransactionList()` many times. Every one is a fresh file read + XML deserialization.
|
||
- **O(N·M) lookups.** `ViewService.cs:138-139` — `accountList.Single(...)` inside a `Select` over all transactions. Should be a dictionary keyed by id.
|
||
- **No logging.** `ILogger` is not used anywhere.
|
||
- **No error surface.** Repositories throw or return `null`; UI dereferences with `!`.
|
||
- **Anemic domain model + `Copy` methods.** `Account.Copy`, `Transaction.Copy`, `SubClass.Copy` etc. are hand-rolled property copies used to merge edits into loaded entities.
|
||
- **Vendor coupling in the Domain project.** `Schaad.Accounting.Common` references `Schaad.Finance.Api.dll`. `IChartService`, `IFileService`, and `IViewService` live in `Common` and depend on `Schaad.Finance.Api` types.
|
||
- **Duplicated formatting logic.** `ToFormattedString(decimal)` exists in `Extensions.cs` and again in `FileService.cs`.
|
||
- **Culture setup in `Program.cs`** runs *after* `MapRazorComponents` and just sets `DefaultThreadCurrentCulture` globally. Should be `RequestLocalizationOptions` middleware.
|
||
- **Constructors doing I/O.** `AccountRepository`, `SubclassRepository`, `SplitPredefinitonRepository`, `BookingRuleRepository` call `EnsureFileExisits` in the constructor.
|
||
- **Typos leak into public API.** `EnsureFileExisits`, `SplitPredefinitonRepository`.
|
||
- **Dead / redundant code.** `AccountRepository.EnsureFileExisits` uses `new` to hide the base method; its `file.IndexOf("Accounts") > -1` guard is redundant. `Home.razor.cs:50` uses a `loaded` flag though `OnInitializedAsync` already runs once per instance. Commented-out `//var subclasses = subclassRepository.GetSubClassList();` in `ChartService`.
|
||
- **Legacy nuget packages.** All three library projects reference `System.Text.RegularExpressions 4.3.1`, `System.Xml.XmlSerializer 4.3.0`, `System.ComponentModel.Annotations 5.0.0` — legacy .NET Standard packages, redundant on `net9.0`.
|
||
- **`Common.csproj`** has `<Folder Include="Interfaces\Extensions\" />` for a folder that does not exist.
|
||
- **`ClassIds`** should be `static class`; currently instantiable.
|
||
|
||
### 3. Testability
|
||
|
||
- Zero tests, zero test project.
|
||
- Business rules (balance calculation, FX conversion, bank-transaction matching, CSV export, split logic) are entangled with mutable singletons and side effects on returned entities.
|
||
|
||
---
|
||
|
||
## Phased plan
|
||
|
||
### Phase 1 — Stop the bleeding (correctness, low churn)
|
||
|
||
1. ~~Change `ISettingsService` registration from `Singleton` to `Scoped`.~~ **Deferred.** `MyHeader.YearChanged` uses `NavigateTo(..., forceLoad: true)` after mutating settings, which tears down the SignalR circuit — a Scoped instance would be recreated with default values on the new circuit. Proper fix: persist the year/mandator selection to a cookie or query string, then Scoped becomes safe. Tracked as a new Phase 2 item.
|
||
2. Fix `TrySetYear` (capture `this.year` before overwriting). Defer the `ChartService` prior-year-loading pattern to Phase 3.
|
||
3. Fix `DummyFxService` typo (or delete the class and replace with a real `IFxService` implementation from `Schaad.Finance.Api`).
|
||
4. Remove mutation-on-read in `TransactionRepository.GetTransaction`, `ViewService.GetTransactionViewList(accountId)`, and `FileService.GetTransactionListCsv`.
|
||
5. Add a not-found guard to `TransactionRepository.GetTransaction`.
|
||
6. De-duplicate the DI registrations. One `AddAccounting()` extension called once in `Program.cs`.
|
||
7. Replace `.Class == 3/4` magic numbers with `ClassIds.*`. Update CLAUDE.md's incorrect ClassIds section.
|
||
8. Make XML writes atomic: write to `foo.xml.tmp` then `File.Move(..., overwrite: true)`. Wrap Load/Save in a per-file `SemaphoreSlim` (or a simple `lock`).
|
||
|
||
### Phase 2 — Structural cleanup
|
||
|
||
9. **Introduce a per-request unit of work / cache.** A scoped `IAccountingContext` that loads each XML file at most once per request and holds the deserialized lists.
|
||
10. Replace `Copy(target)` methods with a single merge-in-place pattern (or `record with`).
|
||
11. Extract shared formatting to a single `Formatting` helper.
|
||
12. Move `IFileService`, `IChartService`, `IViewService` out of `Common` (they depend on `Schaad.Finance.Api`). Common should have no vendor dependency.
|
||
13. Fix typos (`EnsureFileExists`, `SplitPredefinitionRepository`).
|
||
14. Move file existence bootstrapping out of constructors into a startup step (`IHostedService` or lazy first-use).
|
||
15. Fix `AccountRepository.EnsureFileExists` to compute the start-balance updates in memory and save once.
|
||
16. Remove legacy NuGet packages. Remove the stale `Interfaces\Extensions\` folder entry.
|
||
17. Set culture via `RequestLocalizationOptions` middleware.
|
||
18. Add `ILogger<T>` to services and repositories.
|
||
18b. Persist selected year and mandator across page reloads (cookie, query string, or `ProtectedLocalStorage`). Prerequisite for making `ISettingsService` `Scoped` (Phase 1 item 1, deferred).
|
||
|
||
### Phase 3 — Async & performance
|
||
|
||
19. Convert repository interfaces to async.
|
||
20. Cache the current view's data behind the scoped unit-of-work; invalidate on save.
|
||
21. Precompute `accountsById` and `subclassNameByNumber` dictionaries once per request.
|
||
22. Clean up `ChartService` prior-year loading pattern: introduce an explicit "load year data" helper instead of mutating `ISettingsService`.
|
||
|
||
### Phase 4 — Testability & safety net
|
||
|
||
23. Add a `Schaad.Accounting.Tests` xUnit project.
|
||
24. First tests: `ViewService.GetBalanceView`, `MatchBankTransactionByBookingRule` / `SameAccountsLastMonth`, `TransactionRepository` FX round-trip, `FileService.GetTransactionListCsv`.
|
||
25. Abstract the XML store (`IEntityStore<T>`) so tests use an in-memory store.
|
||
|
||
### Phase 5 — Nice-to-haves
|
||
|
||
26. Replace hand-written XML models with `record` types + source-generated serializers.
|
||
27. Inject Fixer.io key via `IOptions<FxSettings>` instead of threading through method params.
|
||
28. Prune unused Fluent UI packages.
|
||
29. Add a health/backup admin page.
|
||
|
||
---
|
||
|
||
## PR breakdown
|
||
|
||
- **PR A** — Phase 1 items 1–7 + CLAUDE.md fix.
|
||
- **PR B** — Phase 1 item 8 (atomic writes + lock).
|
||
- **PR C** — Phase 2 mechanical cleanup: items 11, 13, 15, 16, 17.
|
||
- **PR D** — Phase 2 item 9: per-request unit of work / cache. Split off from PR C because it is invasive enough to warrant its own review.
|
||
- **PR E** — Phase 3 item 22: ChartService cleanup. Promoted ahead of async because it fixes an active correctness bug — `GetAccountExpensesPerMonth(accountId, year)` was mutating `ISettingsService` on the singleton to hop years, with the year parameter always hardcoded to `DateTime.Now.Year`, so the Spendings-over-time chart discarded the user's header year selection.
|
||
- **PR F** — Phase 3 item 21: precomputed dictionaries in `ViewService` to eliminate O(N·M) `Single(...)` scans and per-account transaction filtering.
|
||
- **Item 19 (async I/O)** — landed as PR P after being explicitly requested. See PR P entry below.
|
||
- **PR G** — Phase 4 items 23 + 24: `Schaad.Accounting.Tests` xUnit + NSubstitute + Shouldly project. Adopts the `xxxTestShould.DoThisWhenThat` naming convention with Shouldly assertions (no xUnit `Assert.*`). Covers `ViewService`, `TransactionRepository`, `Formatting`, `RepositoryCache`, `AccountRepository`, `ChartService`, and `FileService.GetTransactionListCsv` — 38 tests locking in the earlier PRs' behavior. Note: originally shipped as three separate commits (initial project, expanded coverage, Shouldly + naming conversion) and later squashed into one commit at the user's request.
|
||
- **PR H** — Phase 2 item 18 (first slice): add `ILogger<FileService>` to `FileService.ImportAccountStatementFile` so bank-statement imports emit `Information` for the file being processed and each account's import count, `Warning` when an account is skipped because it belongs to a different mandator, and `Error` when the vendor parser reports a failure. Rest of the logging (BaseRepository save failures, MatchOpenBankTransactions summary) tracked as a follow-up because it requires threading loggers through all seven repositories.
|
||
- **PR I** — Phase 2 item 12: move the service interfaces (`IViewService`, `IFileService`, `IChartService`) from `Schaad.Accounting.Common` into `Schaad.Accounting.Services/Interfaces/`. Namespaces are unchanged (`Schaad.Accounting.Interfaces`), so no consumer needs a `using` update. Drops the `Schaad.Finance.Api` `<Reference>` from `Common.csproj` — Common is now vendor-free and matches its documented role as the "shared models, DTOs, interfaces" layer.
|
||
- **PR J** — Phase 2 item 18 (second slice): thread `ILogger<T>` through `BaseRepository` and every concrete repository so `BaseRepository.Save`'s catch block logs the failing file path and exception before rethrowing (previously the exception's origin was silently swallowed and only the stack trace at the callsite survived). Pulls `Microsoft.Extensions.Logging.Abstractions` into the Db project. `TransactionRepositoryTestShould` and `AccountRepositoryTestShould` use `NullLogger<T>.Instance`. Remaining item 18 work (MatchOpenBankTransactions match-count summary in `ViewService`) still open.
|
||
- **PR K** — Phase 5 item 27: introduce `IFxConverter.ConvertToChf(amount, fromCurrency)` and its `FxConverter` implementation. The vendor `IFxService` and the FixerIo API key are now hidden inside `FxConverter`; callers stop threading the API key through every method call. `ViewService` drops both `IFxService` and `ISettingsService` from its constructor and takes `IFxConverter` instead. Simplifies `GetAccountViewList` (no more per-call `settingsService.GetSettings()` reads) and `GetBalanceSheetView`. Two-test `FxConverterTestShould` locks in the target-currency and API-key routing.
|
||
- **PR L** — Phase 2 item 10: drop the hand-rolled `Copy(target)` methods from `Account`, `BookingRule`, `BookingText`, `SubClass`, and `SplitPredefinition`; keep only a `Transaction.Clone()` for defensive-copy needs in `GetTransaction` / `ViewService.WithDisplaySign`. Every `SaveXxx` now uses `FindIndex` → in-place replace (or `Add` for new entries), moving the two "hidden" defaults (`Account.Currency = "CHF"` when unset, `Transaction.BookingDate = ValueDate` when unset) into the corresponding `SaveXxx` method where they belong. Fixes a pre-existing bug: `Transaction.Copy` never copied `RelatedParty`, so update-saves silently dropped it. Added `PreserveRelatedPartyWhenRoundTrippingTransaction` and `DefaultBookingDateToValueDateWhenBookingDateIsUnset` as regression tests. 42 tests total.
|
||
- **PR M** — Phase 2 item 18 (final slice): add `ILogger<ViewService>` and log a match-count summary from `MatchOpenBankTransactions` (`"Matched {Matched} of {Total} open bank transactions"`). Completes item 18 — statement imports (PR H), repository save failures (PR J), and match runs now all emit structured logs. Also removes a dead `ISettingsService` injection from `Home.razor.cs` (declared with `[Inject]` but never used anywhere in the file).
|
||
- **PR N** — Small polish pass covering three long-noted rough edges from the original analysis: `ClassIds` becomes a `static class` (was instantiable); `Home.razor.cs` loses the redundant `loaded` guard in `OnInitializedAsync` (Blazor already runs that lifecycle hook once per component instance); the hardcoded `mandator = "Claudio Schaad"` in `SettingsService` moves to a `DefaultMandator` field on `SettingsDataset`, plumbed through `appsettings.Development.json`.
|
||
- **PR O** — Phase 5 item 26 (partial): convert `BalanceDataset` and `BalanceSheetDataset` to positional records. Both were constructor-initialised value carriers with read-only usage. The rest of item 26 was descoped: the XML-serialised domain models (`Account`, `Transaction`, ...) need mutable public setters for `XmlSerializer`, and value-equality on mutable data is a footgun (hash changes on mutation); source-generated XML serialisers do not exist without switching file formats. `DataSerie` was already a record; `MessageDataset` has a real mutation method (`Add`) and stays a class; `AccountDataset` / `TransactionDataset` / `BookingRuleDataset` inherit from the mutable domain models and can't cleanly become records without a bigger refactor.
|
||
- **PR P** — Phase 3 item 19: full async I/O top to bottom. `BaseRepository.LoadAsync/SaveAsync/LoadListAsync` return `Task<T>`; the per-file `lock (obj)` becomes `SemaphoreSlim` so it can be `await`-ed. `Save` serialises to a `MemoryStream` synchronously (XmlSerializer has no async form) then writes bytes with `File.WriteAllBytesAsync`; `File.Move` (atomic rename) has no async counterpart in .NET 9 and stays sync. Every repository interface + implementation, every service interface + implementation (except the vendor-owned `IFxService`), every Razor page/dialog `OnInitializedAsync`, and every test-file assertion becomes `async` / `await`. 51 files touched in one atomic diff — the codebase does not compile in intermediate states. `AccountRepository.EnsureAccountsFile` keeps two `.GetAwaiter().GetResult()` bridges because it runs from the constructor (constructors can't be async). The observed async payoff is one `File.ReadAllBytesAsync` and one `File.WriteAllBytesAsync` per Load/Save; after PR D each XML file is deserialised at most once per SignalR circuit, so on this single-user local Blazor Server app this is architectural-hygiene work, not a measurable perf win.
|