Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
PierrickVoulet opened this issue Jan 25, 2022 · 5 comments
Assignees
Labels
type: question Request for information or clarification. Not an issue.

Comments

@PierrickVoulet
Copy link
Contributor

Environment details

  • OS: Linux 64
  • PHP version: 8.0.1
  • Package name and version: gax-php, 1.11.2

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
Copy link
Contributor Author

FYI @fiboknacky @aohren

@PierrickVoulet PierrickVoulet changed the title Different string cases are used for metadata field names using REST and gRPC transports Different string case used for metadata field names using REST and gRPC transports Jan 25, 2022
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Jan 26, 2022
@vam-google
Copy link
Contributor

vam-google commented Jan 31, 2022

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?

@vam-google vam-google added the type: question Request for information or clarification. Not an issue. label Jan 31, 2022
@noahdietz noahdietz removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Jan 31, 2022
@noahdietz
Copy link
Contributor

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
Copy link
Contributor

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

@aohren
Copy link

aohren commented Feb 1, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

6 participants