Skip to content

fix(xerrors): keep outer fields when rendering collected errors - #449

Open
Kybxd wants to merge 1 commit into
masterfrom
fix-collected-render-with-fields
Open

fix(xerrors): keep outer fields when rendering collected errors#449
Kybxd wants to merge 1 commit into
masterfrom
fix-collected-render-with-fields

Conversation

@Kybxd

@Kybxd Kybxd commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

Errors produced by a Collector lose the fields attached by enclosing WrapKV layers when rendered via Error(). Since Desc.Stringify picks the message template by the Module field, losing it makes the default template render instead of the module-specific one.

The practical impact: confgen load errors lose Workbook / Worksheet / DataCellPos / DataCell, so there is no hint about which cell to fix. A refer check failure looks like this:

error[E2002]: field value not in referred space
Reason: value "100033333" not in referred space "ItemConf.ID"
Help: guarantee value "100033333" was configured in referred space "ItemConf.ID" ahead

The fields are all correctly attached along the chain (tableParser.Parse adds CellDebugKV, parseMessageFromOneImporter adds Module/BookName/SheetName) — they are only dropped at render time.

This is easy to hit from a downstream checker built on load, which reports issues via err.Error().

Root causes

Two distinct bugs, each affecting a different sheet layout:

1. collected did not implement fieldsRenderer.

withMessage.Error() on a fields-only wrapper delegates through base.renderWithFields, which requires the cause to implement fieldsRenderer. collected only implemented Error/Unwrap/Format, so rendering fell back to c.error.Error() — i.e. joinError.Error() == renderWithFields(nil) — discarding every outer field. This affects single-importer sheets.

2. Collector.Collect only remembered the outermost withMessage layer.

Wrapper chains commonly spread fields across several layers: confgen adds Module in sheetParser.Parse's defer, then BookName/SheetName in parseMessageFromOneImporter, and for multiple importers ParseMessage's Group.Go adds yet another layer carrying only book/sheet names and no Module. Keeping just err.(*withMessage) therefore dropped Module, so multi-importer sheets (merger / scatter) still rendered the default template even after fix 1.

Fix

  • Implement fieldsRenderer on collected, forwarding outerFields to the inner error.
  • Add outerFieldsUntil, which walks every wrapper layer between the error and the collected marker and merges their fields (inner layers win), replacing the outermost-layer-only logic in Collect.

Result

Both single- and multi-importer sheets now render the module-specific template:

error[E2002]: field value not in referred space
Workbook: Activity_ActivityCenter.xlsx (Primary: Activity.xlsx)
Worksheet: SectionConf
DataCellPos: S218
DataCell: 100033333
Reason: value "100033333" not in referred space "ItemConf.ID"
Help: guarantee value "100033333" was configured in referred space "ItemConf.ID" ahead

For a merger sheet it additionally pinpoints which merged workbook the bad cell lives in, which the primary book name alone cannot express.

Tests

Two regression tests are added, mirroring the real confgen chains (single-importer and multi-importer). Each was verified to fail independently when only its corresponding fix is reverted:

  • revert fix 1 → only TestCollected_ErrorPropagatesOuterFields fails
  • revert fix 2 → only TestCollected_ReCollectPreservesAllWrapperFields fails

go test ./... (including test/functest), go vet and golangci-lint run all pass. Verified end to end against a real spreadsheet tree, where the reported cell SectionConf!S218 was confirmed to hold the offending value.

Errors produced by a Collector lost the fields attached by enclosing
WrapKV layers when rendered via Error(), so the default message template
was used instead of the module-specific one (e.g. confgen). The book,
sheet and cell position were therefore missing from confgen load errors,
leaving no hint about which cell to fix:

  error[E2002]: field value not in referred space
  Reason: value "100033333" not in referred space "ItemConf.ID"
  Help: guarantee value "100033333" was configured in referred space "ItemConf.ID" ahead

Two distinct causes are fixed:

1. collected did not implement fieldsRenderer, so base.renderWithFields
   fell back to the inner error's Error() and dropped all outer fields.

2. Collector.Collect only remembered the outermost withMessage layer of
   an already-collected error. Wrapper chains commonly spread fields
   across several layers (confgen adds Module in one layer and
   BookName/SheetName in another), so every layer down to the collected
   marker must be merged. This is what multi-importer sheets (merger or
   scatter) hit, since ParseMessage adds another wrapper layer that
   carries no Module.

Both paths now render the module-specific template:

  error[E2002]: field value not in referred space
  Workbook: Activity_ActivityCenter.xlsx (Primary: Activity.xlsx)
  Worksheet: SectionConf
  DataCellPos: S218
  DataCell: 100033333
  Reason: value "100033333" not in referred space "ItemConf.ID"
  Help: guarantee value "100033333" was configured in referred space "ItemConf.ID" ahead
@github-actions

Copy link
Copy Markdown

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 12, 2026, 9:36 AM

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.65%. Comparing base (7bc5b61) to head (5f434a3).

Files with missing lines Patch % Lines
internal/x/xerrors/collector.go 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
+ Coverage   75.63%   75.65%   +0.02%     
==========================================
  Files          88       88              
  Lines        9531     9543      +12     
==========================================
+ Hits         7209     7220      +11     
- Misses       1747     1748       +1     
  Partials      575      575              

☔ 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.

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.

1 participant