From 4b2482afded70323e483d8632aed34c3df1791d2 Mon Sep 17 00:00:00 2001 From: Sean Story Date: Tue, 5 Sep 2023 16:39:56 -0500 Subject: [PATCH 1/3] Address specific edge case of 400 on list item unique role permissions --- connectors/sources/sharepoint_online.py | 13 +++++++++++++ tests/sources/test_sharepoint_online.py | 17 +++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/connectors/sources/sharepoint_online.py b/connectors/sources/sharepoint_online.py index acc09d241..a81defa05 100644 --- a/connectors/sources/sharepoint_online.py +++ b/connectors/sources/sharepoint_online.py @@ -94,6 +94,13 @@ class NotFound(Exception): pass +class BadRequestError(Exception): + """Internal exception class to handle 400's from the API. + + Similar to the NotFound exception, this allows us to catch edge-case responses that should + be translated as empty resutls, and let us return [].""" + + pass class InternalServerError(Exception): """Exception class to indicate that something went wrong on the server side.""" @@ -432,6 +439,9 @@ async def _handle_client_response_error(self, absolute_url, e): raise NotFound from e # We wanna catch it in the code that uses this and ignore in some cases elif e.status == 500: raise InternalServerError from e + elif e.status == 400: + self._logger.warning(f"Received 400 response from {absolute_url}") + raise BadRequestError from e else: raise @@ -704,6 +714,9 @@ async def site_list_item_has_unique_role_assignments( return response.get("value", False) except NotFound: return False + except BadRequestError: + self._logger.warning(f"Received error response when retrieving `{list_item_id}` from list: `{site_list_name}` in site: `{site_web_url}`") + return False async def site_list_item_role_assignments( self, site_web_url, site_list_name, list_item_id diff --git a/tests/sources/test_sharepoint_online.py b/tests/sources/test_sharepoint_online.py index e46f6618f..023609b87 100644 --- a/tests/sources/test_sharepoint_online.py +++ b/tests/sources/test_sharepoint_online.py @@ -31,6 +31,7 @@ MicrosoftAPISession, MicrosoftSecurityToken, NotFound, + BadRequestError, PermissionsMissing, SharepointOnlineAdvancedRulesValidator, SharepointOnlineClient, @@ -1257,6 +1258,22 @@ async def test_site_list_item_has_unique_role_assignments_not_found( site_list_item_role_assignments_url, list_title, list_item_id ) + + @pytest.mark.asyncio + async def test_site_list_item_has_unique_role_assignments_bad_request( + self, client, patch_fetch + ): + site_list_item_role_assignments_url = f"https://{self.tenant_name}.sharepoint.com/random/totally/made/up/roleassignments" + list_title = "list_title" + list_item_id = 1 + + patch_fetch.side_effect = BadRequestError() + + assert not await client.site_list_item_has_unique_role_assignments( + site_list_item_role_assignments_url, list_title, list_item_id + ) + + @pytest.mark.asyncio async def test_site_list_item_role_assignments(self, client, patch_scroll): site_list_item_role_assignments_url = f"https://{self.tenant_name}.sharepoint.com/random/totally/made/up/roleassignments" From 9fb242633efd268468fc5eae280a48f79777b4ca Mon Sep 17 00:00:00 2001 From: Sean Story Date: Fri, 8 Sep 2023 13:34:00 -0500 Subject: [PATCH 2/3] Don't retry 400 errors --- connectors/sources/sharepoint_online.py | 2 +- tests/sources/test_sharepoint_online.py | 25 +++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/connectors/sources/sharepoint_online.py b/connectors/sources/sharepoint_online.py index a81defa05..5f69079da 100644 --- a/connectors/sources/sharepoint_online.py +++ b/connectors/sources/sharepoint_online.py @@ -296,7 +296,7 @@ async def wrapped(*args, **kwargs): async for item in func(*args, **kwargs): yield item break - except NotFound: + except (NotFound, BadRequestError): raise except Exception: if retry >= retries: diff --git a/tests/sources/test_sharepoint_online.py b/tests/sources/test_sharepoint_online.py index 023609b87..d071f1ef7 100644 --- a/tests/sources/test_sharepoint_online.py +++ b/tests/sources/test_sharepoint_online.py @@ -628,8 +628,6 @@ async def test_call_api_with_404_without_retry_after_header( not_found_error.status = 404 not_found_error.message = "Something went wrong" - mock_responses.get(url, exception=not_found_error) - mock_responses.get(url, exception=not_found_error) mock_responses.get(url, exception=not_found_error) with pytest.raises(NotFound) as e: @@ -638,6 +636,29 @@ async def test_call_api_with_404_without_retry_after_header( assert e is not None + @pytest.mark.asyncio + async def test_call_api_with_400_without_retry_after_header( + self, + microsoft_api_session, + mock_responses, + patch_sleep, + patch_cancellable_sleeps, + ): + url = "http://localhost:1234/download-some-sample-file" + + # First throttle, then do not throttle + bad_request_error = ClientResponseError(None, None) + bad_request_error.status = 400 + bad_request_error.message = "You did something wrong" + + mock_responses.get(url, exception=bad_request_error) + + with pytest.raises(BadRequestError) as e: + async with microsoft_api_session._get(url) as _: + pass + + assert e is not None + @pytest.mark.asyncio async def test_call_api_with_unhandled_status( self, From 4a32f124bd1c981dc62552826ea913e06bdd3a03 Mon Sep 17 00:00:00 2001 From: Sean Story Date: Fri, 8 Sep 2023 14:46:24 -0500 Subject: [PATCH 3/3] Autoformat --- connectors/sources/sharepoint_online.py | 6 +++++- tests/sources/test_sharepoint_online.py | 16 +++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/connectors/sources/sharepoint_online.py b/connectors/sources/sharepoint_online.py index 5f69079da..7c0f4da21 100644 --- a/connectors/sources/sharepoint_online.py +++ b/connectors/sources/sharepoint_online.py @@ -94,6 +94,7 @@ class NotFound(Exception): pass + class BadRequestError(Exception): """Internal exception class to handle 400's from the API. @@ -102,6 +103,7 @@ class BadRequestError(Exception): pass + class InternalServerError(Exception): """Exception class to indicate that something went wrong on the server side.""" @@ -715,7 +717,9 @@ async def site_list_item_has_unique_role_assignments( except NotFound: return False except BadRequestError: - self._logger.warning(f"Received error response when retrieving `{list_item_id}` from list: `{site_list_name}` in site: `{site_web_url}`") + self._logger.warning( + f"Received error response when retrieving `{list_item_id}` from list: `{site_list_name}` in site: `{site_web_url}`" + ) return False async def site_list_item_role_assignments( diff --git a/tests/sources/test_sharepoint_online.py b/tests/sources/test_sharepoint_online.py index d071f1ef7..cd3b7a7dd 100644 --- a/tests/sources/test_sharepoint_online.py +++ b/tests/sources/test_sharepoint_online.py @@ -24,6 +24,7 @@ ACCESS_CONTROL, DEFAULT_RETRY_SECONDS, WILDCARD, + BadRequestError, DriveItemsPage, GraphAPIToken, InternalServerError, @@ -31,7 +32,6 @@ MicrosoftAPISession, MicrosoftSecurityToken, NotFound, - BadRequestError, PermissionsMissing, SharepointOnlineAdvancedRulesValidator, SharepointOnlineClient, @@ -638,11 +638,11 @@ async def test_call_api_with_404_without_retry_after_header( @pytest.mark.asyncio async def test_call_api_with_400_without_retry_after_header( - self, - microsoft_api_session, - mock_responses, - patch_sleep, - patch_cancellable_sleeps, + self, + microsoft_api_session, + mock_responses, + patch_sleep, + patch_cancellable_sleeps, ): url = "http://localhost:1234/download-some-sample-file" @@ -1279,10 +1279,9 @@ async def test_site_list_item_has_unique_role_assignments_not_found( site_list_item_role_assignments_url, list_title, list_item_id ) - @pytest.mark.asyncio async def test_site_list_item_has_unique_role_assignments_bad_request( - self, client, patch_fetch + self, client, patch_fetch ): site_list_item_role_assignments_url = f"https://{self.tenant_name}.sharepoint.com/random/totally/made/up/roleassignments" list_title = "list_title" @@ -1294,7 +1293,6 @@ async def test_site_list_item_has_unique_role_assignments_bad_request( site_list_item_role_assignments_url, list_title, list_item_id ) - @pytest.mark.asyncio async def test_site_list_item_role_assignments(self, client, patch_scroll): site_list_item_role_assignments_url = f"https://{self.tenant_name}.sharepoint.com/random/totally/made/up/roleassignments"