-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Changes from 8 commits
3bd9c45
a08a86e
8198d86
72d9b0b
8324f56
4cb950f
6dde7f0
c2c5e2d
ced6aaf
2bb425f
a4c94c0
62612c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -222,16 +222,20 @@ 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 | ||
in your request. We automatically format your query params into the inputted url. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would we format something like 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] | ||
if "query" in kwargs: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's just to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If query is not set, we don't want to call self.format_parameters, right? (given self.format_parameters() has side effect even the input is None) |
||
self.format_parameters(kwargs.pop("query")) | ||
|
||
def __repr__(self): | ||
return "<HttpRequest [%s]>" % (self.method) | ||
|
@@ -290,11 +294,18 @@ def _format_data(data): | |
def format_parameters(self, params): | ||
# type: (Dict[str, str]) -> None | ||
"""Format parameters into a valid query string. | ||
|
||
It's assumed all parameters have already been quoted as | ||
valid URL strings. | ||
|
||
Passing your query parameters | ||
directly through the `query` kwarg of | ||
:class:`~azure.core.pipeline.transport.HttpRequest` automatically | ||
calls this method for you. | ||
|
||
:param dict params: A dictionary of parameters. | ||
""" | ||
params = params or {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lmazuel want to call this out. This way, if users pass in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If params is not a dict, what's our expected behavior? and in which case, user will set it explicit None? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. params is typed as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be able to pass in a number or a boolean query parameter value, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's true, let me change this to |
||
query = urlparse(self.url).query | ||
if query: | ||
self.url = self.url.partition("?")[0] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -285,7 +285,7 @@ def test_request_url_with_params(self): | |
request.format_parameters({"g": "h"}) | ||
|
||
self.assertIn(request.url, ["a/b/c?g=h&t=y", "a/b/c?t=y&g=h"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_params_as_list(self): | ||
|
||
request = HttpRequest("GET", "/") | ||
|
@@ -300,14 +300,46 @@ def test_request_url_with_params_with_none_in_list(self): | |
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): | ||
|
||
def test_request_url_with_params_with_none_value(self): | ||
|
||
request = HttpRequest("GET", "/") | ||
request.url = "a/b/c?t=y" | ||
with pytest.raises(ValueError): | ||
request.format_parameters({"g": None}) | ||
|
||
def test_request_url_with_params_with_none(self): | ||
|
||
request = HttpRequest("GET", "/") | ||
request.url = "hello" | ||
request.format_parameters(params=None) | ||
assert request.url == "hello?" | ||
|
||
def test_request_url_with_query_kwarg(self): | ||
|
||
request = HttpRequest("GET", "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"]) | ||
|
||
def test_request_url_with_query_kwarg_as_list(self): | ||
|
||
request = HttpRequest("GET", "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_query_kwarg_with_none_in_list(self): | ||
|
||
with pytest.raises(ValueError): | ||
HttpRequest("GET", "a/b/c?t=y", query={"g": ["h",None]}) | ||
|
||
def test_request_url_with_query_kwarg_with_none_value(self): | ||
|
||
with pytest.raises(ValueError): | ||
HttpRequest("GET", "a/b/c?t=y", query={"g": None}) | ||
|
||
def test_request_url_with_query_kwarg_with_none(self): | ||
request = HttpRequest("GET", "hello", query=None) | ||
assert request.url == "hello?" | ||
|
||
def test_request_text(self): | ||
client = PipelineClientBase('http://example.org') | ||
|
There was a problem hiding this comment.
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 adict
. Many services allow/require the same query parameter multiple times (something similar tohttp://contoso.com/api?item=1&item=2&item=7
)There was a problem hiding this comment.
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 thequery
kwarg in place? Currently core only acceptsDict[str, Any]
informat_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