Copilot commented on code in PR #3533:
URL: https://github.com/apache/brpc/pull/3533#discussion_r3959692080
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -385,12 +388,20 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
return -1;
}
const BUTIL_RAPIDJSON_NAMESPACE::Value& data = itr_data->value;
+ if (!data.IsObject()) {
+ LOG(ERROR) << "data field is not a json object";
+ return -1;
+ }
Review Comment:
The new error logs are missing key context needed for debugging in
production (e.g., which request/endpoint failed and, for instances, which index
failed). Consider including `service_name` consistently and the failing
instance index (`i`), and (if available in this function) the Discovery API
address/URL to disambiguate multi-cluster/multi-service failures.
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -385,12 +388,20 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
return -1;
}
const BUTIL_RAPIDJSON_NAMESPACE::Value& data = itr_data->value;
+ if (!data.IsObject()) {
+ LOG(ERROR) << "data field is not a json object";
+ return -1;
+ }
auto itr_service = data.FindMember(service_name);
if (itr_service == data.MemberEnd()) {
LOG(ERROR) << "No " << service_name << " field in discovery response";
return -1;
}
const BUTIL_RAPIDJSON_NAMESPACE::Value& services = itr_service->value;
+ if (!services.IsObject()) {
+ LOG(ERROR) << "Service " << service_name << " is not a json object";
+ return -1;
+ }
Review Comment:
The new error logs are missing key context needed for debugging in
production (e.g., which request/endpoint failed and, for instances, which index
failed). Consider including `service_name` consistently and the failing
instance index (`i`), and (if available in this function) the Discovery API
address/URL to disambiguate multi-cluster/multi-service failures.
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -403,6 +414,10 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
}
for (BUTIL_RAPIDJSON_NAMESPACE::SizeType i = 0; i < instances.Size(); ++i)
{
+ if (!instances[i].IsObject()) {
+ LOG(ERROR) << "instance is not a json object";
+ return -1;
Review Comment:
This change makes a single malformed element in `instances[]` cause the
entire `GetServers()` call to fail (return -1), potentially dropping otherwise
valid instances in mixed-quality responses. If the intent is robustness similar
to `ListDiscoveryNodes()` (which skips invalid entries), consider skipping
non-object instances (and continuing) instead of hard-failing—or
document/justify the stricter behavior here so operators understand why partial
results are not returned.
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -403,6 +414,10 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
}
for (BUTIL_RAPIDJSON_NAMESPACE::SizeType i = 0; i < instances.Size(); ++i)
{
+ if (!instances[i].IsObject()) {
+ LOG(ERROR) << "instance is not a json object";
+ return -1;
+ }
Review Comment:
The new error logs are missing key context needed for debugging in
production (e.g., which request/endpoint failed and, for instances, which index
failed). Consider including `service_name` consistently and the failing
instance index (`i`), and (if available in this function) the Discovery API
address/URL to disambiguate multi-cluster/multi-service failures.
##########
test/brpc_naming_service_unittest.cpp:
##########
@@ -710,6 +728,17 @@ TEST(NamingServiceTest, discovery_sanity) {
ASSERT_FALSE(svc.HasAddr(std::string()));
ASSERT_EQ(2, svc.AddrCount());
}
+
+ const char* invalid_services[] = {
+ "invalid-data.test",
+ "invalid-service.test",
+ "invalid-instance.test",
+ };
+ for (const char* service_name : invalid_services) {
+ servers.clear();
+ ASSERT_EQ(-1, dcns.GetServers(service_name, &servers));
+ ASSERT_TRUE(servers.empty());
+ }
Review Comment:
When this loop fails, it can be hard to see which specific `service_name`
caused the assertion. Add a `SCOPED_TRACE(service_name)` (or equivalent) inside
the loop so test output clearly identifies the failing case.
--
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]
