From 5ae4065d9ef77f87354a1d9e2f757c609fa75ccc Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Wed, 19 Aug 2026 15:45:02 -0700 Subject: [PATCH] better errors for malformed portfolio files --- src/analytics/analysis.zig | 12 +- src/analytics/projections.zig | 12 +- src/cache/store.zig | 250 +++++++++++++++++++++++++++++++++- src/commands/common.zig | 128 ++++++++++++++++- src/data/imported_values.zig | 7 +- src/models/classification.zig | 36 ++++- src/portfolio_loader.zig | 113 ++++++++++++++- 7 files changed, 548 insertions(+), 10 deletions(-) diff --git a/src/analytics/analysis.zig b/src/analytics/analysis.zig index b61e46f..4912dc3 100644 --- a/src/analytics/analysis.zig +++ b/src/analytics/analysis.zig @@ -547,7 +547,17 @@ pub fn parseAccountsFile(allocator: std.mem.Allocator, data: []const u8) !Accoun defer it.deinit(); while (try it.next()) |fields| { - const entry = fields.to(AccountTaxEntry, srf_opts.user_edited) catch continue; + const entry = fields.to(AccountTaxEntry, srf_opts.user_edited) catch |err| { + // Skip the account rather than losing the whole file, but + // name the error: a dropped account loses its tax type, + // cadence and carve-outs, and every consumer then silently + // falls back to defaults. Quiet under `zig build test`, + // where fixtures feed malformed records on purpose. + if (!builtin.is_test) { + log.warn("accounts.srf: skipping malformed record: {s}", .{@errorName(err)}); + } + continue; + }; // A zero/negative large-lot threshold is nonsensical (zero // flags every new lot; negative is meaningless). Reject it and diff --git a/src/analytics/projections.zig b/src/analytics/projections.zig index c63552e..36dc34e 100644 --- a/src/analytics/projections.zig +++ b/src/analytics/projections.zig @@ -763,7 +763,17 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { var annotation_count: u8 = 0; while (it.next() catch null) |field_it| { - const rec = field_it.to(SrfProjection, srf_opts.user_edited) catch continue; + const rec = field_it.to(SrfProjection, srf_opts.user_edited) catch |err| { + // Skip the record rather than losing the whole file, but + // name the error: a dropped record reverts that setting to + // its default without saying so. Quiet under + // `zig build test`, where fixtures feed malformed records + // on purpose. + if (!builtin.is_test) { + log.warn("projections.srf: skipping malformed record: {s}", .{@errorName(err)}); + } + continue; + }; switch (rec) { .config => |c| { config.target_stock_pct = c.target_stock_pct orelse config.target_stock_pct; diff --git a/src/cache/store.zig b/src/cache/store.zig index 47b630e..d0677dc 100644 --- a/src/cache/store.zig +++ b/src/cache/store.zig @@ -2,6 +2,7 @@ const std = @import("std"); const log = std.log.scoped(.cache); const srf = @import("srf"); const srf_opts = @import("../srf_opts.zig"); +const format = @import("../format.zig"); const atomic = @import("../atomic.zig"); const version = @import("../version.zig"); const Date = @import("../Date.zig"); @@ -2090,8 +2091,79 @@ pub fn serializePortfolio(allocator: std.mem.Allocator, lots: []const Lot) ![]co return aw.toOwnedSlice(); } +/// Collected diagnostics for records that could not be parsed. Messages +/// are allocator-owned; the caller frees each one and the list. +/// +/// One message per skipped record, so `items.len` is the skip count. +pub const ParseDiagnostics = std.ArrayList([]const u8); + +/// Longest stretch of a malformed record echoed back to the user. +/// +/// SRF returns which error occurred but not which field caused it, so +/// showing the record is how the user finds the culprit. 120 columns is +/// wide enough for a typical lot to appear whole and narrow enough not +/// to wrap and bury the message it is attached to. +const diag_record_cols: usize = 120; + +/// Append a "could not parse this record" diagnostic naming the error +/// and echoing the record. +/// +/// `data` is the whole file and `line` is 1-based, so the record text is +/// recovered by counting newlines - `srf`'s `state.current_line` is +/// consumed as fields are read and would only yield the unparsed +/// remainder. Multi-line (`#!long`) records show their first line, which +/// is enough to locate them. +fn appendParseDiag( + allocator: std.mem.Allocator, + diags: *ParseDiagnostics, + data: []const u8, + line: usize, + detail: []const u8, +) !void { + const raw = nthLine(data, line); + const shown = format.truncateToCols(raw, diag_record_cols); + const msg = if (shown.len < raw.len) + try std.fmt.allocPrint(allocator, "line {d}: {s}\n {s}...", .{ line, detail, shown }) + else + try std.fmt.allocPrint(allocator, "line {d}: {s}\n {s}", .{ line, detail, shown }); + errdefer allocator.free(msg); + try diags.append(allocator, msg); +} + +/// The `line`-th line of `data`, 1-based, without its terminator. +/// Returns an empty slice when `line` is out of range. Borrowed. +fn nthLine(data: []const u8, line: usize) []const u8 { + if (line == 0) return data[0..0]; + var n: usize = 1; + var it = std.mem.splitScalar(u8, data, '\n'); + while (it.next()) |l| : (n += 1) { + if (n == line) return std.mem.trimEnd(u8, l, "\r"); + } + return data[0..0]; +} + /// Deserialize a portfolio from SRF data. Caller owns the returned Portfolio. pub fn deserializePortfolio(allocator: std.mem.Allocator, data: []const u8) !Portfolio { + return deserializePortfolioDiag(allocator, data, null); +} + +/// `deserializePortfolio`, optionally collecting a diagnostic per +/// skipped record into `diags`. +/// +/// A skipped lot silently changes every figure zfin prints - net worth, +/// allocation, contributions, compare - so the caller needs to be able +/// to say so next to those figures rather than hoping a `std.log.warn` +/// on stderr was noticed. `portfolio_loader` collects these into +/// `LoadedPortfolio.warnings`, and `cli.loadPortfolio` prints them. +/// +/// Passing null keeps the previous behaviour exactly: warn to the log +/// and carry on. That is what the git-historical and import paths want, +/// where a warning about an old revision would be noise. +pub fn deserializePortfolioDiag( + allocator: std.mem.Allocator, + data: []const u8, + diags: ?*ParseDiagnostics, +) !Portfolio { var lots: std.ArrayList(Lot) = .empty; errdefer { for (lots.items) |lot| { @@ -2115,8 +2187,12 @@ pub fn deserializePortfolio(allocator: std.mem.Allocator, data: []const u8) !Por // `user_edited` coercion: see `srf_opts.zig` for why hand-edited // files get different options from cache files. The `catch` // below still handles genuinely unparseable values. - var lot = fields.to(Lot, srf_opts.user_edited) catch { - std.log.warn("portfolio: could not parse record at line {d}", .{line}); + var lot = fields.to(Lot, srf_opts.user_edited) catch |err| { + if (diags) |d| { + try appendParseDiag(allocator, d, data, line, @errorName(err)); + } else { + std.log.warn("portfolio: could not parse record at line {d}: {s}", .{ line, @errorName(err) }); + } skipped += 1; continue; }; @@ -2136,7 +2212,11 @@ pub fn deserializePortfolio(allocator: std.mem.Allocator, data: []const u8) !Por .cash => try allocator.dupe(u8, "CASH"), .illiquid => try allocator.dupe(u8, "ILLIQUID"), else => { - std.log.warn("portfolio: record at line {d} has no symbol, skipping", .{line}); + if (diags) |d| { + try appendParseDiag(allocator, d, data, line, "no symbol"); + } else { + std.log.warn("portfolio: record at line {d} has no symbol, skipping", .{line}); + } if (lot.note) |n| allocator.free(n); if (lot.label) |l| allocator.free(l); if (lot.account) |a| allocator.free(a); @@ -2151,7 +2231,10 @@ pub fn deserializePortfolio(allocator: std.mem.Allocator, data: []const u8) !Por try lots.append(allocator, lot); } - if (skipped > 0) { + // Only log the rollup when nobody is collecting: a caller with + // `diags` reports the count itself, in band, and would otherwise say + // it twice. + if (skipped > 0 and diags == null) { std.log.warn("portfolio: {d} record(s) could not be parsed and were skipped", .{skipped}); } @@ -4171,6 +4254,165 @@ test "deserializePortfolio: underscore digit separators parse (they always did)" try std.testing.expectApproxEqAbs(@as(f64, 1_234_567), p.lots[0].shares, 0.5); } +test "deserializePortfolioDiag: a bad enum names the error and echoes the record" { + // The shape `strings_to_numbers` cannot help with: enum coercion + // ignores that option entirely, so a typo'd security_type is the + // realistic way a lot gets dropped. srf reports WHICH error but not + // which field, so the record text is how the user finds `stok`. + const allocator = std.testing.allocator; + const data = + "#!srfv1\n" ++ + "symbol::AAPL,shares:num:100,open_date::2024-01-15,open_price:num:140.00\n" ++ + "symbol::MSFT,security_type::stok,shares:num:50,open_date::2024-02-01,open_price:num:400.00\n"; + + var diags: ParseDiagnostics = .empty; + defer { + for (diags.items) |w| allocator.free(w); + diags.deinit(allocator); + } + + var p = try deserializePortfolioDiag(allocator, data, &diags); + defer p.deinit(); + + // The good lot survives; the bad one is dropped, not fatal. + try std.testing.expectEqual(@as(usize, 1), p.lots.len); + try std.testing.expectEqualStrings("AAPL", p.lots[0].symbol); + + // One message per skipped record, so len is the count. + try std.testing.expectEqual(@as(usize, 1), diags.items.len); + const msg = diags.items[0]; + try std.testing.expect(std.mem.indexOf(u8, msg, "line 3") != null); + try std.testing.expect(std.mem.indexOf(u8, msg, "StringValueNotValidEnumMember") != null); + // The record is echoed so the culprit is visible without opening the file. + try std.testing.expect(std.mem.indexOf(u8, msg, "security_type::stok") != null); +} + +test "deserializePortfolioDiag: a symbol-less non-cash record is reported too" { + // The second skip path. A record with no symbol that is not cash or + // illiquid has nothing to key on, and previously vanished with only + // a log line. + const allocator = std.testing.allocator; + const data = + "#!srfv1\n" ++ + "shares:num:100,open_date::2024-01-15,open_price:num:140.00\n"; + + var diags: ParseDiagnostics = .empty; + defer { + for (diags.items) |w| allocator.free(w); + diags.deinit(allocator); + } + + var p = try deserializePortfolioDiag(allocator, data, &diags); + defer p.deinit(); + + try std.testing.expectEqual(@as(usize, 0), p.lots.len); + try std.testing.expectEqual(@as(usize, 1), diags.items.len); + try std.testing.expect(std.mem.indexOf(u8, diags.items[0], "no symbol") != null); + try std.testing.expect(std.mem.indexOf(u8, diags.items[0], "line 2") != null); +} + +test "deserializePortfolioDiag: an over-long record is truncated with a marker" { + // A wide direct-indexing or option line would wrap and bury the + // message it is attached to, so the echo is clipped to + // `diag_record_cols` display columns. + const allocator = std.testing.allocator; + var long: std.ArrayList(u8) = .empty; + defer long.deinit(allocator); + try long.appendSlice(allocator, "#!srfv1\nsymbol::MSFT,security_type::stok,shares:num:50,open_date::2024-02-01,open_price:num:400.00,note::"); + try long.appendSlice(allocator, "x" ** 200); + try long.append(allocator, '\n'); + + var diags: ParseDiagnostics = .empty; + defer { + for (diags.items) |w| allocator.free(w); + diags.deinit(allocator); + } + + var p = try deserializePortfolioDiag(allocator, long.items, &diags); + defer p.deinit(); + + try std.testing.expectEqual(@as(usize, 1), diags.items.len); + const msg = diags.items[0]; + try std.testing.expect(std.mem.endsWith(u8, msg, "...")); + // Clipped well short of the 200-char note. + try std.testing.expect(msg.len < 200); +} + +test "deserializePortfolioDiag: a clean file produces no diagnostics" { + const allocator = std.testing.allocator; + const data = + "#!srfv1\n" ++ + "symbol::AAPL,shares:num:100,open_date::2024-01-15,open_price:num:140.00\n"; + + var diags: ParseDiagnostics = .empty; + defer diags.deinit(allocator); + + var p = try deserializePortfolioDiag(allocator, data, &diags); + defer p.deinit(); + + try std.testing.expectEqual(@as(usize, 1), p.lots.len); + try std.testing.expectEqual(@as(usize, 0), diags.items.len); +} + +test "deserializePortfolio: null diags keeps the old skip-and-carry-on behaviour" { + // The 16 call sites that did not opt in must be unaffected: the bad + // record is skipped, the good one survives, nothing is returned + // about it. + const data = + "#!srfv1\n" ++ + "symbol::AAPL,shares:num:100,open_date::2024-01-15,open_price:num:140.00\n" ++ + "symbol::MSFT,security_type::stok,shares:num:50,open_date::2024-02-01,open_price:num:400.00\n"; + var p = try deserializePortfolio(std.testing.allocator, data); + defer p.deinit(); + try std.testing.expectEqual(@as(usize, 1), p.lots.len); +} + +test "deserializePortfolio: null diags still logs the symbol-less skip" { + // Covers the non-collecting branch of the second skip path: a record + // with no symbol that is neither cash nor illiquid. The 16 call sites + // that never opted in must keep skipping it rather than aborting. + const prev_level = std.testing.log_level; + std.testing.log_level = .err; + defer std.testing.log_level = prev_level; + + const data = + "#!srfv1\n" ++ + "shares:num:100,open_date::2024-01-15,open_price:num:140.00\n" ++ + "symbol::AAPL,shares:num:10,open_date::2024-01-15,open_price:num:150.00\n"; + var p = try deserializePortfolio(std.testing.allocator, data); + defer p.deinit(); + try std.testing.expectEqual(@as(usize, 1), p.lots.len); + try std.testing.expectEqualStrings("AAPL", p.lots[0].symbol); +} + +test "deserializePortfolioDiag: an illiquid record with no symbol gets a placeholder" { + // Cash and illiquid are the two types allowed to omit `symbol` - + // they get a placeholder rather than being skipped. + const allocator = std.testing.allocator; + const data = + "#!srfv1\n" ++ + "security_type::illiquid,shares:num:450000,open_date::2020-06-01,open_price:num:350000\n"; + + var diags: ParseDiagnostics = .empty; + defer diags.deinit(allocator); + + var p = try deserializePortfolioDiag(allocator, data, &diags); + defer p.deinit(); + + try std.testing.expectEqual(@as(usize, 1), p.lots.len); + try std.testing.expectEqualStrings("ILLIQUID", p.lots[0].symbol); + try std.testing.expectEqual(@as(usize, 0), diags.items.len); +} + +test "nthLine: 1-based, terminator-free, out of range is empty" { + const data = "alpha\nbeta\r\ngamma"; + try std.testing.expectEqualStrings("alpha", nthLine(data, 1)); + try std.testing.expectEqualStrings("beta", nthLine(data, 2)); // \r trimmed + try std.testing.expectEqualStrings("gamma", nthLine(data, 3)); // no trailing \n + try std.testing.expectEqualStrings("", nthLine(data, 4)); + try std.testing.expectEqualStrings("", nthLine(data, 0)); +} + test "cacheKeys: directory names only, sorted, store-internal keys excluded" { const allocator = std.testing.allocator; const io = std.testing.io; diff --git a/src/commands/common.zig b/src/commands/common.zig index ed22375..c671aee 100644 --- a/src/commands/common.zig +++ b/src/commands/common.zig @@ -499,9 +499,126 @@ pub fn loadPortfolio(ctx: *framework.RunCtx, as_of: zfin.Date) ?LoadedPortfolio portfolio_loader.applySplitAdjustment(svc, ctx.allocator, &loaded, as_of, fetchOptionsFromPolicy(ctx.globals.refresh_policy)); } + // A failed write here means stdout is broken, so the command's own + // output is about to fail too. Refuse to hand back a portfolio we + // could not attach the caveat to rather than letting it be reported + // as if it were complete. + printPortfolioWarnings(ctx.out, ctx.color, loaded.warnings) catch |err| { + stderrPrint(ctx.io, "Error reporting skipped portfolio records: "); + stderrPrint(ctx.io, @errorName(err)); + stderrPrint(ctx.io, "\n"); + loaded.deinit(ctx.allocator); + return null; + }; + return loaded; } +/// Report records that were dropped during the load, before whatever the +/// command is about to print. +/// +/// This is the one place it happens. All 16 CLI entry points reach a live +/// portfolio through `loadPortfolio`, so a single call there covers them +/// without each command remembering to. It deliberately is NOT called +/// from `portfolio_loader`: `contributions` loads two historical +/// revisions through the same loader, and warnings about a months-old +/// commit would be noise rather than news. +/// +/// Written to the command's writer rather than `std.log`, because the +/// point is to be read. A dropped lot silently lowers net worth, +/// allocation percentages and every derived figure; a stderr line is easy +/// to miss with stdout piped or a screen of tables following it. Hence +/// the closing sentence naming the consequence rather than just a count. +/// +/// Takes the writer and color flag rather than a `RunCtx` so the exact +/// wording can be asserted in a test - this output is the whole point of +/// collecting the warnings, so it should not be the untested part. +fn printPortfolioWarnings(out: *std.Io.Writer, color: bool, warnings: []const []const u8) !void { + if (warnings.len == 0) return; + + try setFg(out, color, CLR_WARNING); + try out.print("warning: {d} record(s) skipped while reading the portfolio\n", .{warnings.len}); + try reset(out, color); + for (warnings) |w| { + try setFg(out, color, CLR_MUTED); + try out.print(" {s}\n", .{w}); + try reset(out, color); + } + try setFg(out, color, CLR_WARNING); + try out.writeAll(" Figures below exclude them.\n\n"); + try reset(out, color); +} + +test "printPortfolioWarnings: nothing to report writes nothing" { + var aw: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer aw.deinit(); + try printPortfolioWarnings(&aw.writer, false, &.{}); + try std.testing.expectEqualStrings("", aw.written()); +} + +test "printPortfolioWarnings: names the count and states the consequence" { + // The consequence line is the reason this is in band at all: the + // count alone does not tell the user their totals are short. + var aw: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer aw.deinit(); + const warnings = [_][]const u8{ + "portfolio.srf: line 3: StringValueNotValidEnumMember\n symbol::MSFT,security_type::stok", + "portfolio_closed.srf: line 9: no symbol\n shares:num:5", + }; + try printPortfolioWarnings(&aw.writer, false, &warnings); + const text = aw.written(); + + try std.testing.expect(std.mem.startsWith(u8, text, "warning: 2 record(s) skipped while reading the portfolio\n")); + try std.testing.expect(std.mem.indexOf(u8, text, "portfolio.srf: line 3: StringValueNotValidEnumMember") != null); + try std.testing.expect(std.mem.indexOf(u8, text, "portfolio_closed.srf: line 9: no symbol") != null); + try std.testing.expect(std.mem.endsWith(u8, text, " Figures below exclude them.\n\n")); +} + +test "loadWatchlist: valid symbols load, a malformed record is skipped" { + // `loadWatchlist` had no coverage at all. A record missing `symbol` + // fails coercion and must be skipped rather than costing the rest of + // the watchlist. + const prev_level = std.testing.log_level; + std.testing.log_level = .err; + defer std.testing.log_level = prev_level; + + const io = std.testing.io; + const allocator = std.testing.allocator; + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + try tmp.dir.writeFile(io, .{ .sub_path = "watchlist.srf", .data = + \\#!srfv1 + \\symbol::AAPL + \\note::no symbol here + \\symbol::MSFT + \\ + }); + + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const dir_len = try tmp.dir.realPathFile(io, ".", &path_buf); + const path = try std.fs.path.join(allocator, &.{ path_buf[0..dir_len], "watchlist.srf" }); + defer allocator.free(path); + + const syms = loadWatchlist(io, allocator, path) orelse { + try std.testing.expect(false); + return; + }; + defer { + for (syms) |sym| allocator.free(sym); + allocator.free(syms); + } + + try std.testing.expectEqual(@as(usize, 2), syms.len); + try std.testing.expectEqualStrings("AAPL", syms[0]); + try std.testing.expectEqualStrings("MSFT", syms[1]); +} + +test "loadWatchlist: a missing file is null, not an error" { + const allocator = std.testing.allocator; + try std.testing.expect(loadWatchlist(std.testing.io, allocator, "does_not_exist_watchlist.srf") == null); +} + // ── As-of date parsing (shared by CLI --as-of and TUI date popup) ── pub const AsOfParseError = error{ @@ -1008,7 +1125,16 @@ pub fn loadWatchlist(io: std.Io, allocator: std.mem.Allocator, path: []const u8) var syms: std.ArrayList([]const u8) = .empty; while (it.next() catch null) |fields| { - const entry = fields.to(WatchEntry, srf_opts.user_edited) catch continue; + const entry = fields.to(WatchEntry, srf_opts.user_edited) catch |err| { + // Skip the entry rather than losing the whole watchlist, but + // name the error - otherwise a symbol just stops appearing. + // Quiet under `zig build test`, where fixtures feed + // malformed records on purpose. + if (!builtin.is_test) { + std.log.warn("watchlist.srf: skipping malformed record: {s}", .{@errorName(err)}); + } + continue; + }; const duped = allocator.dupe(u8, entry.symbol) catch continue; syms.append(allocator, duped) catch { allocator.free(duped); diff --git a/src/data/imported_values.zig b/src/data/imported_values.zig index f838a54..17d7da9 100644 --- a/src/data/imported_values.zig +++ b/src/data/imported_values.zig @@ -177,7 +177,12 @@ pub fn parseImportedValues( errdefer points.deinit(allocator); while (it.next() catch return error.InvalidSrf) |fields| { - const point = fields.to(HistoryPoint, srf_opts.user_edited) catch return error.InvalidSrf; + // Aborts rather than skipping: this is a small curated series, + // so one unparseable row means the transcription is suspect and + // a partial series would be worse than none. The error passes + // through instead of collapsing to `InvalidSrf`, so the caller + // can report WHY it failed rather than just that it did. + const point = try fields.to(HistoryPoint, srf_opts.user_edited); try points.append(allocator, point); } diff --git a/src/models/classification.zig b/src/models/classification.zig index dd73c4c..6687a20 100644 --- a/src/models/classification.zig +++ b/src/models/classification.zig @@ -10,10 +10,13 @@ /// symbol::02315N600,asset_class::International Developed,pct:num:20 /// symbol::02315N600,asset_class::Bonds,pct:num:15 const std = @import("std"); +const builtin = @import("builtin"); const srf = @import("srf"); const srf_opts = @import("../srf_opts.zig"); const Date = @import("../Date.zig"); +const log = std.log.scoped(.metadata); + /// A single classification entry for a symbol. pub const ClassificationEntry = struct { symbol: []const u8, @@ -90,7 +93,16 @@ pub fn parseClassificationFile(allocator: std.mem.Allocator, data: []const u8) ! defer it.deinit(); while (try it.next()) |fields| { - const entry = fields.to(ClassificationEntry, srf_opts.user_edited) catch continue; + const entry = fields.to(ClassificationEntry, srf_opts.user_edited) catch |err| { + // Skip the row rather than losing the whole file, but name + // the error: a silently dropped row quietly changes a + // breakdown's percentages. Quiet under `zig build test`, + // where fixtures feed malformed rows on purpose. + if (!builtin.is_test) { + log.warn("metadata.srf: skipping malformed record: {s}", .{@errorName(err)}); + } + continue; + }; // Pre-fill `bucket` if the user didn't curate one. This // shifts the cost of `deriveBucket` to parse time and // makes downstream code free to read `entry.bucket` @@ -288,6 +300,28 @@ test "parse classification file: a hand-typed string separator on pct still pars try std.testing.expectApproxEqAbs(@as(f64, 40), map.entries[1].pct, 0.001); } +test "parse classification file: a record missing symbol is skipped, not fatal" { + // `symbol` has no default, so a record without it fails coercion. + // One bad row must not cost the whole file - every other symbol's + // sector and geo would silently vanish from the breakdowns. + const prev_level = std.testing.log_level; + std.testing.log_level = .err; + defer std.testing.log_level = prev_level; + + const data = + \\#!srfv1 + \\symbol::AAPL,sector::Technology + \\sector::Healthcare,pct:num:100 + \\symbol::MSFT,sector::Technology + ; + var map = try parseClassificationFile(std.testing.allocator, data); + defer map.deinit(); + + try std.testing.expectEqual(@as(usize, 2), map.entries.len); + try std.testing.expectEqualStrings("AAPL", map.entries[0].symbol); + try std.testing.expectEqualStrings("MSFT", map.entries[1].symbol); +} + test "parse classification file: bucket round-trips" { const data = \\#!srfv1 diff --git a/src/portfolio_loader.zig b/src/portfolio_loader.zig index aa36208..3aa19c6 100644 --- a/src/portfolio_loader.zig +++ b/src/portfolio_loader.zig @@ -90,6 +90,16 @@ pub const LoadedPortfolio = struct { portfolio: zfin.Portfolio, positions: []const zfin.Position, syms: []const []const u8, + /// One message per record that could not be parsed and was + /// therefore left out of `portfolio`. Empty on a clean load. + /// + /// A dropped lot silently changes every figure derived from this + /// portfolio, so these travel with the load rather than going only + /// to `std.log`: whoever prints the figures can print the caveat + /// beside them. `commands.common.loadPortfolio` does exactly that. + /// Each message is prefixed with the file it came from, since the + /// load is a union over the whole `portfolio*.srf` glob. Owned. + warnings: []const []const u8 = &.{}, pub fn deinit(self: *LoadedPortfolio, allocator: std.mem.Allocator) void { allocator.free(self.syms); @@ -97,6 +107,8 @@ pub const LoadedPortfolio = struct { self.portfolio.deinit(); for (self.file_datas) |d| allocator.free(d); allocator.free(self.file_datas); + for (self.warnings) |w| allocator.free(w); + allocator.free(self.warnings); // Path-string ownership: `resolved_paths` (if present) owns // the underlying path strings. The `paths` slice is the // borrowed view; free only its outer storage. @@ -387,6 +399,11 @@ fn loadFromBytes( var lots_owner: LotsOwner = .merged_list; var success = false; + // Per-record parse diagnostics, accumulated across every file in the + // glob. Handed to the caller on success so it can report alongside + // the figures the missing lots have silently changed. + var warnings: zfin.cache.ParseDiagnostics = .empty; + defer if (!success) { switch (lots_owner) { .merged_list => { @@ -403,6 +420,8 @@ fn loadFromBytes( .combined_struct => combined.deinit(), .none => {}, } + for (warnings.items) |w| allocator.free(w); + warnings.deinit(allocator); for (file_datas_owned) |d| allocator.free(d); allocator.free(file_datas_owned); allocator.free(paths_owned); @@ -422,12 +441,36 @@ fn loadFromBytes( // without trying to parse. if (data.len == 0) continue; - var portfolio = zfin.cache.deserializePortfolio(allocator, data) catch { + // Diagnostics are gathered per file so each message can name + // the file it came from - the load is a union over the glob, and + // "line 183" is ambiguous across several portfolio files. + var file_diags: zfin.cache.ParseDiagnostics = .empty; + defer { + for (file_diags.items) |w| allocator.free(w); + file_diags.deinit(allocator); + } + + var portfolio = zfin.cache.deserializePortfolioDiag(allocator, data, &file_diags) catch { var msg_buf: [512]u8 = undefined; const msg = std.fmt.bufPrint(&msg_buf, "Error: Cannot parse portfolio file: {s}\n", .{paths_owned[idx]}) catch "Error: Cannot parse portfolio file\n"; stderr.print(io, msg); return null; }; + + for (file_diags.items) |raw_msg| { + const named = std.fmt.allocPrint(allocator, "{s}: {s}", .{ + std.fs.path.basename(paths_owned[idx]), + raw_msg, + }) catch { + portfolio.deinit(); + return null; + }; + warnings.append(allocator, named) catch { + allocator.free(named); + portfolio.deinit(); + return null; + }; + } for (portfolio.lots) |lot| { merged.append(allocator, lot) catch { portfolio.deinit(); @@ -457,6 +500,12 @@ fn loadFromBytes( return null; }; + const warnings_owned = warnings.toOwnedSlice(allocator) catch { + allocator.free(syms); + allocator.free(positions); + return null; + }; + success = true; return .{ .paths = paths_owned, @@ -465,6 +514,7 @@ fn loadFromBytes( .portfolio = combined, .positions = positions, .syms = syms, + .warnings = warnings_owned, }; } @@ -778,6 +828,67 @@ test "loadFromBytes: union of two synthetic SRF files" { try testing.expectEqualStrings("MSFT", loaded.portfolio.lots[1].symbol); } +test "loadFromBytes: a dropped record is reported and names its file" { + // The union spans several files, so "line 3" alone is ambiguous - + // each message carries the basename it came from. Only the second + // file has a bad record here, and only it should be named. + const allocator = testing.allocator; + + const file_a = + \\#!srfv1 + \\symbol::AAPL,shares:num:10,open_date::2024-01-15,open_price:num:150,account::Sample IRA + \\ + ; + const file_b = + \\#!srfv1 + \\symbol::MSFT,shares:num:5,open_date::2024-02-01,open_price:num:300,account::Sample Roth + \\symbol::NVDA,security_type::stok,shares:num:1,open_date::2024-03-01,open_price:num:900 + \\ + ; + + const paths = try allocator.dupe([]const u8, &.{ "portfolio.srf", "portfolio_closed.srf" }); + const datas = try dupeBytes(allocator, &.{ file_a, file_b }); + + var loaded = loadFromBytes(testing.io, allocator, paths, null, datas, zfin.Date.fromYmd(2026, 5, 23)) orelse { + try testing.expect(false); + return; + }; + defer loaded.deinit(allocator); + + // Two good lots merged; the bad one dropped rather than fatal. + try testing.expectEqual(@as(usize, 2), loaded.portfolio.lots.len); + + try testing.expectEqual(@as(usize, 1), loaded.warnings.len); + const w = loaded.warnings[0]; + try testing.expect(std.mem.startsWith(u8, w, "portfolio_closed.srf: ")); + try testing.expect(std.mem.indexOf(u8, w, "line 3") != null); + try testing.expect(std.mem.indexOf(u8, w, "StringValueNotValidEnumMember") != null); + try testing.expect(std.mem.indexOf(u8, w, "security_type::stok") != null); +} + +test "loadFromBytes: a clean load reports nothing" { + // Guards against the warnings slice being non-empty (or unfreed) on + // the happy path - `testing.allocator` catches the leak either way. + const allocator = testing.allocator; + + const file_a = + \\#!srfv1 + \\symbol::AAPL,shares:num:10,open_date::2024-01-15,open_price:num:150,account::Sample IRA + \\ + ; + + const paths = try allocator.dupe([]const u8, &.{"portfolio.srf"}); + const datas = try dupeBytes(allocator, &.{file_a}); + + var loaded = loadFromBytes(testing.io, allocator, paths, null, datas, zfin.Date.fromYmd(2026, 5, 23)) orelse { + try testing.expect(false); + return; + }; + defer loaded.deinit(allocator); + + try testing.expectEqual(@as(usize, 0), loaded.warnings.len); +} + test "loadFromBytes: single file with valid contents" { const allocator = testing.allocator;