diff --git a/internal/x/xerrors/collector.go b/internal/x/xerrors/collector.go index 524f4c8b..cbeee11d 100644 --- a/internal/x/xerrors/collector.go +++ b/internal/x/xerrors/collector.go @@ -15,17 +15,17 @@ import ( // Collectors form a hierarchy via [Collector.NewChild]; [Collector.Join] // recursively merges own errors with children's. // -// When a child's [collected] error is wrapped (e.g. via [WrapKV]), the -// outer [withMessage] is remembered and re-applied in [Collector.Join], -// producing: collected → withMessage{fields} → joinError{…}. +// When a child's [collected] error is wrapped (e.g. via [WrapKV]), the scope +// fields of the wrapper layers are remembered and re-applied in +// [Collector.Join], producing: collected → withMessage{fields} → joinError{…}. type Collector struct { - mu sync.Mutex - errs []error - children []*Collector - counter atomic.Int32 - maxErrs int32 - parent *Collector - outerWM *withMessage // outer WrapKV layer, re-applied in Join() + mu sync.Mutex + errs []error + children []*Collector + counter atomic.Int32 + maxErrs int32 + parent *Collector + outerFields map[string]any // scope fields of the outer wrappers, re-applied in Join() } // NewCollector creates a root Collector. @@ -59,8 +59,8 @@ func normalizeMax(maxErrs int) int32 { // // - nil err: no-op unless this collector (or an ancestor) is already full, // in which case the joined error tree is returned immediately. -// - already-collected err (*collected): records the outer WrapKV layer for -// re-wrapping in Join; does not increment any counter. +// - already-collected err (*collected): records the outer wrapper layers' +// scope fields for re-wrapping in Join; does not increment any counter. // - ordinary err: increments every ancestor's counter, stores the error if // it is within every ancestor's budget, and returns the joined error tree // if any ancestor has now reached its limit (nil otherwise). @@ -75,8 +75,11 @@ func (c *Collector) Collect(err error) error { // Already-collected error (from a Join() somewhere): if the collected // error originates from the same collector tree as c (shares the same // root), it is or will be reachable from the root via tree auto-join, so - // we must not double-count it here. We only record the outer WrapKV - // layer on the originating collector for re-wrapping in its Join(). + // we must not double-count it here. We only record the outer wrapper + // layers' scope fields on the originating collector for re-wrapping in + // its Join(). The assignment is unconditional: a wrapper carrying no + // scope fields must clear the previously recorded ones instead of + // leaving them to be re-applied to this unrelated join. // // Otherwise (foreign collected, e.g. from an unrelated parser-local // fail-fast collector that is not part of c's tree), fall through and @@ -84,11 +87,10 @@ func (c *Collector) Collect(err error) error { // this collector; without this fallback the error would silently vanish. var ce *collected if errors.As(err, &ce) && ce.origin != nil && ce.origin.sameTreeAs(c) { - if wm, ok := err.(*withMessage); ok { - ce.origin.mu.Lock() - ce.origin.outerWM = wm - ce.origin.mu.Unlock() - } + fields := outerScopeFields(err, ce) + ce.origin.mu.Lock() + ce.origin.outerFields = fields + ce.origin.mu.Unlock() if c.IsFull() { return c.Join() } @@ -162,7 +164,7 @@ func (c *Collector) Join() error { copy(ownErrs, c.errs) kids := make([]*Collector, len(c.children)) copy(kids, c.children) - outerWM := c.outerWM + outerFields := c.outerFields c.mu.Unlock() var nonNil []error @@ -180,9 +182,9 @@ func (c *Collector) Join() error { return nil } var inner error = &joinError{errs: nonNil, stack: callers(1)} - // Re-wrap with outer WrapKV fields if present. - if outerWM != nil { - inner = &withMessage{cause: inner, fields: outerWM.fields} + // Re-wrap with the recorded outer scope fields if present. + if len(outerFields) > 0 { + inner = &withMessage{cause: inner, fields: outerFields} } return &collected{ error: inner, @@ -190,6 +192,39 @@ func (c *Collector) Join() error { } } +// outerScopeFields merges the [scopeKeys] fields of every wrapper layer between +// err and the collected marker ce, with inner layers winning on key conflicts. +// Returns nil if no layer carries one. +// +// All intermediate layers must be walked, not just the outermost one: a wrapper +// chain often spreads its fields across several layers (e.g. confgen adds +// Module in one layer and BookName/SheetName in another), so keeping only the +// outermost layer would silently drop the rest. +// +// Non-scope fields are skipped because Join() re-applies these to the whole +// subtree: a cell position or field name belongs to a single error, and +// broadcasting it would point sibling errors at a cell they never touched. +func outerScopeFields(err error, ce *collected) map[string]any { + var fields map[string]any + for cur := err; cur != nil && cur != error(ce); cur = errors.Unwrap(cur) { + fc, ok := cur.(fieldsCarrier) + if !ok { + continue + } + // Walking outer -> inner, so inner layers win. + for k, v := range fc.Fields() { + if !scopeKeys[k] { + continue + } + if fields == nil { + fields = make(map[string]any) + } + fields[k] = v + } + } + return fields +} + // collected marks an error as already joined. Delegates to the inner error. type collected struct { error @@ -198,6 +233,16 @@ type collected struct { func (c *collected) Error() string { return c.error.Error() } func (c *collected) Unwrap() error { return c.error } + +// renderWithFields implements [fieldsRenderer], so that outer fields (e.g. +// Module, BookName, SheetName added by an enclosing [WrapKV]) are propagated +// into the joined children instead of being dropped. Without this, rendering +// falls back to the inner error's Error(), which loses the outer fields and +// thus renders the default message template rather than the module-specific +// one (e.g. confgen). +func (c *collected) renderWithFields(outerFields map[string]any) string { + return renderCause(c.error, outerFields) +} func (c *collected) Format(s fmt.State, verb rune) { if f, ok := c.error.(fmt.Formatter); ok { f.Format(s, verb) diff --git a/internal/x/xerrors/collector_test.go b/internal/x/xerrors/collector_test.go index 03118b07..9ac093e1 100644 --- a/internal/x/xerrors/collector_test.go +++ b/internal/x/xerrors/collector_test.go @@ -726,9 +726,9 @@ func TestCollected_CollectForeignWrappedCollectedIsNotDropped(t *testing.T) { assert.Contains(t, got.Error(), "err 1") } -// Collecting a WrapKV'd same-tree collected error records the outer WrapKV -// layer on the originating collector so Join() can re-apply the fields. -func TestCollected_CollectSameTreeWrappedCollectedRecordsOuterWM(t *testing.T) { +// Collecting a WrapKV'd same-tree collected error records the outer wrapper's +// scope fields on the originating collector so Join() can re-apply them. +func TestCollected_CollectSameTreeWrappedCollectedRecordsOuterFields(t *testing.T) { root := NewCollector(10) child := root.NewChild(0) _ = child.Collect(fmt.Errorf("err 1")) @@ -741,8 +741,8 @@ func TestCollected_CollectSameTreeWrappedCollectedRecordsOuterWM(t *testing.T) { // and the outer WrapKV layer is recorded on child (origin) for Join(). _ = root.Collect(wrapped) - if assert.NotNil(t, child.outerWM, "outer WrapKV layer should be recorded on the originating collector") { - assert.Equal(t, "test.xlsx", child.outerWM.fields[KeyBookName]) + if assert.NotNil(t, child.outerFields, "outer scope fields should be recorded on the originating collector") { + assert.Equal(t, "test.xlsx", child.outerFields[KeyBookName]) } // child.Join() re-applies the recorded fields, surfacing them in its Desc. @@ -787,6 +787,148 @@ func TestCollected_ErrorDelegates(t *testing.T) { assert.Contains(t, joined.Error(), "hello") } +// collected marker is transparent to field propagation: outer WrapKV fields +// must reach the joined children when rendering via Error(), so the +// module-specific template (confgen) is used instead of the default one. +// +// This mirrors the real confgen chain: +// +// parseFieldValue -> E2002 +// tableParser.Parse -> WrapKV(DataCellPos/DataCell/ColumnName) +// sheetCollector.Join -> collected{withMessage{joinError}} +// sheetParser.Parse -> WrapKV(Module) +// parseMessageFromOneImporter -> WrapKV(Module/BookName/SheetName) +func TestCollected_ErrorPropagatesOuterFields(t *testing.T) { + cellErr := WrapKV(E2002("100033333", "ItemConf.ID"), + KeyDataCellPos, "F12", + KeyDataCell, "100033333", + KeyColumnName, "ItemID", + ) + + child := NewCollector(10).NewChild(5) + _ = child.Collect(cellErr) + + err := WrapKV(WrapKV(child.Join(), KeyModule, ModuleConf), + KeyModule, ModuleConf, + KeyBookName, "Activity.xlsx", + KeySheetName, "SectionConf", + ) + + want := `error[E2002]: field value not in referred space +Workbook: Activity.xlsx +Worksheet: SectionConf +DataCellPos: F12 +DataCell: 100033333 +Reason: value "100033333" not in referred space "ItemConf.ID" +Help: guarantee value "100033333" was configured in referred space "ItemConf.ID" ahead +` + assert.Equal(t, want, err.Error()) +} + +// Re-collecting an already-collected same-tree error must preserve the fields +// of every wrapper layer, not just the outermost one. +// +// This mirrors the real confgen chain of a merger/scatter sheet, where Module +// and BookName/SheetName are added by different layers: +// +// tableParser.Parse -> sheetCollector.Join() == collected +// sheetParser.Parse -> WrapKV(Module) +// parseMessageFromOneImporter -> WrapKV(Module/BookName/SheetName/PBMessage) +// ParseMessage's Group.Go -> WrapKV(BookName/SheetName/Primary*) <- outermost, no Module +// Group.Wait -> bookCollector.Join() +func TestCollected_ReCollectPreservesAllWrapperFields(t *testing.T) { + cellErr := WrapKV(E2002("100033333", "ItemConf.ID"), + KeyDataCellPos, "F12", + KeyDataCell, "100033333", + ) + + book := NewCollector(10) + sheet := book.NewChild(5) + _ = sheet.Collect(cellErr) + + // Module is added by an intermediate layer, while the outermost layer only + // carries book/sheet names. + err := WrapKV(WrapKV(sheet.Join(), + KeyModule, ModuleConf, + KeyBookName, "Activity.xlsx", + KeySheetName, "SectionConf", + ), KeyPrimaryBookName, "Activity.xlsx", KeyPrimarySheetName, "SectionConf") + _ = book.Collect(err) + + want := `error[E2002]: field value not in referred space +Workbook: Activity.xlsx +Worksheet: SectionConf +DataCellPos: F12 +DataCell: 100033333 +Reason: value "100033333" not in referred space "ItemConf.ID" +Help: guarantee value "100033333" was configured in referred space "ItemConf.ID" ahead +` + assert.Equal(t, want, book.Join().Error()) +} + +// Only scope fields (which book/sheet/message) may be broadcast to the joined +// children; per-cell fields belong to a single error. Recording a wrapper's +// DataCellPos/DataCell would point every sibling at a cell it never touched. +// +// This mirrors the real confgen chain of a horizontal map, where the wrapper +// around the nested join carries the *key* column's cell: +// +// parseMessage -> messageCollector.Join() == collected +// parseHorizontalMapField -> WrapKV(CellDebugKV of the key column) +// parseMessage (parent) -> messageCollector.Collect(...) <- same tree +func TestCollected_ReCollectDropsNonScopeFields(t *testing.T) { + sheet := NewCollector(10) + nested := sheet.NewChild(5) + // Sibling 1 owns its cell; sibling 2 has no cell of its own. + _ = nested.Collect(WrapKV(E2002("100033333", "ItemConf.ID"), + KeyDataCellPos, "B4", + KeyDataCell, "100033333", + )) + _ = nested.Collect(E2014("Item1Miss")) + + // The wrapper carries the key column's cell alongside the scope fields. + _ = sheet.Collect(WrapKV(nested.Join(), + KeyModule, ModuleConf, + KeyBookName, "Activity.xlsx", + KeySheetName, "SectionConf", + KeyDataCellPos, "A4", + KeyDataCell, "7", + KeyColumnName, "Item1ID", + )) + + got := sheet.Join().Error() + // Scope fields are broadcast to both children. + assert.Contains(t, got, "Workbook: Activity.xlsx") + assert.Contains(t, got, "Worksheet: SectionConf") + // Sibling 1 keeps its own cell; the wrapper's cell reaches neither. + assert.Contains(t, got, "DataCellPos: B4") + assert.NotContains(t, got, "DataCellPos: A4", + "wrapper cell position must not be broadcast to the joined children") + assert.NotContains(t, got, "DataCell: 7", + "wrapper cell data must not be broadcast to the joined children") +} + +// A wrapper carrying no scope fields must clear the previously recorded ones, +// so a stale book/sheet name is never re-applied to an unrelated join. +func TestCollected_ReCollectFieldlessWrapperClearsStaleFields(t *testing.T) { + book := NewCollector(10) + first := book.NewChild(5) + _ = first.Collect(E2002("v1", "ItemConf.ID")) + _ = book.Collect(WrapKV(book.Join(), + KeyModule, ModuleConf, + KeyBookName, "First.xlsx", + KeySheetName, "S1", + )) + + second := book.NewChild(5) + _ = second.Collect(E2002("v2", "ShopConf.ID")) + _ = book.Collect(Wrap(book.Join())) + + got := book.Join().Error() + assert.NotContains(t, got, "First.xlsx", "stale book name must not survive") + assert.NotContains(t, got, "Worksheet: S1", "stale sheet name must not survive") +} + // collected marker is transparent: errors.Is works through it. func TestCollected_ErrorsIsWorksThrough(t *testing.T) { target := fmt.Errorf("target") diff --git a/internal/x/xerrors/desc.go b/internal/x/xerrors/desc.go index 62e540ad..7035abef 100644 --- a/internal/x/xerrors/desc.go +++ b/internal/x/xerrors/desc.go @@ -80,6 +80,22 @@ var keys = []string{ keyHelp, } +// scopeKeys are the keys identifying the enclosing scope (which dir, book, +// sheet, message) rather than a location inside it. Only these may be shared +// across the errors of a joined tree: a cell position or field name describes +// one error alone, so broadcasting it would point the others at a wrong cell. +var scopeKeys = map[string]bool{ + KeyModule: true, + KeyIndir: true, + KeySubdir: true, + KeyOutdir: true, + KeyBookName: true, + KeyPrimaryBookName: true, + KeySheetName: true, + KeyPrimarySheetName: true, + KeyPBMessage: true, +} + // multiUnwrapper is implemented by joined errors (e.g. errors.Join). type multiUnwrapper interface { Unwrap() []error diff --git a/internal/x/xerrors/errors.go b/internal/x/xerrors/errors.go index 8f1da200..2a98160d 100644 --- a/internal/x/xerrors/errors.go +++ b/internal/x/xerrors/errors.go @@ -135,13 +135,7 @@ func (b *base) Format(s fmt.State, verb rune) { // renderWithFields delegates to the cause, passing outerFields through the stack wrapper. func (b *base) renderWithFields(outerFields map[string]any) string { - if b.cause != nil { - if r, ok := b.cause.(fieldsRenderer); ok { - return r.renderWithFields(outerFields) - } - return b.cause.Error() - } - return "" + return renderCause(b.cause, outerFields) } // withMessage wraps a cause with an optional message and structured fields. @@ -165,6 +159,18 @@ type fieldsRenderer interface { renderWithFields(outerFields map[string]any) string } +// renderCause renders cause with outerFields propagated into it, falling back +// to plain Error() for causes that do not carry fields. +func renderCause(cause error, outerFields map[string]any) string { + if cause == nil { + return "" + } + if r, ok := cause.(fieldsRenderer); ok { + return r.renderWithFields(outerFields) + } + return cause.Error() +} + func (w *withMessage) Error() string { if w.message != "" { // replacesCause: message is the complete text. @@ -205,13 +211,7 @@ func (w *withMessage) renderWithFields(outerFields map[string]any) string { merged := make(map[string]any, len(outerFields)+len(w.fields)) maps.Copy(merged, outerFields) maps.Copy(merged, w.fields) - if w.cause != nil { - if r, ok := w.cause.(fieldsRenderer); ok { - return r.renderWithFields(merged) - } - return w.cause.Error() - } - return "" + return renderCause(w.cause, merged) } func (w *withMessage) Format(s fmt.State, verb rune) {