Skip to content

feat(protogen): per-messager preserveFieldNumbers override - #441

Open
wenchy wants to merge 5 commits into
masterfrom
feat/preserve-field-numbers-binpb-gate
Open

feat(protogen): per-messager preserveFieldNumbers override#441
wenchy wants to merge 5 commits into
masterfrom
feat/preserve-field-numbers-binpb-gate

Conversation

@wenchy

@wenchy wenchy commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary

Field numbers only appear in the binpb wire format — JSON and txtpb encode field names — so preserveFieldNumbers is pure overhead for json/txtpb-only messagers (re-parsing all generated protos + per-field lookups + reshuffling field numbers in committed .proto artifacts).

This PR adds explicit, regex-based per-messager control over preservation, kept entirely inside proto.output — no coupling to conf.output, no silent veto of an explicit user choice.

Approach

PreserveFieldNumbersRules is an ordered list of {messager, preserve} regex rules. First match wins; a messager matching no rule falls back to the global PreserveFieldNumbers:

proto:
  output:
    preserveFieldNumbers: true                # global default
    preserveFieldNumbersRules:                # first matching rule wins
      - { messager: "Temp.*|Test.*", preserve: false }   # skip scratch messagers
      - { messager: "^Item", preserve: true }            # force Item* on

messager is a Go regexp matched unanchored against the message name. An ordered list (not a regex-keyed map) keeps overlapping patterns deterministic.

  • preserveFieldNumbers(name) — first matching rule wins, else global default.
  • anyPreserveFieldNumbers() — true if the global default is true or any rule preserves; gates whether the previously-generated protos are re-parsed at all.
  • Patterns are compiled lazily and cached; an invalid pattern panics eagerly from preprocess (fails fast before regenerating output).

Why this shape

The first iteration gated preservation on conf.output containing binpb. That silently vetoed an explicit preserveFieldNumbers: true (reshuffling committed .proto field numbers behind the user's back) and inverted the protogen→confgen layering (--mode proto depended on conf settings; split runs could diverge). Per-messager rules keep the decision in proto.output and only ever skip preservation for messagers the user explicitly matches.

Non-breaking and additive — the existing PreserveFieldNumbers bool and --preserve-field-numbers flag keep their semantics as the global default.

Changes

  • options/options.go: PreserveFieldNumbersRules []PreserveFieldNumbersRule ({Messager, Preserve}).
  • internal/protogen/protogen.go: preserveFieldNumbers(name), anyPreserveFieldNumbers(), compiledPreserveRules(); eager validation in preprocess.
  • internal/protogen/exporter.go: findMDFromGeneratedProtos gates on preserveFieldNumbers(name).
  • Tests: table-driven unit tests (regex, alternation, anchoring, first-match-wins, substring) + invalid-pattern panic test; exporter integration test proving an {messager: "Item", preserve: false} rule yields sequential field numbers (1,2,3) instead of preserved (1,3,2).

Test plan

  • go build ./...
  • go test ./... (incl. functest)
  • go vet ./options/ ./internal/protogen/ ./internal/confgen/ ./cmd/tableauc/

🤖 Generated with Claude Code

Field numbers only appear in the binary (binpb) wire format; JSON and
txtpb encode field names, so preserveFieldNumbers is pure overhead
(re-parsing all generated protos + per-field lookups) when binpb is not
among the conf output formats.

Gate preservation on binpb presence via a new
(*ConfOutputOption).NeedBinpb() helper, which mirrors confgen's
parseOutputFormats resolution (empty Formats => all formats incl.
binpb). A nil conf opt (programmatic use) preserves the previous
behavior. Users who reuse the generated protos for binpb outside
tableau's confgen can keep preservation active by adding binpb to
conf.output.formats.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 29, 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✅ passedJul 30, 2026, 1:22 PM

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.69%. Comparing base (7bc5b61) to head (ca8c3a5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   75.63%   75.69%   +0.05%     
==========================================
  Files          88       88              
  Lines        9531     9552      +21     
==========================================
+ Hits         7209     7230      +21     
  Misses       1747     1747              
  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.

@Kybxd

Kybxd commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR — the performance motivation is real, and the implementation itself is clean (nil-safety, conservative gating, unit tests for NeedBinpb). However, I have concerns about the approach and would lean against merging it in the current form.

1. An explicit option should not be silently vetoed by another config section

preserveFieldNumbers: true is an explicit, user-facing switch. This PR changes its effective semantics to preserveFieldNumbers AND conf-output-includes-binpb, where the second condition lives in a completely different config subtree (conf.output.formats / conf.output.messagerFormats). A user who explicitly enabled preservation will find it silently not taking effect — the only signal is an info-level log line at generation time.

Critically, this gate is not a transparent optimization: skipping preservation changes the generated .proto files themselves (field numbers get reshuffled), and those files are user-visible artifacts committed to VCS and potentially consumed outside tableau's confgen — the PR description itself acknowledges the "reuse generated protos for binpb data elsewhere" scenario. For those users, upgrading tableau silently reshuffles field numbers and breaks wire compatibility with previously emitted binpb data, and the breakage surfaces at data-load time, far from the cause.

The proposed escape hatch illustrates the design smell: to keep a proto-generation behavior active, users must add binpb to conf.output.formats, which makes confgen actually emit binpb files they don't want. "Whether to output binpb files" and "whether to keep proto field numbers stable" are two independent concerns; binding them to a single knob is exactly what will confuse users.

2. protogen should not depend on confgen's output config

protogen is the lower layer (schema generation); confgen consumes its output. Having the lower layer inspect the upper layer's output options to decide its own behavior is a layering inversion. Concretely: tableauc --mode proto now behaves differently depending on conf settings even though no conf generation happens, and in split workflows (proto and conf generated in separate runs with different --conf-output-formats overrides) the two sides can diverge.

Suggestions

  • Preferred: keep the current explicit semantics. Users who don't need preservation can simply set preserveFieldNumbers: false; the performance cost only affects projects that explicitly opted in.
  • If the optimization is still wanted: make it opt-in rather than a silent override — e.g., evolve the option into a tri-state (auto / true / false, yaml string) where auto enables the binpb-gated behavior, while an explicit true always means true. This keeps the decision inside proto.output and never vetoes an explicit user choice.
  • At minimum, if any gating ships: log at warn level (once, not per sheet — currently the info log fires via findMDFromGeneratedProtos up to 3 times per sheet), and call it out prominently in the release notes, since existing preserveFieldNumbers: true + json-only users will see a one-time field-number reshuffle in their generated protos.

Minor

  • Generator.preserveFieldNumbers() itself has no unit test (codecov shows the new uncovered lines); a small table-driven test over nil/json-only/binpb ConfOutputOpt would help.

…to.output

Replaces the conf.output-coupled binpb gate (which silently vetoed an
explicit preserveFieldNumbers: true and inverted the protogen->confgen
layering) with an explicit, per-messager override that lives entirely
in proto.output.

Add ProtoOutputOption.MessagerPreserveFieldNumbers (map[string]bool,
keyed by message name, same key as conf.output.messagerFormats). A
messager in the map uses its mapped value; otherwise it falls back to
the global PreserveFieldNumbers. This lets users skip preservation for
json/txtpb-only messagers without coupling to conf.output or silently
reshuffling field numbers of binpb-consuming messagers.

  preserveFieldNumbers: true
  messagerPreserveFieldNumbers: { ItemConf: false }

Generator.preserveFieldNumbers(name) resolves per messager (used at
findMDFromGeneratedProtos); Generator.anyPreserveFieldNumbers() decides
whether to parse the previously generated protos at all (used at
preprocess), conservatively true if the global default or any override
is true.

Drops the ConfOutputOpt field, the NeedBinpb helper, and the info log.
Adds table-driven unit tests for both methods plus an exporter test
proving an "Item: false" override yields sequential field numbers
instead of preserved ones.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@wenchy wenchy changed the title feat(protogen): skip preserveFieldNumbers when conf output has no binpb feat(protogen): per-messager preserveFieldNumbers override Jul 30, 2026
@wenchy

wenchy commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

@/dev/stdin

Evolve MessagerPreserveFieldNumbers from an exact-match map[string]bool
into an ordered list of {pattern, preserve} regex rules, so preservation
can be toggled for whole groups of messagers at once (e.g. "Temp.*|Test.*"
or "Conf$"). Rules are evaluated in order; first regex match wins, else
the global PreserveFieldNumbers default applies. Falling back to the
global default keeps the common case (no rules) unchanged.

  preserveFieldNumbers: true
  messagerPreserveFieldNumbers:
    - { pattern: "Temp.*|Test.*", preserve: false }

Patterns are Go regexps matched (unanchored) against the message name.
They are compiled lazily and cached; an invalid pattern panics with a
clear message (consistent with the generator's other config-parse
panics), eagerly triggered from preprocess so it fails fast before any
output is regenerated.

anyPreserveFieldNumbers (the preprocess gate for parsing the previously
generated protos) is conservative: true iff the global default is true
or any rule has preserve=true.

Update unit tests with regex/precedence cases and an invalid-pattern
panic test; update the exporter integration test to use a rule.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@wenchy

wenchy commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

@/dev/stdin

wenchy and others added 2 commits July 30, 2026 20:41
Rename the override field to PreserveFieldNumbersRules (yaml
preserveFieldNumbersRules) so its stem matches the global
PreserveFieldNumbers option, making the relationship obvious in config.
Rename the rule struct field Pattern -> Messager (yaml messager) so the
key self-documents that the regex is matched against a messager name
rather than a generic pattern:

  preserveFieldNumbers: true
  preserveFieldNumbersRules:
    - { messager: "Temp.*|Test.*", preserve: false }
    - { messager: "^Item", preserve: true }

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Trim the option/struct/method comments to the essentials: rule order,
first-match-wins, unanchored regex, and the binpb relevance note.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants