Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

Commit 5b4bc83

Browse files
taimo3810taimo3810
committed
fix: prevent URL-encoding of $alt query parameter in REST transport
The `requests` library encodes '$' as '%24' when using the `params` argument, which causes API errors for parameters like '$alt'. This fix manually builds the URL query string using `urlencode` with `safe="$"` to preserve the '$' character. Changes: - Build query string manually in _get_response method - Add urlencode import to REST transport templates - Update tests to verify query params in URL - Add unit tests for URL query params encoding Fixes: https://github.com/googleapis/gapic-generator-python/issues/2514
1 parent d20dd28 commit 5b4bc83

File tree

8 files changed

+139
-14
lines changed

8 files changed

+139
-14
lines changed

‎gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/_shared_macros.j2‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -171,16 +171,23 @@ def _get_http_options():
171171
timeout,
172172
transcoded_request,
173173
body=None):
174-
174+
175175
uri = transcoded_request['uri']
176176
method = transcoded_request['method']
177177
headers = dict(metadata)
178178
headers['Content-Type'] = 'application/json'
179+
# Build query string manually to avoid URL-encoding special characters like '$'.
180+
# The `requests` library encodes '$' as '%24' when using the `params` argument,
181+
# which causes API errors for parameters like '$alt'. See:
182+
# https://github.com/googleapis/gapic-generator-python/issues/2514
183+
_query_params = rest_helpers.flatten_query_params(query_params, strict=True)
184+
_request_url = "{host}{uri}".format(host=host, uri=uri)
185+
if _query_params:
186+
_request_url = "{}?{}".format(_request_url, urlencode(_query_params, safe="$"))
179187
response = {{ await_prefix }}getattr(session, method)(
180-
"{host}{uri}".format(host=host, uri=uri),
188+
_request_url,
181189
timeout=timeout,
182190
headers=headers,
183-
params=rest_helpers.flatten_query_params(query_params, strict=True),
184191
{% if body_spec %}
185192
data=body,
186193
{% endif %}

‎gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/transports/rest.py.j2‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,13 @@ from google.iam.v1 import policy_pb2 # type: ignore
3333
from google.cloud.location import locations_pb2 # type: ignore
3434
{% endif %}
3535

36-
from requests import __version__ as requests_version
3736
import dataclasses
3837
from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, Union
38+
from urllib.parse import urlencode
3939
import warnings
4040

41+
from requests import __version__ as requests_version
42+
4143
{{ shared_macros.operations_mixin_imports(api, service, opts) }}
4244

4345
from .rest_base import _Base{{ service.name }}RestTransport

‎gapic/ads-templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2‎

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ import grpc
1818
from grpc.experimental import aio
1919
{% if "rest" in opts.transport %}
2020
from collections.abc import Iterable
21+
import urllib.parse
22+
2123
from google.protobuf import json_format
2224
import json
2325
{% endif %}
@@ -45,6 +47,7 @@ from google.api_core import client_options
4547
from google.api_core import exceptions as core_exceptions
4648
from google.api_core import grpc_helpers
4749
from google.api_core import path_template
50+
from google.api_core import rest_helpers
4851
from google.api_core import retry as retries
4952
{% if service.has_lro %}
5053
from google.api_core import future
@@ -1451,8 +1454,12 @@ def test_{{ method_name }}_rest_required_fields(request_type={{ method.input.ide
14511454
('$alt', 'json;enum-encoding=int')
14521455
{% endif %}
14531456
]
1454-
actual_params = req.call_args.kwargs['params']
1455-
assert expected_params == actual_params
1457+
# Verify query params are correctly included in the URL
1458+
# Session.request is called as request(method, url, ...), so url is args[1]
1459+
actual_url = req.call_args.args[1]
1460+
parsed_url = urllib.parse.urlparse(actual_url)
1461+
actual_params = urllib.parse.parse_qsl(parsed_url.query, keep_blank_values=True)
1462+
assert set(expected_params).issubset(set(actual_params))
14561463

14571464

14581465
def test_{{ method_name }}_rest_unset_required_fields():
@@ -1461,9 +1468,55 @@ def test_{{ method_name }}_rest_unset_required_fields():
14611468
unset_fields = transport.{{ method.transport_safe_name|snake_case }}._get_unset_required_fields({})
14621469
assert set(unset_fields) == (set(({% for param in method.query_params|sort %}"{{ param|camel_case }}", {% endfor %})) & set(({% for param in method.input.required_fields %}"{{param.name|camel_case}}", {% endfor %})))
14631470

1464-
14651471
{% endif %}{# required_fields #}
14661472

1473+
1474+
def test_{{ method_name }}_rest_url_query_params_encoding():
1475+
# Verify that special characters like '$' are correctly preserved (not URL-encoded)
1476+
# when building the URL query string. This tests the urlencode call with safe="$".
1477+
transport = transports.{{ service.rest_transport_name }}(credentials=ga_credentials.AnonymousCredentials)
1478+
method_class = transport.{{ method.transport_safe_name|snake_case }}.__class__
1479+
# Get the _get_response static method from the method class
1480+
get_response_fn = method_class._get_response
1481+
1482+
mock_session = mock.Mock()
1483+
mock_response = mock.Mock()
1484+
mock_response.status_code = 200
1485+
mock_session.get.return_value = mock_response
1486+
mock_session.post.return_value = mock_response
1487+
mock_session.put.return_value = mock_response
1488+
mock_session.patch.return_value = mock_response
1489+
mock_session.delete.return_value = mock_response
1490+
1491+
# Mock flatten_query_params to return query params that include '$' character
1492+
with mock.patch.object(rest_helpers, 'flatten_query_params') as mock_flatten:
1493+
mock_flatten.return_value = [('$alt', 'json;enum-encoding=int'), ('foo', 'bar')]
1494+
1495+
transcoded_request = {
1496+
'uri': '/v1/test',
1497+
'method': '{{ method.http_options[0].method }}',
1498+
}
1499+
1500+
get_response_fn(
1501+
host='https://example.com',
1502+
metadata=[],
1503+
query_params={},
1504+
session=mock_session,
1505+
timeout=None,
1506+
transcoded_request=transcoded_request,
1507+
)
1508+
1509+
# Verify the session method was called with the URL containing query params
1510+
session_method = getattr(mock_session, '{{ method.http_options[0].method }}')
1511+
assert session_method.called
1512+
1513+
# The URL should contain '$alt' (not '%24alt') because safe="$" is used
1514+
call_url = session_method.call_args.args[0]
1515+
assert '$alt=json' in call_url
1516+
assert '%24alt' not in call_url
1517+
assert 'foo=bar' in call_url
1518+
1519+
14671520
{% if not method.client_streaming %}
14681521
@pytest.mark.parametrize("null_interceptor", [True, False])
14691522
def test_{{ method_name }}_rest_interceptors(null_interceptor):

‎gapic/templates/%namespace/%name_%version/%sub/services/%service/_shared_macros.j2‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,16 +164,23 @@ def _get_http_options():
164164
timeout,
165165
transcoded_request,
166166
body=None):
167-
167+
168168
uri = transcoded_request['uri']
169169
method = transcoded_request['method']
170170
headers = dict(metadata)
171171
headers['Content-Type'] = 'application/json'
172+
# Build query string manually to avoid URL-encoding special characters like '$'.
173+
# The `requests` library encodes '$' as '%24' when using the `params` argument,
174+
# which causes API errors for parameters like '$alt'. See:
175+
# https://github.com/googleapis/gapic-generator-python/issues/2514
176+
_query_params = rest_helpers.flatten_query_params(query_params, strict=True)
177+
_request_url = "{host}{uri}".format(host=host, uri=uri)
178+
if _query_params:
179+
_request_url = "{}?{}".format(_request_url, urlencode(_query_params, safe="$"))
172180
response = {{ await_prefix }}getattr(session, method)(
173-
"{host}{uri}".format(host=host, uri=uri),
181+
_request_url,
174182
timeout=timeout,
175183
headers=headers,
176-
params=rest_helpers.flatten_query_params(query_params, strict=True),
177184
{% if body_spec %}
178185
data=body,
179186
{% endif %}

‎gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,13 @@ from google.iam.v1 import policy_pb2 # type: ignore
2727
from google.cloud.location import locations_pb2 # type: ignore
2828
{% endif %}
2929

30-
from requests import __version__ as requests_version
3130
import dataclasses
3231
from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, Union
32+
from urllib.parse import urlencode
3333
import warnings
3434

35+
from requests import __version__ as requests_version
36+
3537
{{ shared_macros.operations_mixin_imports(api, service, opts) }}
3638

3739
from .rest_base import _Base{{ service.name }}RestTransport

‎gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest_asyncio.py.j2‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,10 @@ from google.iam.v1 import policy_pb2 # type: ignore
4848
from google.cloud.location import locations_pb2 # type: ignore
4949
{% endif %}
5050

51-
import json # type: ignore
5251
import dataclasses
52+
import json # type: ignore
5353
from typing import Any, Dict, List, Callable, Tuple, Optional, Sequence, Union
54+
from urllib.parse import urlencode
5455

5556
{{ shared_macros.operations_mixin_imports(api, service, opts) }}
5657

‎gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ import grpc
2121
from grpc.experimental import aio
2222
{% if "rest" in opts.transport %}
2323
from collections.abc import Iterable, AsyncIterable
24+
import urllib.parse
25+
2426
from google.protobuf import json_format
2527
{% endif %}
2628
import json
@@ -72,6 +74,7 @@ from google.api_core import exceptions as core_exceptions
7274
from google.api_core import grpc_helpers
7375
from google.api_core import grpc_helpers_async
7476
from google.api_core import path_template
77+
from google.api_core import rest_helpers
7578
from google.api_core import retry as retries
7679
{% if service.has_lro or service.has_extended_lro %}
7780
from google.api_core import future

‎gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,8 +1200,12 @@ def test_{{ method_name }}_rest_required_fields(request_type={{ method.input.ide
12001200
('$alt', 'json;enum-encoding=int')
12011201
{% endif %}
12021202
]
1203-
actual_params = req.call_args.kwargs['params']
1204-
assert expected_params == actual_params
1203+
# Verify query params are correctly included in the URL
1204+
# Session.request is called as request(method, url, ...), so url is args[1]
1205+
actual_url = req.call_args.args[1]
1206+
parsed_url = urllib.parse.urlparse(actual_url)
1207+
actual_params = urllib.parse.parse_qsl(parsed_url.query, keep_blank_values=True)
1208+
assert set(expected_params).issubset(set(actual_params))
12051209

12061210

12071211
def test_{{ method_name }}_rest_unset_required_fields():
@@ -1213,6 +1217,52 @@ def test_{{ method_name }}_rest_unset_required_fields():
12131217
{% endif %}{# required_fields #}
12141218

12151219

1220+
def test_{{ method_name }}_rest_url_query_params_encoding():
1221+
# Verify that special characters like '$' are correctly preserved (not URL-encoded)
1222+
# when building the URL query string. This tests the urlencode call with safe="$".
1223+
transport = transports.{{ service.rest_transport_name }}(credentials=ga_credentials.AnonymousCredentials)
1224+
method_class = transport.{{ method.transport_safe_name|snake_case }}.__class__
1225+
# Get the _get_response static method from the method class
1226+
get_response_fn = method_class._get_response
1227+
1228+
mock_session = mock.Mock()
1229+
mock_response = mock.Mock()
1230+
mock_response.status_code = 200
1231+
mock_session.get.return_value = mock_response
1232+
mock_session.post.return_value = mock_response
1233+
mock_session.put.return_value = mock_response
1234+
mock_session.patch.return_value = mock_response
1235+
mock_session.delete.return_value = mock_response
1236+
1237+
# Mock flatten_query_params to return query params that include '$' character
1238+
with mock.patch.object(rest_helpers, 'flatten_query_params') as mock_flatten:
1239+
mock_flatten.return_value = [('$alt', 'json;enum-encoding=int'), ('foo', 'bar')]
1240+
1241+
transcoded_request = {
1242+
'uri': '/v1/test',
1243+
'method': '{{ method.http_options[0].method }}',
1244+
}
1245+
1246+
get_response_fn(
1247+
host='https://example.com',
1248+
metadata=[],
1249+
query_params={},
1250+
session=mock_session,
1251+
timeout=None,
1252+
transcoded_request=transcoded_request,
1253+
)
1254+
1255+
# Verify the session method was called with the URL containing query params
1256+
session_method = getattr(mock_session, '{{ method.http_options[0].method }}')
1257+
assert session_method.called
1258+
1259+
# The URL should contain '$alt' (not '%24alt') because safe="$" is used
1260+
call_url = session_method.call_args.args[0]
1261+
assert '$alt=json' in call_url
1262+
assert '%24alt' not in call_url
1263+
assert 'foo=bar' in call_url
1264+
1265+
12161266
{% if method.flattened_fields and not method.client_streaming %}
12171267
def test_{{ method_name }}_rest_flattened():
12181268
client = {{ service.client_name }}(

0 commit comments

Comments
 (0)