Improvement for `CustomHeader` in `REST` and `gRPC` for `GCS`

[vnghia]

What component of google-cloud-cpp is this feature request for? google/cloud/storage

---

In TensorFlow, I am using google-cloud-cpp to intereact with GCS.

• In the old implementation ( without google-cloud-cpp), TensorFlow used an env to insert an addional header to the request.
• But in google-cloud-cpp, because of variadic template, I find it hard to pass an additonal header to the request ( since the presence of that header must be know at compile-time ). In addition, to me, it isn't possible to pass an unknown number of CustomHeader. This might be useful for TF users since they couldn't use google-cloud-cpp directly ( therefore, they couldn't pass CustomEncryptionKey, ... ).

I propose that we add a second constructor for CustomHeader that takes a std::unordered_map<std::string, std::string>, and keep that std::unordered_map<std::string, std::string> as a private member. In curl_request_builder.h, we traverse the map and call AddHeader on each key.

---

In gRPC, we have something called metadata ( HTTP 2 header ). It is useless for production but it might be useful for testing. I think we could use CustomHeader to inject metadata to gRPC request ( something like x-goog-emulator-instruction) and eventually, we could use the tests for REST to test gRPC against the emulator.

[coryan]

I really like the idea of supplying multiple custom headers.

The variadic template should not be a major issue, the default value for any "option" is "not set" so you can always insert them and they have no effect, for example, you could do something like this:

void F(gcs::Client cl, bool add_header, std::string key, std::string value) {
  auto option = gcs::CustomHeader{};
  if (add_header) {
    option = gcs::CustomHeader(key, value);
  }
  cl.InsertObject("my-bucket", "my-object", "Hello World\n", std::move(option)).value();
}

[vnghia]

a second constructor for CustomHeader that takes a std::unordered_map<std::string, std::string>

I really like the idea of supplying multiple custom headers.

So what do you think about the std::unordered_map ?

I think we could use CustomHeader to inject metadata to gRPC request

What do you think about this improvement ?

[coryan]

a second constructor for CustomHeader that takes a std::unordered_map<std::string, std::string>

I really like the idea of supplying multiple custom headers.

So what do you think about the std::unordered_map ?

I do not like idea of supporting multiple CustomHeader values, I am not sure using std::unordered_map<> by itself is enough.

If the application says:

void F(gcs::Client client) {
  client.InsertObject(bucket, object, contents,
      gcs::CustomHeader("x-goog-custom1", "blah"), gcs::CustomHeader("x-goog-custom2", "foo"));
}

I think we want both headers to be merged, as opposed to just the last one taking effect. I know the application could have said:

void F(gcs::Client client) {
  client.InsertObject(bucket, object, contents,
      gcs::CustomHeader({{"x-goog-custom1", "blah"}, {"x-goog-custom2", "foo"}}));
}

Moreover, CustomHeader is a WellKnownOption today, and those are magically inserted into a request using GenericRequestBase<>::AddOptionsToHttpRequest().

Finally, there is the question of whether an unordered map is a good representation, as headers can be repeated in a HTTP request.

In short, making all of this work would require some finesse. I think the steps are roughly:

1. We need to represent all the custom headers by a new property (most likely just a member variable) at the root of the GenericRequestBase<> hierarchy, this would probably be a std::vector<std::pair<std::string, std::string>> to support duplicate headers.
2. CustomHeader becomes a internal::ComplexOption<> that always appends values to this new property.
3. We need to introduce a ClearCustomHeaders<> to clear the existing ones and maybe OverrideCustomHeaders<> to replace the list of custom headers with a different list.
4. We need to change AddOptionsToHttpRequest to work with this new scheme.

But I am not 100% sure this is all correct, one would need to prototype it and see if it works and then adapt if it does not.

I think we could use CustomHeader to inject metadata to gRPC request

What do you think about this improvement ?

That sounds like a great idea. But we need to merge it with the other changes.

[vnghia]

But I am not 100% sure this is all correct, one would need to prototype it and see if it works and then adapt if it does not.

I think I will just leave it here.

That sounds like a great idea. But we need to merge it with the other changes.

I could not find something like SetCommonOptions for gRPC. It would be useful for not only CustomHeader but also other options. Since we haves something like this in each gRPC request

CommonRequestParams common_request_params = 9;
CommonObjectRequestParams common_object_request_params = 8; // Object-related operations

We should consider adding 2 functions:

// context is used when `CustomHeader` is present.
void SetCommonOptions(grpc::ClientContext& context, Message& proto, Request& const request);
void SetCommonObjectOptions(Message& proto, Request& const request);

WDYT ?

[coryan]

We should consider adding 2 functions:

// context is used when `CustomHeader` is present.
void SetCommonOptions(grpc::ClientContext& context, Message& proto, Request& const request);
void SetCommonObjectOptions(Message& proto, Request& const request);

WDYT ?

Sounds like a good idea.

[coryan]

Punt for now, still sounds like a good idea, but no time right now.

[coryan]

Realistically we do not have time to work on this.