[core] add query kwarg to HttpRequest #16942

iscai-msft · 2021-02-25T19:02:39Z

sdk/core/azure-core/CHANGELOG.md

sdk/core/azure-core/azure/core/pipeline/transport/_base.py

lmazuel · 2021-02-25T23:33:47Z

sdk/core/azure-core/tests/test_pipeline.py

+
+        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"])


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

sdk/core/azure-core/tests/test_pipeline.py

iscai-msft · 2021-02-26T15:41:11Z

sdk/core/azure-core/azure/core/pipeline/transport/_base.py

        :param dict params: A dictionary of parameters.
        """
+        params = params or {}


@lmazuel want to call this out. This way, if users pass in params=None or query=None from the kwarg in HttpRequest, we don't throw a None error and we tack on the ? to the end of the url

If params is not a dict, what's our expected behavior? and in which case, user will set it explicit None?

params is typed as a Dict[str, str]. i'm not quite sure about the use case of users passing inNone for query, but I added this bc @lmazuel wanted to make sure if query is None, we add the ?` at the end of the url

You should be able to pass in a number or a boolean query parameter value, no?

that's true, let me change this to Dict[str, Any]

johanste · 2021-03-02T20:54:46Z

sdk/core/azure-core/azure/core/pipeline/transport/_base.py

        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:


Let's just to a pop with a default value. No need to look it up twice :).

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)

johanste · 2021-03-02T20:55:53Z

sdk/core/azure-core/azure/core/pipeline/transport/_base.py

@@ -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.


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)

johanste · 2021-03-02T21:02:07Z

sdk/core/azure-core/azure/core/pipeline/transport/_base.py

@@ -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


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)

@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

johanste

We should take a look at what other HTTP stacks do (e.g. requests parameters and httpx's params) before we lock down this API.

check-enforcer · 2021-03-04T18:35:32Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run python - [service] - ci

…into add_query_kwarg * 'master' of https://github.com/Azure/azure-sdk-for-python: (69 commits) march release (Azure#16966) Release of Device Update for IoT Hub SDK for Python. (Azure#17005) Add Get-AllPackageInfoFromRepo (Azure#16947) Track1 package is incorrectly set as track2 (Azure#17075) [text analytics] add normalized_text (Azure#17074) Renaming with_token identity function (Azure#17066) Adapt to azure core's cloud event (Azure#17063) align perf tests with js (Azure#17069) [Perfstress][Storage] Added FileShare perf tests (Azure#15834) [formrecognizer] Adding custom forms perf test (Azure#16969) Fix LanguageShort typo (Azure#17068) sas creds updates (Azure#17065) [eventgrid] Fix Sample eh (Azure#17064) [Perfstress][Storage] Added Datalake perf tests (Azure#15861) [text analytics] Healthcare n-ary relations (Azure#16997) ServiceBus dict-representation acceptance and kwarg-update functionality (Azure#14807) [text analytics] add perf tests (Azure#17060) Add cloud event to core (Azure#16800) [Perf] Small fixes to storage-blob (Azure#17055) [EG] Regenerate Code (Azure#17053) ...

…into add_query_kwarg * 'master' of https://github.com/Azure/azure-sdk-for-python: EG - more docs imrpovement (Azure#17079) [EventHubs] add logging.info to warn the usage of partition key of non-string type (Azure#17057)

iscai-msft · 2021-03-04T22:30:51Z

putting on the backburner, so will close this PR

add query kwarg to core

3bd9c45

iscai-msft requested review from lmazuel and xiangyan99 as code owners February 25, 2021 19:02

ghost added the Azure.Core label Feb 25, 2021

iscai-msft requested a review from johanste February 25, 2021 19:02

tag p4 # in changelog

a08a86e

xiangyan99 reviewed Feb 25, 2021

View reviewed changes

sdk/core/azure-core/CHANGELOG.md Outdated Show resolved Hide resolved

fix version in changelog

8198d86

lmazuel reviewed Feb 25, 2021

View reviewed changes

sdk/core/azure-core/azure/core/pipeline/transport/_base.py Outdated Show resolved Hide resolved

lmazuel reviewed Feb 25, 2021

View reviewed changes

sdk/core/azure-core/tests/test_pipeline.py Outdated Show resolved Hide resolved

iscai-msft added 3 commits February 25, 2021 18:46

make query doc less alarmist

72d9b0b

add back format_params tests

8324f56

add ? if query=None

4cb950f

iscai-msft commented Feb 26, 2021

View reviewed changes

iscai-msft added 2 commits February 26, 2021 10:51

add params None test for format_parameters

6dde7f0

remove unnecessary pop fallback

c2c5e2d

iscai-msft requested review from lmazuel and xiangyan99 March 1, 2021 22:18

xiangyan99 approved these changes Mar 1, 2021

View reviewed changes

johanste reviewed Mar 2, 2021

View reviewed changes

johanste requested changes Mar 2, 2021

View reviewed changes

make typing of query dict[str, any]

ced6aaf

iscai-msft added 3 commits March 4, 2021 13:53

update typing for kwargs

a4c94c0

iscai-msft closed this Mar 4, 2021

iscai-msft deleted the add_query_kwarg branch May 13, 2021 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] add query kwarg to HttpRequest #16942

[core] add query kwarg to HttpRequest #16942

iscai-msft commented Feb 25, 2021

lmazuel Feb 25, 2021

iscai-msft Feb 26, 2021

xiangyan99 Mar 1, 2021 •

edited

Loading

iscai-msft Mar 1, 2021

johanste Mar 2, 2021

iscai-msft Mar 2, 2021

johanste Mar 2, 2021

xiangyan99 Mar 2, 2021

johanste Mar 2, 2021 •

edited

Loading

johanste Mar 2, 2021

iscai-msft Mar 4, 2021

johanste left a comment

check-enforcer bot commented Mar 4, 2021

iscai-msft commented Mar 4, 2021


		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"])

[core] add query kwarg to HttpRequest #16942

[core] add query kwarg to HttpRequest #16942

Conversation

iscai-msft commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangyan99 Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanste Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanste left a comment

Choose a reason for hiding this comment

check-enforcer bot commented Mar 4, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

iscai-msft commented Mar 4, 2021

xiangyan99 Mar 1, 2021 •

edited

Loading

johanste Mar 2, 2021 •

edited

Loading