slachiewicz opened a new pull request, #3827: URL: https://github.com/apache/thrift/pull/3827
JIRA: [THRIFT-6204](https://issues.apache.org/jira/browse/THRIFT-6204) Client: go Stacked on #3822; the first commit is that PR. Review the second one. Default requiredness means "write if set" ([`doc/specs/idl.md`](https://github.com/apache/thrift/blob/master/doc/specs/idl.md)), and Python, Java, Node.js and C# all omit such a field when it holds no value. The Go writer wrote it unconditionally, so a nil pointer field either panicked on the way out — that half is #3822 — or, when its struct had no members, went onto the wire as an empty struct. Guarding the write with the generated `IsSet` helper, as optional fields already are, is a two-line change. **This is a wire change, and that is the whole question.** Opening it as a draft because that is a maintainer's call, not mine. ## What moves A nil field whose struct has no members is written today and omitted after: ``` before: 0c 00 01 00 08 00 02 00 00 00 07 00 field 1 present, empty struct after: 08 00 02 00 00 00 07 00 field 1 absent ``` A peer that declares that field `required` accepts the first and rejects the second: ``` before bytes -> err=<nil> after bytes -> err=Required field E is not set ``` Nothing panicked in that case, so this is working code changing behaviour, not a crash being fixed. ## Blast radius `is_pointer_field()` is true for every struct, union and exception field whatever its requiredness, and for every `cpp.ref` field, so the guard reaches wider than unions. Regenerating every IDL under `test/`, `lib/go/test/` and `tutorial/` with and without it: | Measure | Count | |---|---| | generated files changed | 75 of 650 | | write sites newly guarded | 227 | | changed files holding service `Args`/`Result` structs | 59 | The last row is the one to weigh: this reaches RPC argument and result encoding, not only user-declared structs. ## How I would land it Three options, and I have no stake in which: 1. As is, in a major, with a release note. 2. Behind a `go:` generator option defaulting off, so nobody's wire changes without asking. 3. Not at all, with the panic fixed in #3822 and the spec deviation documented. Say which and I will rework it. Verified: `go test ./tests/ -count=1` in `lib/go/test` passes after regenerating `gopath` through the `Makefile.am` recipe. `go build ./gopath/src/...` reports only the two pre-existing `includestest.Numberz` errors that #3817 fixes. ## CI The two red cross-test jobs are inherited from master — `d6782a87` turned the Python peer-certificate check on and 178 `ip-ssl` cases fail identically on every open PR, including a two-line `.asf.yaml` change. See #3818. *This change was created with AI assistance.* -- 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]
