Skip to content

fix(confgen): preserve sub-table book/sheet name in merger errors#420

Merged
Kybxd merged 3 commits into
masterfrom
primary-book-sheet-name
Jun 23, 2026
Merged

fix(confgen): preserve sub-table book/sheet name in merger errors#420
Kybxd merged 3 commits into
masterfrom
primary-book-sheet-name

Conversation

@Kybxd

@Kybxd Kybxd commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

When a merger sub-table fails to parse, error messages incorrectly attributed the failure to the primary book/sheet instead of the offending sub-table. This PR fixes the attribution and adds an integration test to lock the behavior in.

Problem

ParseMessage runs parseMessageFromOneImporter for each merger importer in a goroutine. When one fails, the returned error was wrapped only with PrimaryBookName / PrimarySheetName:

return xerrors.WrapKV(err,
    xerrors.KeyPrimaryBookName, info.PrimaryBookName,
    xerrors.KeyPrimarySheetName, info.SheetOpts.Name)

Collector retains only the outermost withMessage as outerWM, so this outer WrapKV overwrote the inner layer that carried the actual sub-table's BookName / SheetName. The final NewDesc therefore pointed at the primary table rather than the shard that actually failed.

Fix

Attach the sub-table's BookName / SheetName on the same outer WrapKV layer alongside the primary names, so all four fields survive to NewDesc:

bookName := getRelBookName(info.ExtInfo.InputDir, impInfo.Filename())
sheetName := getRealSheetName(info, impInfo)
protomsg, err := parseMessageFromOneImporter(info, collector, impInfo)
if err != nil {
    return xerrors.WrapKV(err,
        xerrors.KeyBookName, bookName,
        xerrors.KeySheetName, sheetName,
        xerrors.KeyPrimaryBookName, info.PrimaryBookName,
        xerrors.KeyPrimarySheetName, info.SheetOpts.Name)
}

bookName / sheetName are now computed once and reused for both the error path and the success-path oneMsg.

Test

Added internal/confgen/testdata/collector_merger/ containing:

  • proto/merger.proto — a workbook (MergerCollector#*.csv) with one sheet whose merger option pulls MergerShard*.csv#MergerCollectorItemConf.
  • csv/merger/MergerCollector#@TABLEAU.csv, MergerCollector#MergerCollectorItemConf.csv, MergerShard1#MergerCollectorItemConf.csv — fixtures triggering a parse error in the shard.

collector_integration_test.go asserts the error description contains the shard book/sheet (MergerShard1 / MergerCollectorItemConf) rather than only the primary book/sheet.

Why a separate top-level testdata dir?

buildWorkbookIndex resolves merger shard globs in the test's inputDir. If merger.proto lived under the shared collector/proto/ directory, the normal/ and overflow/ test suites — which reuse the same collectortest proto package — would fail to resolve MergerShard*.csv in their own inputDirs. An isolated collector_merger/ dir with its own proto package avoids polluting those suites; the trade-off is one extra top-level testdata directory.

Risk

Low. Pure additive on the error path (extra KV fields) plus new test fixtures. No behavior change on the success path beyond hoisting two local variables.

@github-actions

github-actions Bot commented Jun 18, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJun 18, 2026, 7:46 AM

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.94%. Comparing base (1743f92) to head (9a93349).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   74.87%   74.94%   +0.07%     
==========================================
  Files          88       88              
  Lines        9384     9388       +4     
==========================================
+ Hits         7026     7036      +10     
+ Misses       1785     1781       -4     
+ Partials      573      571       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Kybxd added 3 commits June 18, 2026 15:45
When parseMessageFromOneImporter fails inside the ParseMessage merger goroutine, only PrimaryBookName/PrimarySheetName were attached to the wrapped error. Because Collector keeps just the outermost withMessage as outerWM, this overwrote the inner WrapKV layer that carried the actual sub-table's BookName/SheetName, so error messages pointed at the primary table instead of the offending sub-table.

Attach the sub-table's BookName/SheetName on the same outer WrapKV layer alongside the primary names, so all four fields survive into the final NewDesc.
…ution

Add isolated testdata under internal/confgen/testdata/collector_merger/
to verify error origin attribution in merger flow (shard CSV + main book
with merger option). Kept in a separate top-level dir to avoid polluting
the shared collectortest proto package consumed by normal/overflow suites
via buildWorkbookIndex shard resolution.
@Kybxd Kybxd force-pushed the primary-book-sheet-name branch from d223100 to 9a93349 Compare June 18, 2026 07:46
@Kybxd Kybxd merged commit 17a0e8e into master Jun 23, 2026
9 checks passed
@Kybxd Kybxd deleted the primary-book-sheet-name branch June 23, 2026 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants