Copilot commented on code in PR #3635:
URL: https://github.com/apache/brpc/pull/3635#discussion_r4333702728
##########
src/butil/third_party/rapidjson/reader.h:
##########
@@ -1539,10 +1539,9 @@ class GenericReader {
if ((parseFlags & kParseStopWhenDoneFlag) && state ==
IterativeParsingFinishState) {
// wwb: Update parseResult_.Offset() when
kParseStopWhenDoneFlag
// is set which means the user needs to know where to resume
- // parsing in next calls to JsonToProtoMessage()
- if (is.Peek() != '\0') {
- SetParseError(kParseErrorNone, is.Tell());
- }
+ // parsing in next calls to JsonToProtoMessage(), including
+ // when the root ends exactly at EOF.
+ SetParseError(kParseErrorNone, is.Tell());
Review Comment:
This comment is in a third-party RapidJSON header but references a
project-specific API (`JsonToProtoMessage()`) and includes an author tag
(`wwb:`). To keep the intent clear and reusable, reword it in library-agnostic
terms (e.g., 'resume parsing in subsequent calls') and drop the author prefix.
##########
test/brpc_protobuf_json_unittest.cpp:
##########
@@ -1689,6 +1689,92 @@ TEST_F(ProtobufJsonTest, string_to_int64) {
ASSERT_EQ(person.data(), 1234567);
}
+TEST_F(ProtobufJsonTest, parse_json_offset_at_eof) {
+ const struct {
+ const char* json;
+ size_t expected_offset;
+ bool array_to_single_repeated;
+ } cases[] = {
+ {"{}", 2, false},
+ {" \t{}", 4, false},
+ {R"({"person":[]})", 13, false},
+ {"[]", 2, true},
+ {"{} \n", 2, false},
+ {"{}{}", 2, false},
+ };
+
+ for (const auto& test_case : cases) {
+ SCOPED_TRACE(test_case.json);
+ json2pb::Json2PbOptions options;
+ options.allow_remaining_bytes_after_parsing = true;
+ options.array_to_single_repeated = test_case.array_to_single_repeated;
+ AddressBook message;
+ std::string error;
+ size_t offset = 0;
+ ASSERT_TRUE(json2pb::JsonToProtoMessage(
+ test_case.json, &message, options, &error, &offset)) << error;
+ EXPECT_EQ(test_case.expected_offset, offset);
+ EXPECT_TRUE(error.empty());
+
+ butil::IOBuf buf;
+ buf.append(test_case.json);
+ butil::IOBufAsZeroCopyInputStream stream(buf);
+ message.Clear();
+ offset = 0;
+ ASSERT_TRUE(json2pb::JsonToProtoMessage(
+ &stream, &message, options, &error, &offset)) << error;
+ EXPECT_EQ(test_case.expected_offset, offset);
+ EXPECT_TRUE(error.empty());
+ }
+}
+
+TEST_F(ProtobufJsonTest, parse_multiple_json_at_eof) {
+ const std::string jsons[] = {
+ R"({"message":"first"})",
+ R"({"message":"second"})",
+ };
+ const char* expected_messages[] = {"first", "second"};
+ std::string remaining = jsons[0] + jsons[1];
+ butil::IOBuf buf;
+ buf.append(remaining);
+ butil::IOBufAsZeroCopyInputStream stream(buf);
+ json2pb::ZeroCopyStreamReader reader(&stream);
+ json2pb::Json2PbOptions options;
+ options.allow_remaining_bytes_after_parsing = true;
+ test::EchoRequest message;
+ std::string error;
+ size_t string_offset = 0;
+ size_t stream_offset = 0;
+ size_t total_offset = 0;
+
+ for (size_t i = 0; i < 2; ++i) {
+ SCOPED_TRACE(i);
+ message.Clear();
+ ASSERT_TRUE(json2pb::JsonToProtoMessage(
+ remaining, &message, options, &error, &string_offset)) << error;
+ EXPECT_EQ(expected_messages[i], message.message());
+ EXPECT_EQ(jsons[i].size(), string_offset);
+ remaining = remaining.substr(string_offset);
+
+ message.Clear();
+ ASSERT_TRUE(json2pb::JsonToProtoMessage(
+ &reader, &message, options, &error, &stream_offset)) << error;
+ EXPECT_EQ(expected_messages[i], message.message());
+ total_offset += jsons[i].size();
+ EXPECT_EQ(total_offset, stream_offset);
+ EXPECT_EQ(total_offset, reader.Tell());
+ }
+ EXPECT_TRUE(remaining.empty());
+ EXPECT_FALSE(json2pb::JsonToProtoMessage(
+ remaining, &message, options, &error, &string_offset));
+ EXPECT_TRUE(error.empty());
+ EXPECT_EQ(0ul, string_offset);
+ EXPECT_FALSE(json2pb::JsonToProtoMessage(
+ &reader, &message, options, &error, &stream_offset));
+ EXPECT_TRUE(error.empty());
+ EXPECT_EQ(total_offset, stream_offset);
Review Comment:
These negative-test assertions depend on `string_offset`/`stream_offset`
values produced by prior successful parses. To make the test independent and
easier to reason about, explicitly reset the out-params (and clear `error` /
`message`) immediately before the failure checks. For the stream case, consider
initializing `stream_offset` to `reader.Tell()` right before the failing call
so the assertion verifies it remains unchanged on EOF.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
