googleapis / gax-python

Google API Extensions for Python
http://gax-python.readthedocs.org/en/latest/
BSD 3-Clause "New" or "Revised" License
25 stars 13 forks source link

Fix x-goog-api-client header inconsistency for spanner client #193

Closed vam-google closed 7 years ago

vam-google commented 7 years ago

Merges tuples in settings.kwargs['metadata'] propertly. For example for the input of:

options.kwargs = {metadata: [('x-goog-api-client', 'val1')]}
settings.kwargs = {metadata: [('google-cloud-resource-prefix', 'val2'), ('x-goog-api-client', 'val3')]}

The merged result will be:

this_settings.kwargs = {metadata: [('x-goog-api-client', 'val1'), ('google-cloud-resource-prefix', 'val2')]}
vam-google commented 7 years ago

Note, I checked NodeJS and it already has correct headers (though google-cloud-resource-prefix seems missing, now I'm not sure if that header is required at all). @geigerj checked Ruby earlier and it also already has correct headers.

So currently among Python/Ruby/NodeJS the issue is present only in Python, and this PR is intended to fix it. I believe one of the reasons of why Python is different is because it uses python's tuples to represent metadata, and tuples are unique to Python.

geigerj commented 7 years ago

@vam-google I think I'm missing some context on this PR. I indeed checked Ruby earlier, but nowhere in the Ruby manual layer are we setting the metadata on a per-RPC basis using CallOptions (to my knowledge, at least). I only checked one RPC for each of the admin services and the data client, so I would not have picked up on configuration with this level of granularity anyway. It's not clear to me what the use case for configuring per-RPC metadata is, anyway, in the context of Spanner.

EDIT: I understand this sort of per-RPC metadata may be needed for header regionalization -- I'm just confused about how this relates to Spanner.

vam-google commented 7 years ago

I believe the main problem maker here is the google-cloud-resource-prefix header, which is sent from the spanner client only. I'm not sure if we still need to send it or no (because NodeJS, for example, does not send it, but Java and Python do).

So in python, the spanner client also sets google-cloud-resource-prefix header in the kwargs['metadata'], and that is how the gax's x-goog-api-client header disappears from the request (because that one is also set in kwargs['metadata'], and one overwrites another).

I would assume, that ruby does not send the google-cloud-resource-prefix header, and that is why things work fine.

So another way of fixing spanner headers would be just to stop sending google-cloud-resource-prefix if it is not required (and maybe it is not). But even in that case, I believe merging metadata list would still be a reasonable feature of the gax lib.

Note, google-cloud-resource-prefix is specific to request, not to client itself, that is why it is configured per-RPC.

vam-google commented 7 years ago

Sure, I'll add the test. @lukesneeringer please take a look at this PR.

vam-google commented 7 years ago

Added test, PTAL

vam-google commented 7 years ago

PTAL