Support mocking for the storage library without exposing implementation details

[coryan]

Background

Some customers need to "mock" the storage library for their own testing. We have examples on how to to this, but they use names in the google::cloud::storage::internal:: namespace:

google-cloud-cpp/google/cloud/storage/examples/storage_client_mock_samples.cc

Lines 116 to 145 in ee2e0e5

	TEST(StorageMockingSamples, MockReadObjectFailure) {
	namespace gcs = google::cloud::storage;
	std::shared_ptr<gcs::testing::MockClient> mock =
	std::make_shared<gcs::testing::MockClient>();
	auto client = gcs::testing::ClientFromMock(mock);
	std::string text = "this is a mock http response";
	EXPECT_CALL(*mock, ReadObject)
	.WillOnce([](gcs::internal::ReadObjectRangeRequest const& request) {
	EXPECT_EQ(request.bucket_name(), "mock-bucket-name") << request;
	auto* mock_source = new gcs::testing::MockObjectReadSource;
	::testing::InSequence seq;
	EXPECT_CALL(*mock_source, IsOpen).WillRepeatedly(Return(true));
	EXPECT_CALL(*mock_source, Read)
	.WillOnce(Return(google::cloud::Status(
	google::cloud::StatusCode::kInvalidArgument,
	"Invalid Argument")));
	EXPECT_CALL(*mock_source, IsOpen).WillRepeatedly(Return(false));
	std::unique_ptr<gcs::internal::ObjectReadSource> result(mock_source);
	return google::cloud::make_status_or(std::move(result));
	});
	gcs::ObjectReadStream stream =
	client.ReadObject("mock-bucket-name", "mock-object-name");
	EXPECT_TRUE(stream.bad());
	stream.Close();
	}

Normally gcs::internal:: would be reserved for implementation details, and would be subject to change without notice. Exposing and documenting how they work muddles the message to our customers ("don't use internal because it changes, except for these bits, maybe") and complicate the life for the library developers ("these bits of internal need to remain backwards compatible").

Even when the types and functions do not change, the sequence of calls may change, possibly breaking a mock, for an example see #6983 and #7042.

Overview

I think we should change the mechanism used to mock the gcs::Client class. We should define a public API for mocking. Keeping in mind that there are 50 odd RPCs in the GCS surface, we should also try to avoid defining one *Request and *Response type for each RPC. Specially considering there are half a dozen or more "optional" parameters with each RPC.

I think we should define the following APIs.

First, a class that applications can use to mock gcs::Client, the class would live in a public namespace, such as:

namespace google::cloud::storage_mocks {
class MockClient {
public:
  // details about methods below
};
} // namespace google::cloud::storage_mocks

Note that this class does not require an interface. It does not expose any of the APIs defined in gcs::internal.

Second, a factory function to use this class from testing code:

namespace google::cloud::storage_mocks {

google::cloud::storage::Client MakeMockClient(std::shared_ptr<MockClient> mock);

} // namespace google::cloud::storage_mocks

Applications do not need to know how a MockClient is mapped to a client (we will describe that below), but once they have a gcs::Client object they can use it in their test code as usual.

Third, all the methods for MockClient will have exactly the same signature:

namespace google::cloud::storage_mocks {
class MockClient {
public:
  MOCK_METHOD(MockHttpResponse, GetObjectMetadata, (MockHttpRequest const&), ());
};
} // namespace google::cloud::storage_mocks

The HttpResponse and HttpRequest classes are provided as part of the mocking library. I think we should just clone the API (but not the implementation) from the functions framework:

class MockHttpRequest {
 public:
  // Each field has an accessor and modifier, the modifiers can be chained:
  std::string const& payload() const;
  MockHttpRequest&& set_payload(std::string v) &&;

  // Some member functions omitted for brevity:
  MockHttpRequest&& set_verb(std::string v) &&;
  MockHttpRequest&& add_query_parameter(std::string k, std::string v) &&;
  MockHttpRequest&& add_header(std::string k, std::string v) &&;
};

class MockHttpResponse {
 public:
  // Each field has an accessor and modifier, the modifiers can be chained:
  std::string const& payload() const;
  MockHttpRequest&& set_payload(std::string v) &&;

  // Some member functions omitted for brevity:
  MockHttpRequest&& set_status_code(std::string v) &&;
  MockHttpRequest&& add_header(std::string k, std::string v) &&;
};

These are inspired by the analogous classes in the functions framework:

https://github.com/GoogleCloudPlatform/functions-framework-cpp/blob/bf2ba9a4365a583a95e948e26d1bfff4db6a491f/google/cloud/functions/http_request.h#L31-L116

https://github.com/GoogleCloudPlatform/functions-framework-cpp/blob/bf2ba9a4365a583a95e948e26d1bfff4db6a491f/google/cloud/functions/http_response.h#L38-L81

Mocking ReadObject

ReadObject is a "streaming" RPC. For this proposal I think we should just consider it a regular RPC with one addition: the HttpResponse class needs a "close status" member function that indicates the status at the end of the stream it should be possible to return a payload and a failure, to simulate errors where the download is interrupted:

class MockHttpResponse {
 public:
  Status close_status() const;
  MockHttpResponse&& set_close_status(Status v) &&;
};

This will need to be handled by the wrapper around MockClient (see below).

The MakeMockClient Function

This function would wrap the MockClient class with a internal::RawClient class each member function would look similar, so we illustrate the design using just GetObjectMetadata(). Note that most of this functionality, such as creating HTTP requests, and parsing the HTTP response, already exist as part of the HTTP implementation. And we have no plans to remove this implementation.

namespace google::cloud::storage::internal {

class MockRawClient : public RawClient {
 public:
  MockRawClient(std::shared_ptr<storage_mocks::MockClient> mock) : mock_(std::move(mock)) {
  }

  StatusOr<ObjectMetadata> GetObjectMetadata(internal::GetObjectMetadataRequest request) override {
    auto http_request = HttpReques{}.set_verb("GET").set_target(
       "/storage/v1/b/" + request.bucket() + "/o/" + request.object());
    // add headers and the optional query parameters from `request`
    AddOptionsToHttpRequest(http_request, request);
    HttpResponse response = mock_->ReadObject(http_request);
    if (response->status_code() >= 300) return AsStatus(*response);
    return MockObjectReadSource(response); // returns response->payload() and response->closing_status()
  }
};

}

MockRawClient and streaming RPCs

The one function that is more interesting in MockRawClient is ReadObject. It needs to return a custom ObjectReadSource:

StatusOr<std::unique_ptr<ObjectReadSource>>
MockRawClient::ReadObject(ReadObjectRangeRequest const&) override {
    auto http_request = HttpReques{}.set_verb("GET").set_target(
       "/storage/v1/b/" + request.bucket() + "/o/" + request.object());
    http_request.add_query_parameter("alt=media");
    // add headers and the optional query parameters from `request`
    AddOptionsToHttpRequest(http_request, request);
    auto response = mock_->GetObjectMetadata(http_request);
    if (response->status_code() >= 300) return AsStatus(*response);
    internal::ObjectMetadataParser::FromString(response->payload());
}

[coryan]

We still want this, but I have made no progress. May be easier with the move to rest-internal.

[coryan]

This will be needed for the retry header work (2022-Q4)

[coryan]

We need to revisit this before the end of 2023-H1.

[coryan]

Still want to do this before we create a 3.0 release.

[coryan]

Punt.

[dbolduc]

We don't have time to do this. We are focusing on the async client.