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

[core] add query kwarg to HttpRequest #16942

Closed
wants to merge 12 commits into from
6 changes: 5 additions & 1 deletion sdk/core/azure-core/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# Release History

## 1.11.1 (Unreleased)
## 1.12. (Unreleased)
iscai-msft marked this conversation as resolved.
Show resolved Hide resolved

### Features

- Add kwarg `query` to `HttpRequest`, which accepts your query parameters and formats them into your inputted URL. #16942


## 1.11.0 (2021-02-08)
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/azure-core/CLIENT_LIBRARY_DEVELOPER.md
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ When creating the transport, "use_env_settings" parameter can be used to enable
synchronous_transport = RequestsTransport(use_env_settings=True)
```

If "use_env_settings" is set to True(by default), the transport will look for environment variables
If "use_env_settings" is set to True(by default), the transport will look for environment variables

- HTTP_PROXY
- HTTPS_PROXY
Expand Down Expand Up @@ -193,7 +193,7 @@ The HttpRequest has the following API. It does not vary between transports:
```python
class HttpRequest(object):

def __init__(self, method, url, headers=None, files=None, data=None):
def __init__(self, method, url, headers=None, files=None, data=None, **kwargs):
self.method = method
self.url = url
self.headers = CaseInsensitiveDict(headers)
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/azure-core/azure/core/_version.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@
# regenerated.
# --------------------------------------------------------------------------

VERSION = "1.11.1"
VERSION = "1.12.0"
16 changes: 14 additions & 2 deletions sdk/core/azure-core/azure/core/pipeline/transport/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,16 +222,21 @@ class HttpRequest(object):
:param files: Files list.
:param data: Body to be sent.
:type data: bytes or str.
:keyword dict[str,any] query: A dictionary of query parameters you would like to include
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query parameter should take a sequence of key-value tuples in addition to a dict. Many services allow/require the same query parameter multiple times (something similar to http://contoso.com/api?item=1&item=2&item=7)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johanste would it be ok to keep it as Dict[str, Any] for this release so we get the query kwarg in place? Currently core only accepts Dict[str, Any] in format_parameters, so this would be at parity. For next release, I can open an issue to expand query to allow in lists of tuples etc, following httpx's possible values

in your request. We automatically format your query params into the inputted url.
Copy link
Member

@johanste johanste Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we format something like query={ 'items': [ 'a', 'b', 'c', 'd' ] }?

I believe we need to be very explicit about if/when/what we encode and how (and this should also translate into specific test cases to make sure we don't break stuff)

"""

def __init__(self, method, url, headers=None, files=None, data=None):
# type: (str, str, Mapping[str, str], Any, Any) -> None
def __init__(self, method, url, headers=None, files=None, data=None, **kwargs):
# type: (str, str, Mapping[str, str], Any, Any, Any) -> None
self.method = method
self.url = url
self.headers = _case_insensitive_dict(headers)
self.files = files
self.data = data
self.multipart_mixed_info = None # type: Optional[Tuple]
query = kwargs.pop("query", None)
if query:
self.format_parameters(query)

def __repr__(self):
return "<HttpRequest [%s]>" % (self.method)
Expand Down Expand Up @@ -290,6 +295,13 @@ def _format_data(data):
def format_parameters(self, params):
# type: (Dict[str, str]) -> None
"""Format parameters into a valid query string.

We recommend to directly pass your query parameter
iscai-msft marked this conversation as resolved.
Show resolved Hide resolved
directly through the `query` kwarg of
:class:`~azure.core.pipeline.transport.HttpRequest` instead
of calling this method, as that automatically handles
query parameter formatting for you.

It's assumed all parameters have already been quoted as
valid URL strings.

Expand Down
29 changes: 14 additions & 15 deletions sdk/core/azure-core/tests/test_pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,35 +278,34 @@ def test_request_xml(self):

assert request.data == b"<?xml version='1.0' encoding='utf-8'?>\n<root />"

def test_request_url_with_params(self):
def test_request_url_with_query(self):

request = HttpRequest("GET", url="a/b/c?t=y", query={"g": "h"})

self.assertIn(request.url, ["a/b/c?g=h&t=y", "a/b/c?t=y&g=h"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a test when there was no query in the first place (to also check we add correctly the ?


def test_request_url_with_format_parameters(self):

request = HttpRequest("GET", "/")
request.url = "a/b/c?t=y"
request.format_parameters({"g": "h"})

self.assertIn(request.url, ["a/b/c?g=h&t=y", "a/b/c?t=y&g=h"])

def test_request_url_with_params_as_list(self):

request = HttpRequest("GET", "/")
request.url = "a/b/c?t=y"
request.format_parameters({"g": ["h","i"]})
def test_request_url_with_query_params_as_list(self):

request = HttpRequest("GET", url="a/b/c?t=y", query={"g": ["h","i"]})

self.assertIn(request.url, ["a/b/c?g=h&g=i&t=y", "a/b/c?t=y&g=h&g=i"])

def test_request_url_with_params_with_none_in_list(self):
def test_request_url_with_query_params_with_none_in_list(self):

request = HttpRequest("GET", "/")
request.url = "a/b/c?t=y"
with pytest.raises(ValueError):
request.format_parameters({"g": ["h",None]})

def test_request_url_with_params_with_none(self):
HttpRequest("GET", "/", query={"g": ["h",None]})

request = HttpRequest("GET", "/")
request.url = "a/b/c?t=y"
def test_request_url_with_query_params_with_none(self):
with pytest.raises(ValueError):
request.format_parameters({"g": None})
HttpRequest("GET", "/", query={"g": None})
iscai-msft marked this conversation as resolved.
Show resolved Hide resolved


def test_request_text(self):
Expand Down