Jens-G commented on PR #3689: URL: https://github.com/apache/thrift/pull/3689#issuecomment-5140688671
### Code review Found 3 issues: 1. The new "wire-compatible changes" framing holds for `--audit-allow-optional-field-removal`, but not for `--audit-allow-required-field-to-default`. Dropping `required` is only safe in one rollout direction: a writer built from the new schema may omit the field entirely, while a peer still built from the old schema rejects the message at deserialization. https://github.com/apache/thrift/blob/194dcab82a69101a42e4f91dc4e494728b92febb/test/audit/README.md#L8-L18 Reproduced with 0.25.0 on `struct Record { 1: required string value }` -> `struct Record { 1: string value }`: the new-schema Java writer skips the field when unset (`if (struct.value != null)`), while the old-schema reader ends `read()` with `struct.validate()`, which throws `Required field 'value' was not present!`. That check is emitted here, and the equivalent exists in every binding I looked at: - Java `validate()`: https://github.com/apache/thrift/blob/194dcab82a69101a42e4f91dc4e494728b92febb/compiler/cpp/src/thrift/generate/t_java_generator.cc#L2235-L2242 (called from `read()` at https://github.com/apache/thrift/blob/194dcab82a69101a42e4f91dc4e494728b92febb/compiler/cpp/src/thrift/generate/t_java_generator.cc#L5615-L5618) - C++: https://github.com/apache/thrift/blob/194dcab82a69101a42e4f91dc4e494728b92febb/compiler/cpp/src/thrift/generate/t_cpp_generator.cc#L1826-L1836 - Python: https://github.com/apache/thrift/blob/194dcab82a69101a42e4f91dc4e494728b92febb/compiler/cpp/src/thrift/generate/t_py_generator.cc#L1274-L1279 The compatibility validation in the PR description used C++ with an `i32` field, a primitive that is always written, so it exercises the one case that cannot break. Nullable-typed fields are about half of `test/ThriftTest.thrift` (36 of 77, counting only string/binary/containers). Suggest scoping "wire-compatible" to the first bullet and documenting the required rollout order for the second: every reader must be off `required` before any writer stops setting the field. 2. This needs a JIRA ticket. Two new public compiler flags, changed audit pass/fail semantics, and new build/CI wiring is a significant change, not a trivial one, so `CONTRIBUTING.md` applies: the PR title and commit subject both need the `THRIFT-NNNN:` prefix. The commit's `Client: cpp` and `Generated-by:` trailers are already correct, so amending the subject line is all that is needed. https://github.com/apache/thrift/blob/194dcab82a69101a42e4f91dc4e494728b92febb/CONTRIBUTING.md#L15-L26 3. `--audit-allow-required-field-to-default` also relaxes service method argument lists and `throws` clauses, which neither the README nor the `help()` text mentions. Both are compared through `compare_single_struct`, so the new `requiredToDefaultAllowed` gate in `compare_struct_field` applies to them as well. Unlike `optional` — which the parser downgrades to default requiredness inside a service body, warning `optional keyword is ignored in argument lists` — `required` is preserved there, so `void doIt(1: required string arg)` is in scope. The consequence is larger than for a plain struct field: the server throws while reading `doIt_args`, so the entire call fails before the handler runs. None of the new fixtures cover a `service`. https://github.com/apache/thrift/blob/194dcab82a69101a42e4f91dc4e494728b92febb/compiler/cpp/src/thrift/audit/t_audit.cpp#L360-L365 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
