Fix paging when the ordering field is not a plain UTC string - #75
Merged
Conversation
Paging read the ordering field with ToString() and concatenated "Z". That failed with sysparm_display_value=all, where each field is a { display_value, value } object so ToString() yields JSON, and it silently corrupted values that Newtonsoft had already converted to DateTime during deserialisation: those render in the host's culture and timezone, so an en-GB host turned 2026-01-02T05:00:00+05:00 into a window of 2026-02-01 07:00:00. Date tokens are now taken as dates, the raw value is preferred over the display value, remaining strings are parsed with the invariant culture and AssumeUniversal, and an unusable ordering field raises a ServiceNowApiException naming the field and value. Adds PagingFieldParsingTests; 4 of its 5 cases were verified to fail against the previous behaviour. Verified against a live instance: with the fix, display-value modes none/true/all all return the same 5,410 rows. Reported by @jamesmanning in #25.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 22 |
| Duplication | 7 |
AI Reviewer: run a review on demand. To trigger the first review automatically, go to your organization or repository integration settings. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #74, originally reported by @jamesmanning in #25.
The problem
Paging read the ordering field with
ToString()and concatenated"Z"onto it. That breaks in two ways oncesysparm_display_valueis involved.With
sysparm_display_value=allevery field is returned as a{ display_value, value }object, soToString()yields JSON and the parse throws, taking the whole query with it. This is the failure reported in #25.Less obviously, Newtonsoft recognises ISO-8601 text while deserialising and converts it to a
DateTimebefore the client ever sees it, shifting it to the host's local timezone.ToString()then renders it in the host's culture:The paging window ends up a month out, with no error raised. Being host-dependent, this behaves differently on a UTC build agent than on a developer machine, which makes it easy to miss.
Appending
"Z"is separately wrong for any value that already carries an offset, since it produces an unparseable string.The change
ParsePagingFieldValuenow:{ display_value, value }, preferring the rawvalue, which carries the underlying UTC timestampDatetokens as dates, avoiding theToString()round-trip that introduced the host's culture and timezoneInvariantCultureandAssumeUniversal | AdjustToUniversal, instead of concatenating"Z"ServiceNowApiExceptionnaming the field and the offending value when the ordering field cannot be interpreted, rather than a bareFormatExceptionsurfacing from inside aMax()Testing
Adds
PagingFieldParsingTests(5 cases) and extractsStubServiceNowHandlerinto its own file so both paging test classes can share it. The stub now records request URIs, so a test can assert on the paging window actually sent rather than only on the rows returned.These are credential-free and run in CI.
Verified against the previous behaviour: reverting the parse makes 4 of the 5 new cases fail. The fifth,
PlainUtcPagingField_StillPagesAsBefore, passes either way, which is the point: the common case is unchanged.Verified against a live instance: the same multi-page query run with
sysparm_display_valueunset,=trueand=allnow returns the same 5,410 rows in all three modes. Theallrun is the one that previously died.Notes
DateTimeOffset.MinValueon a parse failure. That interacts badly with the termination logic: if every row in a page fails to parse, the window cannot advance, producing either a spurious "paging window has not increased" error or a silently truncated result. TheDateTimeStyles.AssumeUniversalidea from that PR is carried forward.