cleanup(generator): check extension existence in IsGRPCLongrunningOperation (#14675)

Author: cuiy0006

generator/internal/longrunning.cc

@@ -75,12 +75,8 @@ DeduceLongrunningOperationResponseType(
 }  // namespace
 
 bool IsLongrunningOperation(MethodDescriptor const& method) {
-  bool grpc_lro =
-      method.output_type()->full_name() == "google.longrunning.Operation";
-  auto operation_service_extension =
-      method.options().GetExtension(google::cloud::operation_service);
-  bool http_lro = !operation_service_extension.empty();
-  return grpc_lro || http_lro;
+  return IsGRPCLongrunningOperation(method) ||
+         IsHttpLongrunningOperation(method);
 }
 
 bool IsLongrunningMetadataTypeUsedAsResponse(MethodDescriptor const& method) {
@@ -95,10 +91,11 @@ bool IsLongrunningMetadataTypeUsedAsResponse(MethodDescriptor const& method) {
 void SetLongrunningOperationMethodVars(
     google::protobuf::MethodDescriptor const& method,
     VarsDictionary& method_vars) {
+  if (!IsLongrunningOperation(method)) return;
   method_vars["longrunning_operation_type"] =
       ProtoNameToCppName(method.output_type()->full_name());
 
-  if (method.output_type()->full_name() == "google.longrunning.Operation") {
+  if (IsGRPCLongrunningOperation(method)) {
     auto operation_info =
         method.options().GetExtension(google::longrunning::operation_info);
     method_vars["longrunning_metadata_type"] = ProtoNameToCppName(absl::visit(
@@ -118,9 +115,7 @@ void SetLongrunningOperationMethodVars(
     return;
   }
 
-  auto operation_service_extension =
-      method.options().GetExtension(google::cloud::operation_service);
-  if (!operation_service_extension.empty()) {
+  if (IsHttpLongrunningOperation(method)) {
     method_vars["longrunning_response_type"] = ProtoNameToCppName(absl::visit(
         FullyQualifiedMessageTypeVisitor(),
         FullyQualifyMessageType(method, method.output_type()->full_name())));
@@ -136,7 +131,8 @@ void SetLongrunningOperationMethodVars(
 }
 
 bool IsGRPCLongrunningOperation(MethodDescriptor const& method) {
-  return method.output_type()->full_name() == "google.longrunning.Operation";
+  return method.output_type()->full_name() == "google.longrunning.Operation" &&
+         method.options().HasExtension(google::longrunning::operation_info);
 }
 
 bool IsHttpLongrunningOperation(MethodDescriptor const& method) {
@@ -150,7 +146,7 @@ void SetLongrunningOperationServiceVars(
     VarsDictionary& service_vars) {
   for (int i = 0; i != service.method_count(); ++i) {
     auto const* method = service.method(i);
-    if (method->output_type()->full_name() == "google.longrunning.Operation") {
+    if (IsGRPCLongrunningOperation(*method)) {
       service_vars["longrunning_operation_include_header"] =
           "google/longrunning/operations.pb.h";
       service_vars["longrunning_response_type"] =
@@ -169,9 +165,7 @@ void SetLongrunningOperationServiceVars(
           R"""(absl::StrCat("/", rest_internal::DetermineApiVersion("v1", *options) ,"/", request.name(), ":cancel"))""";
       return;
     }
-    if (!method->options()
-             .GetExtension(google::cloud::operation_service)
-             .empty()) {
+    if (IsHttpLongrunningOperation(*method)) {
       service_vars["longrunning_response_type"] = ProtoNameToCppName(
           absl::visit(FullyQualifiedMessageTypeVisitor(),
                       FullyQualifyMessageType(

generator/internal/longrunning_test.cc

@@ -46,12 +46,20 @@ TEST(LongrunningTest, IsGRPCLongrunningOperation) {
     service {
       name: "Service"
       method {
-        name: "Lro"
+        name: "GrpcLro"
+        input_type: "google.longrunning.Bar"
+        output_type: "google.longrunning.Operation"
+        options {
+          [google.longrunning.operation_info] {}
+        }
+      }
+      method {
+        name: "NonLro1"
         input_type: "google.longrunning.Bar"
         output_type: "google.longrunning.Operation"
       }
       method {
-        name: "NonLro"
+        name: "NonLro2"
         input_type: "google.longrunning.Bar"
         output_type: "google.longrunning.Bar"
       }
@@ -61,14 +69,20 @@ TEST(LongrunningTest, IsGRPCLongrunningOperation) {
   ASSERT_TRUE(TextFormat::ParseFromString(kServiceText, &service_file));
   DescriptorPool pool;
   FileDescriptor const* service_file_descriptor = pool.BuildFile(service_file);
-  EXPECT_TRUE(
-      IsLongrunningOperation(*service_file_descriptor->service(0)->method(0)));
   EXPECT_TRUE(IsGRPCLongrunningOperation(
       *service_file_descriptor->service(0)->method(0)));
   EXPECT_FALSE(IsHttpLongrunningOperation(
       *service_file_descriptor->service(0)->method(0)));
+  EXPECT_FALSE(IsGRPCLongrunningOperation(
+      *service_file_descriptor->service(0)->method(1)));
+  EXPECT_FALSE(IsGRPCLongrunningOperation(
+      *service_file_descriptor->service(0)->method(2)));
+  EXPECT_TRUE(
+      IsLongrunningOperation(*service_file_descriptor->service(0)->method(0)));
   EXPECT_FALSE(
       IsLongrunningOperation(*service_file_descriptor->service(0)->method(1)));
+  EXPECT_FALSE(
+      IsLongrunningOperation(*service_file_descriptor->service(0)->method(2)));
 }
 
 TEST(LongrunningTest, IsLongrunningMetadataTypeUsedAsResponseEmptyResponse) {

generator/internal/service_code_generator_test.cc

@@ -171,6 +171,9 @@ TEST(PredicateUtilsTest, HasLongRunningMethodOne) {
         name: "One"
         input_type: "google.protobuf.Bar"
         output_type: "google.longrunning.Operation"
+        options {
+          [google.longrunning.operation_info] {}
+        }
       }
       method {
         name: "Two"
@@ -224,11 +227,17 @@ TEST(PredicateUtilsTest, HasLongRunningMethodMoreThanOne) {
         name: "Two"
         input_type: "google.protobuf.Bar"
         output_type: "google.longrunning.Operation"
+        options {
+          [google.longrunning.operation_info] {}
+        }
       }
       method {
         name: "Three"
         input_type: "google.protobuf.Bar"
         output_type: "google.longrunning.Operation"
+        options {
+          [google.longrunning.operation_info] {}
+        }
       }
     }
   )pb";