Jens-G opened a new pull request, #3877: URL: https://github.com/apache/thrift/pull/3877
JIRA: [THRIFT-6279](https://issues.apache.org/jira/browse/THRIFT-6279) Client: erl `thrift_json_protocol` had three problems: - it could not read a message; - it wrote a string given as a binary without quotes; - over a socket or framed transport, a read never ended. After this change it reads what it writes, and a read ends with its message. This also covers the second point of THRIFT-6269 (one transport call per byte), which was folded into this ticket. ### Change **Reading** - Since THRIFT-1357, the in-tree parser hands over numbers as numbers, and keys and strings as binaries. The read path still treated them as lists (`list_to_integer/1`, `list_to_float/1`, string patterns in `json_to_typeid/1`). This is fixed for the message header, field ids, field and container type names, integers and doubles. - Reading the start of a container now returns type ids, which `thrift_protocol` matches against the declared types. - The stop field no longer takes the struct's `end_object` with it; `struct_end` reads it. - The message name is returned as a list, as the binary protocol returns it. The processor splits it with `string:tokens/2`. - Map keys of type bool and double are read from their key text, since they are written as quoted object keys. **Writing** - `{string, Str}` is written quoted and escaped, for lists and binaries alike. Escaped are `"`, `\` and control characters, as `\b \f \n \r \t` or `\u00XX`. **Where a message ends** (design (a) from the ticket discussion: collect the message, then parse it) - `read_all/1` scans the bytes for brackets outside of strings, taking escapes into account, and stops when the outer array closes. - Each call asks the transport for the fewest bytes the message can still need: one per open bracket, plus the end of an open string. - `thrift_socket_transport:read/2`, and the buffered transport over it, block until they have all the bytes asked for, so a larger read could wait past the end of the message. - Measured: reading a 100 KB string argument takes 25,016 transport calls, down from about 100,000. - **Error handling:** - A transport error is returned as `{error, Reason}`; the processor and the client already handle that. - A transport that runs out in the middle of a message returns `{error, eof}`. - The special case for `{error, 'EOF'}` is gone. - A type spec that still described event data as strings was corrected. ### Not in this PR Interop with the other bindings' JSON protocols stays open: booleans as `1`/`0`, binary fields base64-encoded, integral and special doubles. So does adding JSON to the Erlang entry of `test/tests.json`. Binary fields need a type term of their own from the generator, so this goes to a follow-up ticket. The README names the difference. ### Tests New module `lib/erl/test/test_thrift_json_protocol.erl`: - **Round trip of every type,** including map keys of each scalar type and a list of nested structs. - **Strings** with every character that needs escaping, JSON punctuation and non-ASCII text, given as a list and as a binary: - no raw control byte appears in the output; - the escape sequences are exactly as expected; - the value reads back unchanged. - **Two messages back to back** in one buffer. This shows that a read does not take bytes of the next message. - **Error cases:** a truncated message gives an error, and a transport error is returned. - **Transport calls:** reading a 100 KB string takes fewer than size/3 calls. - **Over a real socket:** `thrift_socket_server` and `thrift_client_util` with `json`, buffered and framed, eight calls on one connection: - awkward strings and an empty string; - i32, double, bool and list; - a struct, twice. Results: - **Without this change:** all 10 tests fail on OTP 25 and 28, with badarg, missing escapes, or `case_clause` on `{error,closed}` over the socket. - **`rebar3 eunit`:** 370 tests, 0 failures on OTP 25.3.2.21, 25.3.2.9 (rebar3 3.18.0), 26.2.5.21, 27.3.4.17 and 28.5.0.6, each run two to three times. - **Mutations**, each caught: - Reading 4096 bytes at a time: the two-messages test reads into the second message, and the buffered socket waits. - Reading 1 byte at a time: caught by the call count. - No escaping. - A scan that ignores strings, or ignores escapes. - The stop field taking the struct's `end_object`. - The message name as a binary. - Treating a transport that has run out as the end of the message. - Asking for one byte more than the minimum inside a string: the buffered socket waits, on the empty-string call. - **Dialyzer:** no warnings. - **erlfmt:** `rebar3 fmt -c` reports nothing for the changed files. ### Threat model Checked against `doc/thrift-threat-model.md`. A read now ends at the end of the message instead of at the end of the stream. It never asks the transport for more than the message still needs. The document is not changed. The change was written with AI assistance (Claude Opus 5). 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
