Conversation
pv-go does allow for this eagerness by passing the descriptors (or concrete instances of the messages) when configuring the validator. I believe most of the other implementations follow the same pattern. The validators are built per-message type and fail open; compilation errors are only exposed once validate is called on that message type (or a dependent thereof). The main reason this is lazy by default is because pv implementations should support dynamic messages where the descriptor is unknown until the point of validation. pv-go's conformance runner indeed does not look at the descriptors until Validate is called. This applies to a future Rust implementation as well. All this to say, moving these to a separate suite should not gain anything but may hide bugs where a realistic descriptor set may contain a mix of valid and invalid rules. |
|
Thanks @rodaine - I didn't think of dynamic messages. I think this makes sense now. |
I noticed that currently compilation error test cases are in the same descriptor set as others. This effectively makes it impossible to write a protovalidate implementation that eagerly compiles descriptors i.e. at server startup to minimize first-request overhead. While different languages will have different expectations around this, it seems the conformance runner should not be the one making that decision. Note that protovalidate-cc requires adding descriptors before validating, and I only now after some digging realized it doesn't compile the rules until validation time breaking my expectation - I think this is generally surprising to users. And for a hypothetical rust implementation, it becomes even less idiomatic where exclusive borrows are supposed to allow avoiding locks entirely while preserving compile-time thread-safety, not possible if we can't compile rules when added and holding the exclusive borrow.
As the runner already runs suites independently, fixing this is just a matter of separating out a suite for compilation errors. However, this changes the test name for these cases, so failure lists etc do get impacted. I suspect this is acceptable but let me know any thoughts.