Skip to content

fix(servicenow-provider): fix pagination, auth, and a NameError in notify_update - #6724

Open
Prabal864 wants to merge 1 commit into
keephq:mainfrom
Prabal864:fix/servicenow-provider-bugs
Open

fix(servicenow-provider): fix pagination, auth, and a NameError in notify_update#6724
Prabal864 wants to merge 1 commit into
keephq:mainfrom
Prabal864:fix/servicenow-provider-bugs

Conversation

@Prabal864

Copy link
Copy Markdown
Contributor

What

Fixes #6723.

Four independent bugs in keep/providers/servicenow_provider/servicenow_provider.py, all confirmed directly against the current code:

  1. Pagination always returns page 1. _query's if sysparm_offset: params["sysparm_offset"] = 0 hardcoded 0 instead of forwarding the caller's sysparm_offset. _get_incidents calls _query in a while True loop, advancing offset only when a full page comes back — since every page after the first was actually still page 1, and a full page (100 items) kept coming back, this loop never terminated naturally for any instance with >= 100 incidents.
  2. NameError instead of HTTPError in _notify_update's failure branchresp.raise_for_status() where resp is only ever assigned inside the status_code == 200 branch, so the else branch always raised NameError instead of the real HTTP error.
  3. Inverted auth in _notify_update — its auth-tuple construction used if self._access_token where every other auth site in this file (_query, _notify, pull_topology, _get_auth) uses if not self._access_token. This meant OAuth-configured instances got basic auth sent on ticket updates instead of (or alongside) the bearer token.
  4. Missing comma in pull_topology's CMDB fields list"owned_by.name" directly followed by "manufacturer.name", with no comma between them, so Python's adjacent string-literal concatenation silently merged them into one nonexistent field ("owned_by.namemanufacturer.name"), so the ServiceNow API never returned either value.

Testing

Added to tests/test_servicenow_provider.py:

  • TestQueryPagination — asserts a non-zero sysparm_offset reaches the request unchanged, and that 0 stays 0.
  • TestNotifyUpdate — asserts basic auth is used when there's no access token, the bearer header is used (and auth=None) when there is one, and the failure path raises the real HTTPError instead of crashing with NameError.
  • TestPullTopologyFields — asserts owned_by.name and manufacturer.name both appear as separate entries in the requested sysparm_fields.

I wasn't able to run the suite in my sandbox for the same reason as my last PR here — this repo's ContextManager (imported transitively by the provider base classes) unconditionally pulls in keep/api/core/db.py's full dependency chain (opentelemetry, mysql-connector, etc.), and a full pip install -e . fails on Windows regardless (uvloop doesn't support it). I traced each fix by hand against the exact current code (line numbers/behavior confirmed via git show/git grep against upstream/main) and the added tests follow this file's existing MagicMock-based fixture pattern, so they should run cleanly under CI. python -m py_compile passes on both changed files.

Note: I didn't run black/isort on the full file since it isn't currently formatted with either on main and CI doesn't enforce them here — didn't want to bundle an unrelated reformat into this diff. My changed lines are minimal and match the surrounding style.

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. Bug Something isn't working labels Aug 21, 2026
@Prabal864
Prabal864 force-pushed the fix/servicenow-provider-bugs branch from 1abdea9 to 5932b43 Compare August 21, 2026 20:57
…tify_update

Four independent bugs in the ServiceNow provider:

- _query always reset sysparm_offset to 0 instead of forwarding the
  caller's value, so _get_incidents' pagination loop kept re-fetching
  page 1 forever whenever there were >= 1 page of incidents.
- _notify_update's auth selection was inverted (used basic auth only
  when an access token was present, i.e. the opposite of every other
  auth site in this file), so OAuth-configured instances silently sent
  basic auth on ticket updates instead of the bearer token.
- _notify_update's failure branch called resp.raise_for_status() where
  resp was never assigned, raising NameError instead of the intended
  HTTPError.
- pull_topology's CMDB fields list was missing a comma between
  "owned_by.name" and "manufacturer.name", so Python concatenated them
  into a single nonexistent field name and neither value was ever
  returned.

Fixes keephq#6723
@Prabal864
Prabal864 force-pushed the fix/servicenow-provider-bugs branch from 5932b43 to 6a245e5 Compare August 23, 2026 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: ServiceNow pagination loops with page 1 data and incorrect exception handling in notify_update

1 participant