From 16672cfc72a3acc91647e87bdeaa44ab6c612092 Mon Sep 17 00:00:00 2001 From: Laurent Mazuel Date: Wed, 17 Jun 2020 16:02:20 -0700 Subject: [PATCH 1/6] Let the transport handle bad urls --- .../azure/core/pipeline/transport/_base.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py index d6e14b4f6876..9a47f2511447 100644 --- a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py +++ b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py @@ -112,6 +112,18 @@ def _case_insensitive_dict(*args, **kwargs): def _format_url_section(template, **kwargs): + """String format the template with the kwargs, auto-skip sections of the template that are NOT in the kwargs. + + By default in Python, "format" will raise a KeyError if a template element if not found. Here the section between + the slashes will be removed from the template instead. + + This is used for API like Storage, where when Swagger has template section not defined as parameter. + + :param str template: a string template to fill + :param dict[str,str] kwargs: Template values as string + :rtype: str + :returns: Template + """ components = template.split("/") while components: try: @@ -718,7 +730,7 @@ def format_url(self, url_template, **kwargs): parsed = urlparse(url) if not parsed.scheme or not parsed.netloc: url = url.lstrip("/") - base = self._base_url.format(**kwargs).rstrip("/") + base = _format_url_section(self._base_url, **kwargs).rstrip("/") url = _urljoin(base, url) else: url = self._base_url.format(**kwargs) From 1c37d517cdf3cfcd48f5e1e098656a0a90555c4e Mon Sep 17 00:00:00 2001 From: Laurent Mazuel Date: Wed, 17 Jun 2020 16:06:41 -0700 Subject: [PATCH 2/6] Typo --- sdk/core/azure-core/azure/core/pipeline/transport/_base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py index 9a47f2511447..5730c0b559b3 100644 --- a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py +++ b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py @@ -114,7 +114,7 @@ def _case_insensitive_dict(*args, **kwargs): def _format_url_section(template, **kwargs): """String format the template with the kwargs, auto-skip sections of the template that are NOT in the kwargs. - By default in Python, "format" will raise a KeyError if a template element if not found. Here the section between + By default in Python, "format" will raise a KeyError if a template element is not found. Here the section between the slashes will be removed from the template instead. This is used for API like Storage, where when Swagger has template section not defined as parameter. @@ -122,7 +122,7 @@ def _format_url_section(template, **kwargs): :param str template: a string template to fill :param dict[str,str] kwargs: Template values as string :rtype: str - :returns: Template + :returns: Template completed """ components = template.split("/") while components: From a096c405b8d10a57afdfe2652e866d14bbc31e8e Mon Sep 17 00:00:00 2001 From: Laurent Mazuel Date: Wed, 17 Jun 2020 16:09:41 -0700 Subject: [PATCH 3/6] ChangeLog --- sdk/core/azure-core/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index ca1427a75ab2..3e2a871b735d 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -6,6 +6,7 @@ ### Bug fixes - `AzureKeyCredentialPolicy` will now accept (and ignore) passed in kwargs #11963 +- Better error messages if passed endpoint is incorrect #12106 ## 1.6.0 (2020-06-03) From fa39b65908fe533f1be5f275fa72bcb49016325c Mon Sep 17 00:00:00 2001 From: Laurent Mazuel Date: Wed, 17 Jun 2020 16:15:03 -0700 Subject: [PATCH 4/6] Tests --- sdk/core/azure-core/tests/test_pipeline.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sdk/core/azure-core/tests/test_pipeline.py b/sdk/core/azure-core/tests/test_pipeline.py index c2fa56b0379e..8921302276aa 100644 --- a/sdk/core/azure-core/tests/test_pipeline.py +++ b/sdk/core/azure-core/tests/test_pipeline.py @@ -204,6 +204,12 @@ def test_format_url_no_base_url(self): formatted = client.format_url("https://google.com/subpath/{foo}", foo="bar") assert formatted == "https://google.com/subpath/bar" + def test_format_incorrect_endpoint(self): + # https://github.com/Azure/azure-sdk-for-python/pull/12106 + client = PipelineClientBase('{Endpoint}/text/analytics/v3.0') + formatted = client.format_url("foo/bar") + # We don't care about the value, we care it doesn't fail + assert formatted is not None class TestClientRequest(unittest.TestCase): def test_request_json(self): From dc257240808f3612396805bbecd513436c43489c Mon Sep 17 00:00:00 2001 From: Laurent Mazuel Date: Thu, 18 Jun 2020 13:28:46 -0700 Subject: [PATCH 5/6] Make it better --- .../azure-core/azure/core/pipeline/transport/_base.py | 8 +++++++- sdk/core/azure-core/tests/test_pipeline.py | 6 +++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py index 5730c0b559b3..0f4b7534824c 100644 --- a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py +++ b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py @@ -730,7 +730,13 @@ def format_url(self, url_template, **kwargs): parsed = urlparse(url) if not parsed.scheme or not parsed.netloc: url = url.lstrip("/") - base = _format_url_section(self._base_url, **kwargs).rstrip("/") + try: + base = self._base_url.format(**kwargs).rstrip("/") + except KeyError as key: + raise ValueError( + "The value provided for the url part {} was incorrect, and resulted in an invalid url".format(key.args[0]) + ) + url = _urljoin(base, url) else: url = self._base_url.format(**kwargs) diff --git a/sdk/core/azure-core/tests/test_pipeline.py b/sdk/core/azure-core/tests/test_pipeline.py index 8921302276aa..bbbd37c09f5e 100644 --- a/sdk/core/azure-core/tests/test_pipeline.py +++ b/sdk/core/azure-core/tests/test_pipeline.py @@ -207,9 +207,9 @@ def test_format_url_no_base_url(self): def test_format_incorrect_endpoint(self): # https://github.com/Azure/azure-sdk-for-python/pull/12106 client = PipelineClientBase('{Endpoint}/text/analytics/v3.0') - formatted = client.format_url("foo/bar") - # We don't care about the value, we care it doesn't fail - assert formatted is not None + with pytest.raises(ValueError) as exp: + client.format_url("foo/bar") + assert str(exp.value) == "The value provided for the url part Endpoint was incorrect, and resulted in an invalid url" class TestClientRequest(unittest.TestCase): def test_request_json(self): From 872f2131c219e3351ae1ce838d79dc251274b6a8 Mon Sep 17 00:00:00 2001 From: Laurent Mazuel Date: Thu, 18 Jun 2020 13:35:21 -0700 Subject: [PATCH 6/6] Pylint --- sdk/core/azure-core/azure/core/pipeline/transport/_base.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py index 0f4b7534824c..f7e83aed4460 100644 --- a/sdk/core/azure-core/azure/core/pipeline/transport/_base.py +++ b/sdk/core/azure-core/azure/core/pipeline/transport/_base.py @@ -733,9 +733,8 @@ def format_url(self, url_template, **kwargs): try: base = self._base_url.format(**kwargs).rstrip("/") except KeyError as key: - raise ValueError( - "The value provided for the url part {} was incorrect, and resulted in an invalid url".format(key.args[0]) - ) + err_msg = "The value provided for the url part {} was incorrect, and resulted in an invalid url" + raise ValueError(err_msg.format(key.args[0])) url = _urljoin(base, url) else: