impl(rest): leverage EmptyResponseType to remove ambiguities in Post and Delete template functions (#14390)

scotthart · web-flow · commit 6505814e75b5 · 2024-06-28T14:02:53.000Z
diff --git a/generator/integration_tests/golden/v1/internal/golden_kitchen_sink_rest_stub.cc b/generator/integration_tests/golden/v1/internal/golden_kitchen_sink_rest_stub.cc
@@ -101,7 +101,7 @@ Status DefaultGoldenKitchenSinkRestStub::DoNothing(
       google::cloud::rest_internal::RestContext& rest_context,
       Options const& options,
       google::protobuf::Empty const& request) {
-  return rest_internal::Post(
+  return rest_internal::Post<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/doNothing"));
 }
@@ -110,7 +110,7 @@ Status DefaultGoldenKitchenSinkRestStub::ExplicitRouting1(
       google::cloud::rest_internal::RestContext& rest_context,
       Options const& options,
       google::test::admin::database::v1::ExplicitRoutingRequest const& request) {
-  return rest_internal::Post(
+  return rest_internal::Post<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/", request.table_name(), ":explicitRouting1"),
       rest_internal::TrimEmptyQueryParameters({std::make_pair("app_profile_id", request.app_profile_id()),
@@ -121,7 +121,7 @@ Status DefaultGoldenKitchenSinkRestStub::ExplicitRouting2(
       google::cloud::rest_internal::RestContext& rest_context,
       Options const& options,
       google::test::admin::database::v1::ExplicitRoutingRequest const& request) {
-  return rest_internal::Post(
+  return rest_internal::Post<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/", request.table_name(), ":explicitRouting2"),
       rest_internal::TrimEmptyQueryParameters({std::make_pair("app_profile_id", request.app_profile_id()),
diff --git a/generator/integration_tests/golden/v1/internal/golden_rest_only_rest_stub.cc b/generator/integration_tests/golden/v1/internal/golden_rest_only_rest_stub.cc
@@ -45,7 +45,7 @@ Status DefaultGoldenRestOnlyRestStub::Noop(
       google::cloud::rest_internal::RestContext& rest_context,
       Options const& options,
       google::protobuf::Empty const& request) {
-  return rest_internal::Post(
+  return rest_internal::Post<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/noop"));
 }
diff --git a/generator/integration_tests/golden/v1/internal/golden_thing_admin_rest_stub.cc b/generator/integration_tests/golden/v1/internal/golden_thing_admin_rest_stub.cc
@@ -146,7 +146,7 @@ Status DefaultGoldenThingAdminRestStub::DropDatabase(
       google::cloud::rest_internal::RestContext& rest_context,
       Options const& options,
       google::test::admin::database::v1::DropDatabaseRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/", request.database()));
 }
@@ -252,7 +252,7 @@ Status DefaultGoldenThingAdminRestStub::DeleteBackup(
       google::cloud::rest_internal::RestContext& rest_context,
       Options const& options,
       google::test::admin::database::v1::DeleteBackupRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/", request.name()));
 }
diff --git a/generator/internal/stub_rest_generator.cc b/generator/internal/stub_rest_generator.cc
@@ -389,7 +389,7 @@ Status Default$stub_rest_class_name$::$method_name$(
       google::cloud::rest_internal::RestContext& rest_context,
       Options const& options,
       $request_type$ const& request) {
-  return rest_internal::$method_http_verb$(
+  return rest_internal::$method_http_verb$<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, $request_resource$, $preserve_proto_field_names_in_json$,
       $method_rest_path$$method_http_query_parameters$);
 }
diff --git a/google/cloud/compute/global_operations/v1/internal/global_operations_rest_stub.cc b/google/cloud/compute/global_operations/v1/internal/global_operations_rest_stub.cc
@@ -71,7 +71,7 @@ Status DefaultGlobalOperationsRestStub::DeleteOperation(
     Options const& options,
     google::cloud::cpp::compute::global_operations::v1::
         DeleteOperationRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", "compute", "/",
                    rest_internal::DetermineApiVersion("v1", options), "/",
diff --git a/google/cloud/compute/global_organization_operations/v1/internal/global_organization_operations_rest_stub.cc b/google/cloud/compute/global_organization_operations/v1/internal/global_organization_operations_rest_stub.cc
@@ -47,7 +47,7 @@ Status DefaultGlobalOrganizationOperationsRestStub::DeleteOperation(
     Options const& options,
     google::cloud::cpp::compute::global_organization_operations::v1::
         DeleteOperationRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", "compute", "/",
                    rest_internal::DetermineApiVersion("v1", options), "/",
diff --git a/google/cloud/compute/instances/v1/internal/instances_rest_stub.cc b/google/cloud/compute/instances/v1/internal/instances_rest_stub.cc
@@ -876,7 +876,7 @@ Status DefaultInstancesRestStub::SendDiagnosticInterrupt(
     Options const& options,
     google::cloud::cpp::compute::instances::v1::
         SendDiagnosticInterruptRequest const& request) {
-  return rest_internal::Post(
+  return rest_internal::Post<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", "compute", "/",
                    rest_internal::DetermineApiVersion("v1", options), "/",
diff --git a/google/cloud/compute/region_operations/v1/internal/region_operations_rest_stub.cc b/google/cloud/compute/region_operations/v1/internal/region_operations_rest_stub.cc
@@ -45,7 +45,7 @@ Status DefaultRegionOperationsRestStub::DeleteOperation(
     Options const& options,
     google::cloud::cpp::compute::region_operations::v1::
         DeleteOperationRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", "compute", "/",
                    rest_internal::DetermineApiVersion("v1", options), "/",
diff --git a/google/cloud/compute/zone_operations/v1/internal/zone_operations_rest_stub.cc b/google/cloud/compute/zone_operations/v1/internal/zone_operations_rest_stub.cc
@@ -44,7 +44,7 @@ Status DefaultZoneOperationsRestStub::DeleteOperation(
     Options const& options,
     google::cloud::cpp::compute::zone_operations::v1::
         DeleteOperationRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", "compute", "/",
                    rest_internal::DetermineApiVersion("v1", options), "/",
diff --git a/google/cloud/internal/rest_stub_helpers.h b/google/cloud/internal/rest_stub_helpers.h
@@ -30,6 +30,8 @@ namespace cloud {
 namespace rest_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
+struct EmptyResponseType {};
+
 std::vector<std::pair<std::string, std::string>> TrimEmptyQueryParameters(
     std::vector<std::pair<std::string, std::string>> query_params);
 
@@ -58,7 +60,9 @@ StatusOr<Response> RestResponseToProto(RestResponse&& rest_response) {
   return destination;
 }
 
-template <typename Request>
+template <
+    typename Response, typename Request,
+    std::enable_if_t<std::is_same<Response, EmptyResponseType>::value, int> = 0>
 Status Delete(
     rest_internal::RestClient& client, rest_internal::RestContext& rest_context,
     Request const&, bool, std::string path,
@@ -70,7 +74,9 @@ Status Delete(
   return AsStatus(std::move(**response));
 }
 
-template <typename Response, typename Request>
+template <typename Response, typename Request,
+          std::enable_if_t<!std::is_same<Response, EmptyResponseType>::value,
+                           int> = 0>
 StatusOr<Response> Delete(
     rest_internal::RestClient& client, rest_internal::RestContext& rest_context,
     Request const&, bool, std::string path,
@@ -111,7 +117,9 @@ StatusOr<Response> Patch(
   return RestResponseToProto<Response>(std::move(**response));
 }
 
-template <typename Response, typename Request>
+template <typename Response, typename Request,
+          std::enable_if_t<!std::is_same<Response, EmptyResponseType>::value,
+                           int> = 0>
 StatusOr<Response> Post(
     rest_internal::RestClient& client, rest_internal::RestContext& rest_context,
     Request const& request, bool preserve_proto_field_names, std::string path,
@@ -128,7 +136,9 @@ StatusOr<Response> Post(
   return RestResponseToProto<Response>(std::move(**response));
 }
 
-template <typename Request>
+template <
+    typename Response, typename Request,
+    std::enable_if_t<std::is_same<Response, EmptyResponseType>::value, int> = 0>
 Status Post(
     rest_internal::RestClient& client, rest_internal::RestContext& rest_context,
     Request const& request, bool preserve_proto_field_names, std::string path,
diff --git a/google/cloud/internal/rest_stub_helpers_test.cc b/google/cloud/internal/rest_stub_helpers_test.cc
@@ -156,13 +156,16 @@ TEST(RestStubHelpers, DeleteWithEmptyResponse) {
 
   RestContext context;
   Request request;
-  auto result = Delete(*mock_client, context, request, false, "/v1/delete/");
+  auto result = Delete<EmptyResponseType>(*mock_client, context, request, false,
+                                          "/v1/delete/");
   EXPECT_THAT(result, IsOk());
 
-  result = Delete(*mock_client, context, request, false, "/v1/delete/");
+  result = Delete<EmptyResponseType>(*mock_client, context, request, false,
+                                     "/v1/delete/");
   EXPECT_THAT(result, StatusIs(StatusCode::kInternal));
 
-  result = Delete(*mock_client, context, request, false, "/v1/delete/");
+  result = Delete<EmptyResponseType>(*mock_client, context, request, false,
+                                     "/v1/delete/");
   EXPECT_THAT(result, StatusIs(StatusCode::kPermissionDenied));
   EXPECT_THAT(result.message(), Eq("Permission foo denied on resource bar."));
   EXPECT_THAT(result.error_info().domain(), Eq("googleapis.com"));
@@ -212,6 +215,38 @@ TEST(RestStubHelpers, DeleteWithNonEmptyResponse) {
   EXPECT_THAT(result->nanos(), Eq(456));
 }
 
+TEST(RestStubHelpers, DeleteWithNonEmptySameResponseRequestTypes) {
+  std::string json_response(kJsonResponsePayload);
+  auto* mock_200_response = new MockRestResponse();
+  EXPECT_CALL(*mock_200_response, StatusCode()).WillOnce([]() {
+    return HttpStatusCode::kOk;
+  });
+  EXPECT_CALL(std::move(*mock_200_response), ExtractPayload).WillOnce([&] {
+    return MakeMockHttpPayloadSuccess(json_response);
+  });
+
+  auto mock_client = std::make_unique<MockRestClient>();
+  EXPECT_CALL(*mock_client, Delete)
+      .WillOnce([&](RestContext&, RestRequest const& request) {
+        EXPECT_THAT(request.path(), Eq("/v1/delete/"));
+        return Status(StatusCode::kInternal, "Internal Error");
+      })
+      .WillOnce([&](RestContext&, RestRequest const& request) {
+        EXPECT_THAT(request.path(), Eq("/v1/delete/"));
+        return std::unique_ptr<rest_internal::RestResponse>(mock_200_response);
+      });
+
+  RestContext context;
+  Request request;
+  auto result =
+      Delete<Request>(*mock_client, context, request, false, "/v1/delete/");
+  EXPECT_THAT(result, StatusIs(StatusCode::kInternal));
+
+  result =
+      Delete<Request>(*mock_client, context, request, false, "/v1/delete/");
+  EXPECT_STATUS_OK(result);
+}
+
 TEST(RestStubHelpers, Get) {
   std::string json_response(kJsonResponsePayload);
   auto* mock_200_response = new MockRestResponse();
@@ -374,6 +409,40 @@ TEST(RestStubHelpers, PostWithNonEmptyResponse) {
   EXPECT_THAT(result->nanos(), Eq(456));
 }
 
+TEST(RestStubHelpers, PostWithNonEmptySameResponseRequestTypes) {
+  std::string json_response(kJsonUpdateResponse);
+  std::string json_request(kJsonUpdatePayload);
+  auto* mock_200_response = new MockRestResponse();
+  EXPECT_CALL(*mock_200_response, StatusCode()).WillOnce([]() {
+    return HttpStatusCode::kOk;
+  });
+  EXPECT_CALL(std::move(*mock_200_response), ExtractPayload).WillOnce([&] {
+    return MakeMockHttpPayloadSuccess(json_response);
+  });
+
+  auto mock_client = std::make_unique<MockRestClient>();
+  EXPECT_CALL(*mock_client,
+              Post(_, _, A<std::vector<absl::Span<char const>> const&>()))
+      .WillOnce([&](RestContext&, RestRequest const&,
+                    std::vector<absl::Span<char const>> const&) {
+        return Status(StatusCode::kInternal, "Internal Error");
+      })
+      .WillOnce([&](RestContext&, RestRequest const&,
+                    std::vector<absl::Span<char const>> const&)
+                    -> google::cloud::StatusOr<
+                        std::unique_ptr<rest_internal::RestResponse>> {
+        return std::unique_ptr<rest_internal::RestResponse>(mock_200_response);
+      });
+
+  RestContext context;
+  Request request;
+  auto result = Post<Request>(*mock_client, context, request, true, "/v1/", {});
+  EXPECT_THAT(result, StatusIs(StatusCode::kInternal));
+
+  result = Post<Request>(*mock_client, context, request, true, "/v1/", {});
+  EXPECT_STATUS_OK(result);
+}
+
 TEST(RestStubHelpers, PostWithEmptyResponse) {
   std::string json_request(kJsonUpdatePayload);
   auto* mock_200_response = new MockRestResponse();
@@ -414,11 +483,12 @@ TEST(RestStubHelpers, PostWithEmptyResponse) {
   RestContext context;
   std::vector<std::pair<std::string, std::string>> params = {
       {"response_type", proto_request.response_type()}, {"foo", "bar"}};
-  auto result =
-      Post(*mock_client, context, proto_request, true, "/v1/", params);
+  auto result = Post<EmptyResponseType>(*mock_client, context, proto_request,
+                                        true, "/v1/", params);
   EXPECT_THAT(result, StatusIs(StatusCode::kInternal));
 
-  result = Post(*mock_client, context, proto_request, true, "/v1/", params);
+  result = Post<EmptyResponseType>(*mock_client, context, proto_request, true,
+                                   "/v1/", params);
   EXPECT_THAT(result, IsOk());
 }
 
diff --git a/google/cloud/spanner/admin/internal/database_admin_rest_stub.cc b/google/cloud/spanner/admin/internal/database_admin_rest_stub.cc
@@ -213,7 +213,7 @@ Status DefaultDatabaseAdminRestStub::DropDatabase(
     google::cloud::rest_internal::RestContext& rest_context,
     Options const& options,
     google::spanner::admin::database::v1::DropDatabaseRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/",
                    request.database()));
@@ -375,7 +375,7 @@ Status DefaultDatabaseAdminRestStub::DeleteBackup(
     google::cloud::rest_internal::RestContext& rest_context,
     Options const& options,
     google::spanner::admin::database::v1::DeleteBackupRequest const& request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/",
                    request.name()));
diff --git a/google/cloud/spanner/admin/internal/instance_admin_rest_stub.cc b/google/cloud/spanner/admin/internal/instance_admin_rest_stub.cc
@@ -172,7 +172,7 @@ Status DefaultInstanceAdminRestStub::DeleteInstanceConfig(
     Options const& options,
     google::spanner::admin::instance::v1::DeleteInstanceConfigRequest const&
         request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/",
                    request.name()),
@@ -332,7 +332,7 @@ Status DefaultInstanceAdminRestStub::DeleteInstance(
     Options const& options,
     google::spanner::admin::instance::v1::DeleteInstanceRequest const&
         request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/",
                    request.name()));
@@ -431,7 +431,7 @@ Status DefaultInstanceAdminRestStub::DeleteInstancePartition(
     Options const& options,
     google::spanner::admin::instance::v1::DeleteInstancePartitionRequest const&
         request) {
-  return rest_internal::Delete(
+  return rest_internal::Delete<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, false,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/",
                    request.name()),
diff --git a/google/cloud/sql/v1/internal/sql_operations_rest_stub.cc b/google/cloud/sql/v1/internal/sql_operations_rest_stub.cc
@@ -71,7 +71,7 @@ Status DefaultSqlOperationsServiceRestStub::Cancel(
     google::cloud::rest_internal::RestContext& rest_context,
     Options const& options,
     google::cloud::sql::v1::SqlOperationsCancelRequest const& request) {
-  return rest_internal::Post(
+  return rest_internal::Post<google::cloud::rest_internal::EmptyResponseType>(
       *service_, rest_context, request, true,
       absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/",
                    "projects", "/", request.project(), "/", "operations", "/",

Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ Status DefaultGoldenRestOnlyRestStub::Noop(`
`45`	`45`	`google::cloud::rest_internal::RestContext& rest_context,`
`46`	`46`	`Options const& options,`
`47`	`47`	`google::protobuf::Empty const& request) {`
`48`		`- return rest_internal::Post(`
	`48`	`+ return rest_internal::Post<google::cloud::rest_internal::EmptyResponseType>(`
`49`	`49`	`*service_, rest_context, request, false,`
`50`	`50`	`absl::StrCat("/", rest_internal::DetermineApiVersion("v1", options), "/noop"));`
`51`	`51`	`}`
Original file line number	Diff line number	Diff line change
`@@ -389,7 +389,7 @@ Status Default$stub_rest_class_name$::$method_name$(`
`389`	`389`	`google::cloud::rest_internal::RestContext& rest_context,`
`390`	`390`	`Options const& options,`
`391`	`391`	`$request_type$ const& request) {`
`392`		`- return rest_internal::$method_http_verb$(`
	`392`	`+ return rest_internal::$method_http_verb$<google::cloud::rest_internal::EmptyResponseType>(`
`393`	`393`	`*service_, rest_context, $request_resource$, $preserve_proto_field_names_in_json$,`
`394`	`394`	`$method_rest_path$$method_http_query_parameters$);`
`395`	`395`	`}`