From 9391d8d6defee376dd5a312eee46390d4907faf6 Mon Sep 17 00:00:00 2001 From: Kybxd <627940450@qq.com> Date: Thu, 18 Jun 2026 14:59:35 +0800 Subject: [PATCH 1/3] fix(confgen): preserve sub-table book/sheet name in merger errors 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. --- internal/confgen/parser.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/confgen/parser.go b/internal/confgen/parser.go index 15f5e67b..f26bde67 100644 --- a/internal/confgen/parser.go +++ b/internal/confgen/parser.go @@ -158,17 +158,21 @@ func ParseMessage(info *SheetInfo, collector *xerrors.Collector, impInfos ...imp for _, impInfo := range impInfos { // map-reduce: map jobs for concurrent processing g.Go(func(ctx context.Context) error { + 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) } mu.Lock() msgs = append(msgs, oneMsg{ protomsg: protomsg, - bookName: getRelBookName(info.ExtInfo.InputDir, impInfo.Filename()), - sheetName: getRealSheetName(info, impInfo), + bookName: bookName, + sheetName: sheetName, }) mu.Unlock() return nil From cc1ef1ef87b1e6e373602ca0f3edb49363dca3ae Mon Sep 17 00:00:00 2001 From: Kybxd <627940450@qq.com> Date: Thu, 18 Jun 2026 15:20:27 +0800 Subject: [PATCH 2/3] test(confgen): add merger collector integration test for error attribution 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. --- .../confgen/collector_integration_test.go | 59 +++++++++++++++++++ .../csv/merger/MergerCollector#@TABLEAU.csv | 2 + ...ergerCollector#MergerCollectorItemConf.csv | 5 ++ .../MergerShard1#MergerCollectorItemConf.csv | 4 ++ .../collector_merger/proto/merger.proto | 31 ++++++++++ 5 files changed, 101 insertions(+) create mode 100644 internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#@TABLEAU.csv create mode 100644 internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#MergerCollectorItemConf.csv create mode 100644 internal/confgen/testdata/collector_merger/csv/merger/MergerShard1#MergerCollectorItemConf.csv create mode 100644 internal/confgen/testdata/collector_merger/proto/merger.proto diff --git a/internal/confgen/collector_integration_test.go b/internal/confgen/collector_integration_test.go index 06769d0f..a06a3e56 100644 --- a/internal/confgen/collector_integration_test.go +++ b/internal/confgen/collector_integration_test.go @@ -181,3 +181,62 @@ func TestCollectorIntegration_MultiBookCapped(t *testing.T) { assert.Contains(t, got, e2012("Collector2#*.csv", "HeroConf", cells[i], v, "uint32")) } } + +// TestCollectorIntegration_MergerSubtableBookName verifies that when a +// SHARD sub-table fails to parse inside the ParseMessage merger goroutine, +// the rendered error reports the offending shard's own BookName/SheetName, +// and additionally annotates the primary main workbook as "(Primary: ...)". +// +// Layout: +// - main workbook "MergerCollector#*.csv" / sheet MergerCollectorItemConf +// contains only valid rows. +// - shard workbook "MergerShard1#*.csv" / sheet MergerCollectorItemConf +// contains one invalid Num cell ("bad_num") that triggers E2012. +// +// The shard is merged via `merger: "MergerShard*.csv#MergerCollectorItemConf"` +// in merger.proto. parseMessageFromOneImporter fails on the shard, the +// merger goroutine WrapKV's the error with both the shard names AND the +// primary names; the fix ensures the shard names survive into the final +// error description (regression: previously only the primary names were +// kept and the user could not tell which shard actually broke). +// +// Note: uses a separate proto package + testdata tree so its workbook +// options don't perturb the other TestCollectorIntegration_* tests above. +func TestCollectorIntegration_MergerSubtableBookName(t *testing.T) { + const mergerProtoDir = "./testdata/collector_merger/proto" + gen := NewGenerator("collectormergertest", + "./testdata/collector_merger/csv/merger/", + "./testdata/_collector_merger_out/", + options.Conf( + &options.ConfOption{ + Input: &options.ConfInputOption{ + ProtoPaths: []string{mergerProtoDir}, + ProtoFiles: []string{mergerProtoDir + "/*.proto"}, + Formats: []format.Format{format.CSV}, + }, + Output: &options.ConfOutputOption{ + Formats: []format.Format{format.JSON}, + }, + }, + ), + ) + err := gen.Generate("MergerCollector#MergerCollectorItemConf.csv") + require.Error(t, err) + + got := err.Error() + t.Logf("got error string:\n%s", got) + + // The offending cell ("bad_num") must be reported against the SHARD + // workbook, not the main workbook. + assert.Contains(t, got, "Workbook: MergerShard1#*.csv", + "shard BookName must appear in the rendered error") + assert.Contains(t, got, "Worksheet: MergerCollectorItemConf", + "shard SheetName must appear in the rendered error") + // And the main workbook must be annotated as Primary, proving both + // (BookName, PrimaryBookName) survived WrapKV layering. + assert.Contains(t, got, "(Primary: MergerCollector#*.csv)", + "primary BookName must be annotated alongside the shard BookName") + // The error must NOT point at the main workbook as the offending one. + assert.NotContains(t, got, "Workbook: MergerCollector#*.csv ", + "main workbook must not be reported as the offending file") +} diff --git a/internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#@TABLEAU.csv b/internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#@TABLEAU.csv new file mode 100644 index 00000000..16ee2490 --- /dev/null +++ b/internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#@TABLEAU.csv @@ -0,0 +1,2 @@ +Sheet,Merger +MergerCollectorItemConf,"MergerShard*.csv#MergerCollectorItemConf" diff --git a/internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#MergerCollectorItemConf.csv b/internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#MergerCollectorItemConf.csv new file mode 100644 index 00000000..5100d53c --- /dev/null +++ b/internal/confgen/testdata/collector_merger/csv/merger/MergerCollector#MergerCollectorItemConf.csv @@ -0,0 +1,5 @@ +ID,Num +uint32,int32 +Item's ID,Item's num +1,10 +2,20 diff --git a/internal/confgen/testdata/collector_merger/csv/merger/MergerShard1#MergerCollectorItemConf.csv b/internal/confgen/testdata/collector_merger/csv/merger/MergerShard1#MergerCollectorItemConf.csv new file mode 100644 index 00000000..2998ecb5 --- /dev/null +++ b/internal/confgen/testdata/collector_merger/csv/merger/MergerShard1#MergerCollectorItemConf.csv @@ -0,0 +1,4 @@ +ID,Num +uint32,int32 +Item's ID,Item's num +3,bad_num diff --git a/internal/confgen/testdata/collector_merger/proto/merger.proto b/internal/confgen/testdata/collector_merger/proto/merger.proto new file mode 100644 index 00000000..40ab4d10 --- /dev/null +++ b/internal/confgen/testdata/collector_merger/proto/merger.proto @@ -0,0 +1,31 @@ +// clang-format off + +syntax = "proto3"; + +package collectormergertest; + +option (tableau.workbook) = {name: "MergerCollector#*.csv"}; + +import "tableau/protobuf/tableau.proto"; + +// MergerCollectorItemConf merges the main workbook with one or more shard +// workbooks matching "MergerShard*.csv#MergerCollectorItemConf". It is used +// to verify that, when a shard sub-table fails to parse, the collected +// error reports the offending shard's BookName/SheetName instead of the +// primary main workbook's names. +message MergerCollectorItemConf { + option (tableau.worksheet) = { + name: "MergerCollectorItemConf" + namerow: 1 + typerow: 2 + noterow: 3 + datarow: 4 + merger: "MergerShard*.csv#MergerCollectorItemConf" + }; + + map item_map = 1 [(tableau.field) = {key:"ID" layout:LAYOUT_VERTICAL}]; + message Item { + uint32 id = 1 [(tableau.field) = {name:"ID"}]; + int32 num = 2 [(tableau.field) = {name:"Num"}]; + } +} From 9a93349e5fb39d840b6d547405f9856f771a4424 Mon Sep 17 00:00:00 2001 From: Kybxd <627940450@qq.com> Date: Thu, 18 Jun 2026 15:45:51 +0800 Subject: [PATCH 3/3] fix(confgen): include book and sheet in error context --- .github/workflows/buf-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/buf-ci.yml b/.github/workflows/buf-ci.yml index ec0885a3..f240c743 100644 --- a/.github/workflows/buf-ci.yml +++ b/.github/workflows/buf-ci.yml @@ -19,5 +19,6 @@ jobs: - uses: actions/checkout@v6 - uses: bufbuild/buf-action@v1 with: + version: "1.69.0" token: ${{ secrets.BUF_TOKEN }} github_token: ${{ secrets.GITHUB_TOKEN }}