feat(load): protovalidate messages on the load path#438
Conversation
Export confgen.NewValidator/Validate and reuse them so the load (checker) path validates fully-assembled messages via protovalidate's global validator, matching confgen's storeMessage coverage.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #438 +/- ##
==========================================
+ Coverage 75.60% 75.61% +0.01%
==========================================
Files 88 88
Lines 9527 9531 +4
==========================================
+ Hits 7203 7207 +4
Misses 1749 1749
Partials 575 575 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
| // protovalidate the fully-assembled message, giving the load path the same | ||
| // coverage as confgen's storeMessage step. | ||
| return confgen.Validate(msg, protovalidate.GlobalValidator) |
There was a problem hiding this comment.
We'd better add test case to cover the error condition of confgen.Validate, expect protovalidate error. This accounts for future regression.
Regression testing is typically conducted after bug fixes are implemented, and it involves re-running tests to ensure features function as expected after new code is added.
There was a problem hiding this comment.
Done. Added a regression test in load/load_test.go (TestLoadOriginProtovalidate) plus a new testdata/unittest/Unittest#ValidateConf.csv fixture whose Name exceeds max_len:10. Loading it through the full load path asserts the violation surfaces as xerrors.ErrE2027, so the test will fail if the confgen.Validate call in loadOrigin is ever dropped.
Actual output:
error[E2027]: protovalidate violation
Reason: "this name is too long" violates rule: name: must be at most 10 characters
There was a problem hiding this comment.
protovalidate should work both on input (origin formats: xlsx/csv/yaml/xml) and output file formats(json/binpb/txtpb), as protovalide works on the final in memory protobuf message structure.
There was a problem hiding this comment.
Good point. Moved the confgen.Validate step out of loadOrigin and converged it at the single LoadMessagerInDir exit, right after the message is fully assembled (and after patch merging, to avoid false positives on intermediate state). Now both input (xlsx/csv/yaml/xml) and output (json/binpb/txtpb) formats validate the final in-memory protobuf message, matching confgen's storeMessage behavior.
Extended the regression test into TestLoadProtovalidate with two subcases and added a JSON fixture:
input-format-csv— loadsValidateConfvia CSVoutput-format-json— loadstestdata/unittest/conf/ValidateConf.json
Both assert the violation surfaces as xerrors.ErrE2027:
error[E2027]: protovalidate violation
Reason: "this name is too long" violates rule: name: must be at most 10 characters
Add a ValidateConf CSV whose Name exceeds max_len:10 and a test that loads it, asserting loadOrigin surfaces the violation as E2027. Guards against regressions where the confgen.Validate call could be dropped.
| } | ||
| // protovalidate the fully-assembled message, giving the load path the same | ||
| // coverage as confgen's storeMessage step. | ||
| return confgen.Validate(msg, protovalidate.GlobalValidator) |
There was a problem hiding this comment.
protovalidate should work both on input (origin formats: xlsx/csv/yaml/xml) and output file formats(json/binpb/txtpb), as protovalide works on the final in memory protobuf message structure.
Move the protovalidate step out of loadOrigin and converge it at the LoadMessagerInDir exit. protovalidate operates on the final in-memory protobuf message, so both input (excel/csv/xml/yaml) and output (json/binpb/txtpb) formats now get the same coverage as confgen's storeMessage step. The check runs after patch merging to avoid false positives on intermediate state. Extend TestLoadOriginProtovalidate into TestLoadProtovalidate with input-format-csv and output-format-json subcases, and add the testdata/unittest/conf/ValidateConf.json fixture.
2 participants
What
Make the
load(checker) path run protovalidate on fully-assembled messages, matching the coverage confgen already provides in itsstoreMessagestep.Why
The
loadpath is typically used by checkers and never reaches confgen'sstoreMessage, so protovalidate constraints (e.g. required fields, CEL rules) were previously not enforced when loading from input formats. This closes that gap.How
internal/confgen/util.go: exportNewValidatorandValidate(previously unexportedvalidate).NewValidatorbuilds a validator whose extension type resolver is derived fromprFiles, shared across the generate and load paths.internal/confgen/confgen.go:GenAll/GenWorkbooknow callNewValidator(prFiles)instead of inliningprotovalidate.New(...).load/load.go: at the end ofloadOrigin, validate the fully-assembled message viaconfgen.Validate(msg, protovalidate.GlobalValidator). The global validator resolves extensions viaprotoregistry.GlobalTypes, which already contains custom predefined rules compiled into the binary, so no dedicated validator instance is needed on this path.Validation is intentionally placed after the message is fully assembled (not inside
ParseMessage), to avoid false positives on intermediate patch fragments and duplicate validation.Test
go build ./...andgo test ./load/... ./internal/confgen/...pass.