From c3c828a94f905a1b8e740c8deb36502ca546d0d9 Mon Sep 17 00:00:00 2001 From: Veydop <37590446+Veydop@users.noreply.github.com> Date: Sat, 22 Aug 2026 02:02:18 +0530 Subject: [PATCH] fix(servicenow): forward sysparm_offset in pagination and fix undefined var in _notify_update --- .../servicenow_provider.py | 38 ++--- tests/test_servicenow_provider.py | 149 ++++++++++++++++++ 2 files changed, 169 insertions(+), 18 deletions(-) diff --git a/keep/providers/servicenow_provider/servicenow_provider.py b/keep/providers/servicenow_provider/servicenow_provider.py index 38e5f7cfb1..3c674ac8b1 100644 --- a/keep/providers/servicenow_provider/servicenow_provider.py +++ b/keep/providers/servicenow_provider/servicenow_provider.py @@ -306,16 +306,20 @@ def _query( sysparm_limit # Limit number of records per request ) if sysparm_offset: - params["sysparm_offset"] = 0 # Start from beginning + params["sysparm_offset"] = sysparm_offset # use the recevied offset - response = requests.get( - request_url, - headers=headers, - auth=auth, - params=params, - verify=False, - timeout=10, - ) + try: + response = requests.get( + request_url, + headers=headers, + auth=auth, + params=params, + verify=False, + timeout=10, + ) + except requests.exceptions.RequestException: + self.logger.exception(f"Failed to query {table_name}") + return [] if not response.ok: self.logger.error( @@ -681,7 +685,7 @@ def pull_topology(self) -> tuple[list[TopologyServiceInDto], dict]: "sys_id", "ip_address", "mac_address", - "owned_by.name" + "owned_by.name", "manufacturer.name", # Retrieve the name of the manufacturer "short_description", "environment", @@ -899,16 +903,14 @@ def _notify(self, table_name: str, payload: dict = {}, **kwargs: dict): def _notify_update(self, table_name: str, ticket_id: str, fingerprint: str): url = f"{self.authentication_config.service_now_base_url}/api/now/table/{table_name}/{ticket_id}" headers = {"Content-Type": "application/json", "Accept": "application/json"} - auth = ( - ( + auth = None + if self._access_token: + headers["Authorization"] = f"Bearer {self._access_token}" + else: + auth = ( self.authentication_config.username, self.authentication_config.password, ) - if self._access_token - else None - ) - if self._access_token: - headers["Authorization"] = f"Bearer {self._access_token}" response = requests.get( url, @@ -932,7 +934,7 @@ def _notify_update(self, table_name: str, ticket_id: str, fingerprint: str): return resp else: self.logger.info("Failed to update ticket", extra={"resp": response.text}) - resp.raise_for_status() + response.raise_for_status() if __name__ == "__main__": diff --git a/tests/test_servicenow_provider.py b/tests/test_servicenow_provider.py index 54baeeff40..90be793566 100644 --- a/tests/test_servicenow_provider.py +++ b/tests/test_servicenow_provider.py @@ -4,6 +4,7 @@ from unittest.mock import MagicMock, patch import pytest +import requests from keep.api.models.incident import IncidentDto, IncidentStatus, IncidentSeverity from keep.contextmanager.contextmanager import ContextManager @@ -332,3 +333,151 @@ def test_severity_mapping_coverage(self): """Test that all ServiceNow impact levels are mapped.""" for impact in ["1", "2", "3"]: assert impact in ServicenowProvider.INCIDENT_SEVERITY_MAP + + +class TestQueryPagination: + """Regression tests for the _query sysparm_offset bug.""" + + def test_query_forwards_requested_offset(self, servicenow_provider): + """_query must send the caller's offset, not always 0.""" + response = MagicMock() + response.ok = True + response.json.return_value = {"result": []} + + with patch("requests.get", return_value=response) as mock_get: + servicenow_provider._query( + "incident", sysparm_limit=50, sysparm_offset=150 + ) + + params = mock_get.call_args.kwargs["params"] + assert params["sysparm_offset"] == 150 + assert params["sysparm_limit"] == 50 + + def test_query_returns_empty_list_on_request_exception(self, servicenow_provider): + """Network failures should be caught, not raised.""" + with patch( + "requests.get", + side_effect=requests.exceptions.ConnectionError("connection refused"), + ): + result = servicenow_provider._query("incident") + + assert result == [] + + +class TestGetIncidentsPagination: + """Regression tests ensuring _get_incidents advances through pages.""" + + def _make_raw_incident(self, number): + return { + "number": number, + "sys_id": number, + "state": "1", + "impact": "1", + "sys_created_on": "2025-01-01 00:00:00", + } + + def test_paginates_across_multiple_pages(self, servicenow_provider): + page1 = [self._make_raw_incident(f"INC{i:04d}") for i in range(100)] + page2 = [self._make_raw_incident("INC0100")] + + with patch.object( + servicenow_provider, "_query", side_effect=[page1, page2] + ) as mock_query: + incidents = servicenow_provider._get_incidents() + + assert len(incidents) == 101 + calls = mock_query.call_args_list + assert len(calls) == 2 + assert calls[0].kwargs["sysparm_offset"] == 0 + assert calls[1].kwargs["sysparm_offset"] == 100 + + def test_stops_when_page_smaller_than_limit(self, servicenow_provider): + page1 = [self._make_raw_incident("INC0001")] + + with patch.object( + servicenow_provider, "_query", side_effect=[page1] + ) as mock_query: + incidents = servicenow_provider._get_incidents() + + assert len(incidents) == 1 + assert mock_query.call_count == 1 + + +class TestNotifyUpdate: + """Tests for _notify_update auth handling and error propagation.""" + + def test_basic_auth_used_when_no_access_token(self, servicenow_provider): + get_response = MagicMock() + get_response.status_code = 200 + get_response.text = json.dumps( + {"result": {"sys_id": "abc123", "number": "INC0010001"}} + ) + + with patch("requests.get", return_value=get_response) as mock_get: + result = servicenow_provider._notify_update( + "incident", "abc123", fingerprint="fp1" + ) + + assert result["sys_id"] == "abc123" + assert result["fingerprint"] == "fp1" + assert mock_get.call_args.kwargs["auth"] == ("admin", "admin") + + def test_oauth_does_not_raise_nameerror(self, servicenow_provider): + """Regression test: auth must not reference an undefined variable + when an OAuth access token is in use.""" + servicenow_provider._access_token = "faketoken" + get_response = MagicMock() + get_response.status_code = 200 + get_response.text = json.dumps({"result": {"sys_id": "abc123"}}) + + with patch("requests.get", return_value=get_response) as mock_get: + result = servicenow_provider._notify_update( + "incident", "abc123", fingerprint="fp2" + ) + + assert result["fingerprint"] == "fp2" + assert mock_get.call_args.kwargs["auth"] is None + assert mock_get.call_args.kwargs["headers"]["Authorization"] == "Bearer faketoken" + + def test_failure_raises_http_error(self, servicenow_provider): + """Regression test: failures must raise HTTPError, not NameError.""" + get_response = MagicMock() + get_response.status_code = 500 + get_response.text = "Internal Server Error" + get_response.raise_for_status.side_effect = requests.exceptions.HTTPError( + "500 Server Error" + ) + + with patch("requests.get", return_value=get_response): + with pytest.raises(requests.exceptions.HTTPError): + servicenow_provider._notify_update( + "incident", "abc123", fingerprint="fp3" + ) + + +class TestPullTopologyFields: + """Regression test for the missing comma in the CMDB fields list.""" + + def test_cmdb_fields_are_separate_entries(self, servicenow_provider): + cmdb_response = MagicMock() + cmdb_response.ok = True + cmdb_response.json.return_value = {"result": []} + rel_type_response = MagicMock() + rel_type_response.ok = True + rel_type_response.json.return_value = {"result": []} + rel_response = MagicMock() + rel_response.ok = True + rel_response.json.return_value = {"result": []} + + with patch( + "requests.get", + side_effect=[cmdb_response, rel_type_response, rel_response], + ) as mock_get: + servicenow_provider.pull_topology() + + cmdb_call = mock_get.call_args_list[0] + fields = cmdb_call.kwargs["params"]["sysparm_fields"].split(",") + + assert "owned_by.name" in fields + assert "manufacturer.name" in fields + assert "owned_by.namemanufacturer.name" not in fields