googleapis / gax-php

Google API Extensions for PHP
http://googleapis.github.io/gax-php/
BSD 3-Clause "New" or "Revised" License
237 stars 52 forks source link

Different string case used for metadata field names using REST and gRPC transports #366

Open PierrickVoulet opened 2 years ago

PierrickVoulet commented 2 years ago

Environment details

Steps to reproduce

Using the google-ads-php library:

  1. Send a request with transport REST and print metadata
// --- Code

$googleAdsClient = (new GoogleAdsClientBuilder())
    ->fromFile()
    ->withOAuth2Credential((new OAuth2TokenBuilder())->fromFile()->build())
    ->withTransport('rest')
    ->build();
[$result, $metadata] = $googleAdsClient->getCustomerServiceClient()
    ->listAccessibleCustomers(['withResponseMetadata' => true]);
var_dump($metadata->getMetadata());

// --- Console

array(11) {
  'Request-Id' =>
  array(1) {
    [0] =>
    string(22) "TIhklBj3MxkjU63id_3jBQ"
  }
  'Content-Type' =>
  array(1) {
    [0] =>
    string(31) "application/json; charset=UTF-8"
  }
  // ...
}
  1. Send request with transport gRPC and print metadata
// --- Code

$googleAdsClient = (new GoogleAdsClientBuilder())
    ->fromFile()
    ->withOAuth2Credential((new OAuth2TokenBuilder())->fromFile()->build())
    ->withTransport('grpc')
    ->build();
[$result, $metadata] = $googleAdsClient->getCustomerServiceClient()
    ->listAccessibleCustomers(['withResponseMetadata' => true]);
var_dump($metadata->getMetadata());

// --- Console

array(3) {
  'content-disposition' =>
  array(1) {
    [0] =>
    string(10) "attachment"
  }
  'request-id' =>
  array(1) {
    [0] =>
    string(22) "s__1d5ubcTbWABXtJNVW2w"
  }
  'date' =>
  array(1) {
    [0] =>
    string(29) "Tue, 25 Jan 2022 19:57:52 GMT"
  }
}
  1. Notice that the request ID is returned as Request-Id with REST and request-id with gRPC.

Could you confirm if this works as intended? Due to this difference in behavior between the two transports we had to add some complexity in our client library which is not optimal.

PierrickVoulet commented 2 years ago

FYI @fiboknacky @aohren

vam-google commented 2 years ago

I believe it was discussed and agreed that it was ok, as headers are case-insensitive according to http spec. @noahdietz I guess it works as intended, doesn't it?

noahdietz commented 2 years ago

Right HTTP spec says headers are case insensitive but in gRPC, I believe the implementation forces metadata keys to be lowercase.

I don't particularly like forcing one transport to behave like another because there is a dependency on emergent behavior. However, it does make it hard for clients to normalize keys before accessing them.

@bshaffer WDYT about forcing REST header keys to lowercase?

bshaffer commented 2 years ago

Unless there's an actual error or implementation issue that I'm missing, I'd prefer to call this WAD

aohren commented 2 years ago

Is it possible to preserve the casing that came over the wire? While sure case-insensitivity conforms to the spec, it would be a lot more useful for client libraries / users if this lib didn't destroy the original case. This artificially limits what it can be used for. Say you were trying to use the client library in a network level debugging tool (to find issues literally related to, say, header casing on one of your servers). Then you couldn't use this library to debug that problem if the library itself is also mangling the casing.

If this were a higher level library, this wouldn't be as important and I'd agree conforming to a spec is good enough. But since this is client library plumbing, it's not unreasonable to think users could try to use it to debug their own networking issues. Why preclude that use case by re-writing the casing in gax?