Copilot commented on code in PR #3635:
URL: https://github.com/apache/brpc/pull/3635#discussion_r4332912768
##########
src/butil/third_party/rapidjson/reader.h:
##########
@@ -1540,9 +1540,7 @@ class GenericReader {
// 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());
- }
+ SetParseError(kParseErrorNone, is.Tell());
Review Comment:
Since the behavior is being changed specifically to record offsets even at
EOF, it would help to update the comment to explicitly mention the EOF case
(previously guarded by `Peek() != '\0'`). This makes the intent clearer for
future maintainers, especially given this is a vendored RapidJSON file.
##########
test/brpc_protobuf_json_unittest.cpp:
##########
@@ -1689,6 +1689,90 @@ 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;
+ } cases[] = {
+ {"{}", 2},
+ {" \t{}", 4},
+ {R"({"person":[]})", 13},
+ {"[]", 2},
+ {"{} \n", 2},
+ {"{}{}", 2},
+ };
+ json2pb::Json2PbOptions options;
+ options.allow_remaining_bytes_after_parsing = true;
+ options.array_to_single_repeated = true;
+
+ for (const auto& test_case : cases) {
+ SCOPED_TRACE(test_case.json);
+ 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 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, &offset)) << error;
+ EXPECT_EQ(expected_messages[i], message.message());
+ EXPECT_EQ(jsons[i].size(), offset);
+ remaining = remaining.substr(offset);
+
+ message.Clear();
+ ASSERT_TRUE(json2pb::JsonToProtoMessage(
+ &reader, &message, options, &error, &offset)) << error;
+ EXPECT_EQ(expected_messages[i], message.message());
+ total_offset += jsons[i].size();
+ EXPECT_EQ(total_offset, offset);
+ EXPECT_EQ(total_offset, reader.Tell());
+ }
Review Comment:
The same `offset` variable is reused for two different semantics in the same
loop (string parse offset relative to `remaining`, then stream/reader offset
absolute from the start). This makes the test harder to read and easier to
accidentally break. Consider using two variables (e.g., `string_offset` and
`stream_offset`) or resetting/renaming to make the distinction explicit.
##########
test/brpc_protobuf_json_unittest.cpp:
##########
@@ -1689,6 +1689,90 @@ 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;
+ } cases[] = {
+ {"{}", 2},
+ {" \t{}", 4},
+ {R"({"person":[]})", 13},
+ {"[]", 2},
+ {"{} \n", 2},
+ {"{}{}", 2},
+ };
+ json2pb::Json2PbOptions options;
+ options.allow_remaining_bytes_after_parsing = true;
+ options.array_to_single_repeated = true;
Review Comment:
`array_to_single_repeated = true` is not required for most of the test cases
here and broadens the test’s scope beyond “offset at EOF”. To keep the
regression focused, consider enabling this option only for the specific case(s)
that need it (e.g., the `"[]"` input) or splitting the array case into a
separate test.
--
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]
