Skip to content

fix(confgen): dedup refer load errors and locate them to referred target - #446

Open
Kybxd wants to merge 3 commits into
masterfrom
fix/refer-load-failure-dedup
Open

fix(confgen): dedup refer load errors and locate them to referred target#446
Kybxd wants to merge 3 commits into
masterfrom
fix/refer-load-failure-dedup

Conversation

@Kybxd

@Kybxd Kybxd commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Problem

When a refer target (e.g. AssistSkillConf.ID) fails to load its value space -- for instance because the referred sheet has a broken row that makes RangeDataRows return an error via Ignored() / parseColumns / etc. -- every row on the referring side would re-invoke loadFunc and re-emit the same error, producing N duplicate errors for one root cause.

Additionally, those N errors were wrapped with the source sheet's BookName / SheetName (the sheet under generation), which pointed users away from the sheet actually containing the offending row.

Changes

ReferredCache: negative caching

  • New failed map[string]struct{} tracks refer expressions whose load has already failed.
  • On loadFunc(refer) error, the failure is still surfaced to the caller (the collector receives the real error once), and the refer is marked as failed.
  • Subsequent ExistsValue calls for the same refer short-circuit to (true, nil) instead of re-triggering the load. This preserves error visibility while eliminating the N-way duplication.
  • Double-checked locking treats failed the same way as references, so concurrent callers don't race into duplicate loads.

AddFromTable: locate errors on the referred target

  • Wraps RangeDataRows errors with KeyReferBookName / KeyReferSheetName.
  • Outer confgen wrappers still set BookName / SheetName to the source sheet under generation (which is what users need to find the refer expression in the schema), while the new keys pinpoint the referred workbook / worksheet that actually contains the offending row.

Supporting bits

  • xerrors/desc.go: add KeyReferBookName / KeyReferSheetName constants and register them in keys.
  • i18n templates (zh.yaml / en.yaml): render ReferWorkbook / ReferWorksheet (引用工作簿 / 引用工作表) when present.

Behavior after this PR

Given AffixConf referring AssistSkillConf.ID and AssistSkill.xlsx having a broken row (e.g. an #IGNORE cell that fails to parse as bool):

Error source Before After
AssistSkill.xlsx self-generation error 1 1
AffixConf first refer check that triggers the load 1 1
Every subsequent row referring the same target N 0
Wrapped BookName / SheetName in the refer-side error source sheet only source sheet + ReferBookName / ReferSheetName pointing at AssistSkill.xlsx

Verification

  • go build ./...
  • go test ./internal/confgen/... ./internal/confgen/fieldprop/...

@github-actions

github-actions Bot commented Aug 5, 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✅ passedAug 17, 2026, 7:12 AM

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.31250% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.79%. Comparing base (108a85f) to head (63545b0).

Files with missing lines Patch % Lines
internal/confgen/fieldprop/refer.go 95.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   75.58%   75.79%   +0.21%     
==========================================
  Files          88       88              
  Lines        9593     9615      +22     
==========================================
+ Hits         7251     7288      +37     
+ Misses       1760     1748      -12     
+ Partials      582      579       -3     

☔ 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
Kybxd force-pushed the fix/refer-load-failure-dedup branch from fe1aee8 to ce9ae11 Compare August 17, 2026 06:54
Kybxd and others added 2 commits August 17, 2026 14:55
When a refer target (e.g. AssistSkillConf.ID) fails to load its value
space -- for instance because the referred sheet has a broken row that
makes RangeDataRows return an error via Ignored()/parseColumns -- every
row on the referring side would re-invoke loadFunc and re-emit the same
error, producing N duplicate errors for one root cause.

- ReferredCache now tracks a 'failed' set alongside 'references'. The
  first loadFunc failure is still surfaced to the caller (so the error
  is not swallowed), but subsequent ExistsValue calls on the same refer
  short-circuit to (true, nil) instead of re-triggering the load.
- AddFromTable wraps RangeDataRows errors with KeyReferBookName /
  KeyReferSheetName, so outer confgen wrappers (which set BookName /
  SheetName to the source sheet under generation) no longer obscure
  where the offending row actually lives.
- xerrors adds KeyReferBookName / KeyReferSheetName; zh/en templates
  render the referred workbook/worksheet when present.
… runs

Patch coverage was failing because the failed-refer short-circuit and
ReferBookName wrapping had no tests. Reset the process-level cache at
each GenAll/GenWorkbook so a prior failure cannot leak into the next run.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Kybxd
Kybxd force-pushed the fix/refer-load-failure-dedup branch from ce9ae11 to 4abd45d Compare August 17, 2026 06:55
…ess global

A package-level cache leaked failed loads and stale value spaces across
Generate/Load calls. Keep one cache per Generator (and per loadOrigin)
and pass it through SheetParserExtInfo into InReferredSpace.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Kybxd
Kybxd force-pushed the fix/refer-load-failure-dedup branch from 4abd45d to 63545b0 Compare August 17, 2026 07:11
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