From bba60e57a596882f10e0c84c50858e38d4491de5 Mon Sep 17 00:00:00 2001 From: Stephan van Rooij <1292510+svrooij@users.noreply.github.com> Date: Sun, 26 Jan 2025 14:41:45 +0100 Subject: [PATCH 01/10] Ensure 304 status code does not throw, but returns None instead Fixes #363 Add handling for 304 responses without a location header in request adapter. * Modify `packages/http/httpx/kiota_http/httpx_request_adapter.py` to include a check for 304 status code in `_should_return_none` method. * Return True if the status code is 304 and there is no location header. * Add a unit test in `packages/http/httpx/tests/test_httpx_request_adapter.py` to verify that the request adapter returns null and does not throw for a 304 response without a location header. * Create a mock 304 response without a location header. * Ensure the unit test checks that the request adapter returns null for the 304 response. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/microsoft/kiota-python/issues/363?shareId=XXXX-XXXX-XXXX-XXXX). --- .../http/httpx/kiota_http/httpx_request_adapter.py | 2 +- .../http/httpx/tests/test_httpx_request_adapter.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index 11276110..8d5ddb54 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -423,7 +423,7 @@ async def get_root_parse_node( span.end() def _should_return_none(self, response: httpx.Response) -> bool: - return response.status_code == 204 or not bool(response.content) + return response.status_code == 204 or response.status_code == 304 or not bool(response.content) async def throw_failed_responses( self, diff --git a/packages/http/httpx/tests/test_httpx_request_adapter.py b/packages/http/httpx/tests/test_httpx_request_adapter.py index 0df147ec..6984bcce 100644 --- a/packages/http/httpx/tests/test_httpx_request_adapter.py +++ b/packages/http/httpx/tests/test_httpx_request_adapter.py @@ -400,3 +400,16 @@ async def test_retries_on_cae_failure( ), ] request_adapter._authentication_provider.authenticate_request.assert_has_awaits(calls) + + +@pytest.mark.asyncio +async def test_send_primitive_async_304_no_location_header_returns_null( + request_adapter, request_info +): + mock_304_response = httpx.Response(status_code=304, headers={"Content-Type": "application/json"}) + request_adapter.get_http_response_message = AsyncMock(return_value=mock_304_response) + resp = await request_adapter.get_http_response_message(request_info) + assert resp.status_code == 304 + assert "location" not in resp.headers + final_result = await request_adapter.send_primitive_async(request_info, "float", {}) + assert final_result is None From 87a742b6873743deeb18c07dad8ab4e0b3ae1d7a Mon Sep 17 00:00:00 2001 From: Stephan van Rooij <1292510+svrooij@users.noreply.github.com> Date: Tue, 28 Jan 2025 16:05:36 +0000 Subject: [PATCH 02/10] fix(httpx): Return None for certain 3xx responses when location header is empty --- packages/http/httpx/kiota_http/httpx_request_adapter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index f4e8ad44..56f0bd74 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -425,7 +425,7 @@ async def get_root_parse_node( span.end() def _should_return_none(self, response: httpx.Response) -> bool: - return response.status_code == 204 or response.status_code == 304 or not bool(response.content) + return response.status_code == 204 or response.status_code == 304 or not bool(response.content) or (not response.headers.get("Location") and response.status_code in [301, 302]) async def throw_failed_responses( self, From ebee879185d4cb2316f10f1f983f10dea8eff339 Mon Sep 17 00:00:00 2001 From: Stephan van Rooij <1292510+svrooij@users.noreply.github.com> Date: Wed, 29 Jan 2025 09:31:39 +0000 Subject: [PATCH 03/10] fix: Throw error when invalid redirect is present, otherwise return None --- .../httpx/kiota_http/httpx_request_adapter.py | 35 +++++++++++++++++-- .../httpx/tests/test_httpx_request_adapter.py | 27 ++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index 56f0bd74..b6e84e26 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -332,6 +332,7 @@ async def send_primitive_async( await self.throw_failed_responses(response, error_map, parent_span, parent_span) if self._should_return_none(response): return None + if response_type == "bytes": return response.content # type: ignore _deserialized_span = self._start_local_tracing_span("get_root_parse_node", parent_span) @@ -425,7 +426,19 @@ async def get_root_parse_node( span.end() def _should_return_none(self, response: httpx.Response) -> bool: - return response.status_code == 204 or response.status_code == 304 or not bool(response.content) or (not response.headers.get("Location") and response.status_code in [301, 302]) + """Helper function to check if the response should return None. + + Conditions: + - The response status code is 204 or 304 + - the response content is empty. + - The response status code is 301 or 302 and the location header is not present. + + Returns: + bool: True if the response should return None, False otherwise. + """ + return response.status_code == 204 or response.status_code == 304 or not bool( + response.content + ) or (not response.headers.get("location") and response.status_code in [301, 302]) async def throw_failed_responses( self, @@ -434,8 +447,26 @@ async def throw_failed_responses( parent_span: trace.Span, attribute_span: trace.Span, ) -> None: - if response.is_success: + if response.is_success or response.status_code == 304: return + if response.is_redirect: + if response.has_redirect_location: + return + # Raise a more specific error if the server returned a redirect status code without a location header + attribute_span.set_status(trace.StatusCode.ERROR) + _throw_failed_resp_span = self._start_local_tracing_span( + "throw_failed_responses", parent_span + ) + _throw_failed_resp_span.set_attribute("status", response.status_code) + exc = APIError( + f"The server returned a redirect status code {response.status_code} without a location header", + response.status_code, + response.headers, # type: ignore + ) + _throw_failed_resp_span.set_status(trace.StatusCode.ERROR, str(exc)) + attribute_span.record_exception(exc) + _throw_failed_resp_span.end() + raise exc try: attribute_span.set_status(trace.StatusCode.ERROR) diff --git a/packages/http/httpx/tests/test_httpx_request_adapter.py b/packages/http/httpx/tests/test_httpx_request_adapter.py index 0a15a980..8ac8b30b 100644 --- a/packages/http/httpx/tests/test_httpx_request_adapter.py +++ b/packages/http/httpx/tests/test_httpx_request_adapter.py @@ -416,6 +416,33 @@ async def test_send_primitive_async_304_no_location_header_returns_null( assert final_result is None +@pytest.mark.asyncio +async def test_send_primitive_async_301_no_location_header_throws( + request_adapter, request_info +): + mock_301_response = httpx.Response(status_code=301, headers={"Content-Type": "application/json"}) + request_adapter.get_http_response_message = AsyncMock(return_value=mock_301_response) + resp = await request_adapter.get_http_response_message(request_info) + assert resp.status_code == 301 + assert "location" not in resp.headers + with pytest.raises(APIError) as e: + final_result = await request_adapter.send_primitive_async(request_info, "float", {}) + assert e is not None + assert e.value.response_status_code == 301 + + +@pytest.mark.asyncio +async def test_send_primitive_async_302_with_location_header_does_not_throw( + request_adapter, request_info +): + mock_302_response = httpx.Response(status_code=302, headers={"Content-Type": "application/json", "location": "https://example.com"}) + request_adapter.get_http_response_message = AsyncMock(return_value=mock_302_response) + resp = await request_adapter.get_http_response_message(request_info) + assert resp.status_code == 302 + assert "location" in resp.headers + final_result = await request_adapter.send_primitive_async(request_info, "float", {}) + + def test_httpx_request_adapter_uses_http_client_base_url(auth_provider): http_client = httpx.AsyncClient(base_url=BASE_URL) request_adapter = HttpxRequestAdapter(auth_provider, http_client=http_client) From e8ac2dcaa92f13bdd37bd38f055601ae9d6c625a Mon Sep 17 00:00:00 2001 From: Stephan van Rooij <1292510+svrooij@users.noreply.github.com> Date: Wed, 29 Jan 2025 09:42:43 +0000 Subject: [PATCH 04/10] chore: Sonarqube recommendation --- packages/http/httpx/tests/test_httpx_request_adapter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/http/httpx/tests/test_httpx_request_adapter.py b/packages/http/httpx/tests/test_httpx_request_adapter.py index 8ac8b30b..452d4727 100644 --- a/packages/http/httpx/tests/test_httpx_request_adapter.py +++ b/packages/http/httpx/tests/test_httpx_request_adapter.py @@ -426,7 +426,7 @@ async def test_send_primitive_async_301_no_location_header_throws( assert resp.status_code == 301 assert "location" not in resp.headers with pytest.raises(APIError) as e: - final_result = await request_adapter.send_primitive_async(request_info, "float", {}) + await request_adapter.send_primitive_async(request_info, "float", {}) assert e is not None assert e.value.response_status_code == 301 @@ -440,7 +440,7 @@ async def test_send_primitive_async_302_with_location_header_does_not_throw( resp = await request_adapter.get_http_response_message(request_info) assert resp.status_code == 302 assert "location" in resp.headers - final_result = await request_adapter.send_primitive_async(request_info, "float", {}) + await request_adapter.send_primitive_async(request_info, "float", {}) def test_httpx_request_adapter_uses_http_client_base_url(auth_provider): From 0e77afb83e25f1afabb3b5cba0b78c2a3c83c94f Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 29 Jan 2025 07:27:49 -0500 Subject: [PATCH 05/10] chore: linting Signed-off-by: Vincent Biret --- .../httpx/kiota_http/httpx_request_adapter.py | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index b6e84e26..fd6d1fee 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -440,26 +440,20 @@ def _should_return_none(self, response: httpx.Response) -> bool: response.content ) or (not response.headers.get("location") and response.status_code in [301, 302]) - async def throw_failed_responses( - self, - response: httpx.Response, - error_map: Optional[dict[str, type[ParsableFactory]]], - parent_span: trace.Span, - attribute_span: trace.Span, - ) -> None: - if response.is_success or response.status_code == 304: - return + def _throw_empty_redirects(self, response: httpx.Response, parent_span: trace.Span, attribute_span: trace.Span) -> None: if response.is_redirect: if response.has_redirect_location: return - # Raise a more specific error if the server returned a redirect status code without a location header + # Raise a more specific error if the server returned a redirect status code + # without a location header attribute_span.set_status(trace.StatusCode.ERROR) _throw_failed_resp_span = self._start_local_tracing_span( "throw_failed_responses", parent_span ) _throw_failed_resp_span.set_attribute("status", response.status_code) exc = APIError( - f"The server returned a redirect status code {response.status_code} without a location header", + f"The server returned a redirect status code {response.status_code}" + " without a location header", response.status_code, response.headers, # type: ignore ) @@ -467,6 +461,17 @@ async def throw_failed_responses( attribute_span.record_exception(exc) _throw_failed_resp_span.end() raise exc + + async def throw_failed_responses( + self, + response: httpx.Response, + error_map: Optional[dict[str, type[ParsableFactory]]], + parent_span: trace.Span, + attribute_span: trace.Span, + ) -> None: + if response.is_success or response.status_code == 304: + return + self._throw_empty_redirects(response, parent_span, attribute_span) try: attribute_span.set_status(trace.StatusCode.ERROR) From 971de7111b05d15f4e06f7054fffaa6e90d8d56b Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 29 Jan 2025 07:30:39 -0500 Subject: [PATCH 06/10] fix: skip body if location header is present Signed-off-by: Vincent Biret --- packages/http/httpx/kiota_http/httpx_request_adapter.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index fd6d1fee..10f04e86 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -440,10 +440,10 @@ def _should_return_none(self, response: httpx.Response) -> bool: response.content ) or (not response.headers.get("location") and response.status_code in [301, 302]) - def _throw_empty_redirects(self, response: httpx.Response, parent_span: trace.Span, attribute_span: trace.Span) -> None: + def _is_redirect_missing_location(self, response: httpx.Response, parent_span: trace.Span, attribute_span: trace.Span) -> bool: if response.is_redirect: if response.has_redirect_location: - return + return False # Raise a more specific error if the server returned a redirect status code # without a location header attribute_span.set_status(trace.StatusCode.ERROR) @@ -471,7 +471,8 @@ async def throw_failed_responses( ) -> None: if response.is_success or response.status_code == 304: return - self._throw_empty_redirects(response, parent_span, attribute_span) + if self._is_redirect_missing_location(response, parent_span, attribute_span) == False: + return try: attribute_span.set_status(trace.StatusCode.ERROR) From d23d377f7b40589a4bd9f1d61cfc4d5586425942 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 29 Jan 2025 07:32:38 -0500 Subject: [PATCH 07/10] chore: formatting Signed-off-by: Vincent Biret --- .../httpx/kiota_http/httpx_request_adapter.py | 6 +++-- .../httpx/tests/test_httpx_request_adapter.py | 22 ++++++++++++------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index 10f04e86..625704fa 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -440,7 +440,9 @@ def _should_return_none(self, response: httpx.Response) -> bool: response.content ) or (not response.headers.get("location") and response.status_code in [301, 302]) - def _is_redirect_missing_location(self, response: httpx.Response, parent_span: trace.Span, attribute_span: trace.Span) -> bool: + def _is_redirect_missing_location( + self, response: httpx.Response, parent_span: trace.Span, attribute_span: trace.Span + ) -> bool: if response.is_redirect: if response.has_redirect_location: return False @@ -453,7 +455,7 @@ def _is_redirect_missing_location(self, response: httpx.Response, parent_span: t _throw_failed_resp_span.set_attribute("status", response.status_code) exc = APIError( f"The server returned a redirect status code {response.status_code}" - " without a location header", + " without a location header", response.status_code, response.headers, # type: ignore ) diff --git a/packages/http/httpx/tests/test_httpx_request_adapter.py b/packages/http/httpx/tests/test_httpx_request_adapter.py index 452d4727..67c622e8 100644 --- a/packages/http/httpx/tests/test_httpx_request_adapter.py +++ b/packages/http/httpx/tests/test_httpx_request_adapter.py @@ -402,12 +402,13 @@ async def test_retries_on_cae_failure( request_adapter._authentication_provider.authenticate_request.assert_has_awaits(calls) - @pytest.mark.asyncio async def test_send_primitive_async_304_no_location_header_returns_null( request_adapter, request_info ): - mock_304_response = httpx.Response(status_code=304, headers={"Content-Type": "application/json"}) + mock_304_response = httpx.Response( + status_code=304, headers={"Content-Type": "application/json"} + ) request_adapter.get_http_response_message = AsyncMock(return_value=mock_304_response) resp = await request_adapter.get_http_response_message(request_info) assert resp.status_code == 304 @@ -417,10 +418,10 @@ async def test_send_primitive_async_304_no_location_header_returns_null( @pytest.mark.asyncio -async def test_send_primitive_async_301_no_location_header_throws( - request_adapter, request_info -): - mock_301_response = httpx.Response(status_code=301, headers={"Content-Type": "application/json"}) +async def test_send_primitive_async_301_no_location_header_throws(request_adapter, request_info): + mock_301_response = httpx.Response( + status_code=301, headers={"Content-Type": "application/json"} + ) request_adapter.get_http_response_message = AsyncMock(return_value=mock_301_response) resp = await request_adapter.get_http_response_message(request_info) assert resp.status_code == 301 @@ -435,7 +436,13 @@ async def test_send_primitive_async_301_no_location_header_throws( async def test_send_primitive_async_302_with_location_header_does_not_throw( request_adapter, request_info ): - mock_302_response = httpx.Response(status_code=302, headers={"Content-Type": "application/json", "location": "https://example.com"}) + mock_302_response = httpx.Response( + status_code=302, + headers={ + "Content-Type": "application/json", + "location": "https://example.com" + } + ) request_adapter.get_http_response_message = AsyncMock(return_value=mock_302_response) resp = await request_adapter.get_http_response_message(request_info) assert resp.status_code == 302 @@ -447,4 +454,3 @@ def test_httpx_request_adapter_uses_http_client_base_url(auth_provider): http_client = httpx.AsyncClient(base_url=BASE_URL) request_adapter = HttpxRequestAdapter(auth_provider, http_client=http_client) assert request_adapter.base_url == BASE_URL - From b891f0704d48b5600c9db2a56a175f3879ca20c1 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 29 Jan 2025 07:35:44 -0500 Subject: [PATCH 08/10] chore: additional formatting Signed-off-by: Vincent Biret --- packages/http/httpx/kiota_http/httpx_request_adapter.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index 625704fa..1c0917b4 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -463,6 +463,7 @@ def _is_redirect_missing_location( attribute_span.record_exception(exc) _throw_failed_resp_span.end() raise exc + return True async def throw_failed_responses( self, @@ -473,7 +474,7 @@ async def throw_failed_responses( ) -> None: if response.is_success or response.status_code == 304: return - if self._is_redirect_missing_location(response, parent_span, attribute_span) == False: + if self._is_redirect_missing_location(response, parent_span, attribute_span) is False: return try: attribute_span.set_status(trace.StatusCode.ERROR) From 93267b46cb21728c70f66b416c5fe47041389982 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 29 Jan 2025 07:45:43 -0500 Subject: [PATCH 09/10] chore: linting Signed-off-by: Vincent Biret --- .../httpx/kiota_http/httpx_request_adapter.py | 65 ++++++++++++------- 1 file changed, 42 insertions(+), 23 deletions(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index 1c0917b4..cca29f3e 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -465,6 +465,41 @@ def _is_redirect_missing_location( raise exc return True + async def _get_error_from_response( + self, + response: httpx.Response, + error_map: Optional[dict[str, type[ParsableFactory]]], + response_status_code_str: str, + response_status_code: int, + attribute_span: trace.Span, + _throw_failed_resp_span: trace.Span, + ) -> object: + error_class = None + if response_status_code_str in error_map: # Error Code 400 - <= 599 + error_class = error_map[response_status_code_str] + elif 400 <= response_status_code < 500 and "4XX" in error_map: # Error code 4XX + error_class = error_map["4XX"] + elif 500 <= response_status_code < 600 and "5XX" in error_map: # Error code 5XX + error_class = error_map["5XX"] + elif "XXX" in error_map: # Blanket case + error_class = error_map["XXX"] + + root_node = await self.get_root_parse_node( + response, _throw_failed_resp_span, _throw_failed_resp_span + ) + attribute_span.set_attribute(ERROR_BODY_FOUND_KEY, bool(root_node)) + + _get_obj_ctx = trace.set_span_in_context(_throw_failed_resp_span) + _get_obj_span = tracer.start_span("get_object_value", context=_get_obj_ctx) + + if not root_node: + return None + error = None + if error_class: + error = root_node.get_object_value(error_class) + _get_obj_span.end() + return error + async def throw_failed_responses( self, response: httpx.Response, @@ -516,29 +551,14 @@ async def throw_failed_responses( raise exc _throw_failed_resp_span.set_attribute("status_message", "received_error_response") - error_class = None - if response_status_code_str in error_map: # Error Code 400 - <= 599 - error_class = error_map[response_status_code_str] - elif 400 <= response_status_code < 500 and "4XX" in error_map: # Error code 4XX - error_class = error_map["4XX"] - elif 500 <= response_status_code < 600 and "5XX" in error_map: # Error code 5XX - error_class = error_map["5XX"] - elif "XXX" in error_map: # Blanket case - error_class = error_map["XXX"] - - root_node = await self.get_root_parse_node( - response, _throw_failed_resp_span, _throw_failed_resp_span + error = await self._get_error_from_response( + response, + error_map, + response_status_code_str, + response_status_code, + attribute_span, + _throw_failed_resp_span, ) - attribute_span.set_attribute(ERROR_BODY_FOUND_KEY, bool(root_node)) - - _get_obj_ctx = trace.set_span_in_context(_throw_failed_resp_span) - _get_obj_span = tracer.start_span("get_object_value", context=_get_obj_ctx) - - if not root_node: - return None - error = None - if error_class: - error = root_node.get_object_value(error_class) if isinstance(error, APIError): error.response_headers = response_headers # type: ignore error.response_status_code = response_status_code @@ -552,7 +572,6 @@ async def throw_failed_responses( response_status_code, response_headers, # type: ignore ) - _get_obj_span.end() raise exc finally: _throw_failed_resp_span.end() From 0313f9e3aeb435a3fe0ef858b2dad11bfd951be2 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Wed, 29 Jan 2025 07:49:55 -0500 Subject: [PATCH 10/10] fix: error map type Signed-off-by: Vincent Biret --- packages/http/httpx/kiota_http/httpx_request_adapter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index cca29f3e..161fd77b 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -468,7 +468,7 @@ def _is_redirect_missing_location( async def _get_error_from_response( self, response: httpx.Response, - error_map: Optional[dict[str, type[ParsableFactory]]], + error_map: dict[str, type[ParsableFactory]], response_status_code_str: str, response_status_code: int, attribute_span: trace.Span,