From 386b5b8286e96e9e2c1c87b0abd0b2606c05a477 Mon Sep 17 00:00:00 2001 From: Roland Banks Date: Tue, 4 Aug 2026 11:33:20 +0700 Subject: [PATCH] Fix paging when the ordering field is not a plain UTC string (#74) 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. --- .../PagingFieldParsingTests.cs | 174 ++++++++++++++++++ ServiceNow.Api.Test/PagingTerminationTests.cs | 29 --- ServiceNow.Api.Test/StubServiceNowHandler.cs | 45 +++++ ServiceNow.Api/ServiceNowClient.cs | 55 +++++- 4 files changed, 271 insertions(+), 32 deletions(-) create mode 100644 ServiceNow.Api.Test/PagingFieldParsingTests.cs create mode 100644 ServiceNow.Api.Test/StubServiceNowHandler.cs diff --git a/ServiceNow.Api.Test/PagingFieldParsingTests.cs b/ServiceNow.Api.Test/PagingFieldParsingTests.cs new file mode 100644 index 0000000..b603821 --- /dev/null +++ b/ServiceNow.Api.Test/PagingFieldParsingTests.cs @@ -0,0 +1,174 @@ +using AwesomeAssertions; +using Newtonsoft.Json.Linq; +using ServiceNow.Api.Exceptions; +using System.Globalization; +using Xunit; + +namespace ServiceNow.Api.Test; + +/// +/// Regression tests for how the ordering field is read out of a returned row when paging. +/// +/// These run entirely against a stubbed message handler, so they need no credentials and no network. +/// +/// The bug they were written for (issue #74, reported as #25): paging read the ordering field with +/// ToString() and concatenated "Z" onto it. That breaks whenever sysparm_display_value is set: +/// with "all" every field is returned as a { display_value, value } object, so ToString() yields JSON +/// and the parse throws, taking the whole query with it. +/// +public class PagingFieldParsingTests +{ + private const string TableName = "cmdb_ci"; + private const int PageSize = 1000; + private static readonly DateTime _baseTime = new(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + + /// + /// The reported failure: with sysparm_display_value=all the ordering field is an object, not a scalar. + /// + [Fact] + public async Task ObjectShapedPagingField_PagesInsteadOfThrowing() + { + using var handler = new StubServiceNowHandler(totalCount: 1_500, + [ + MakeObjectShapedPage(0, PageSize), + MakeObjectShapedPage(PageSize, 500), + [] + ]); + + using var client = new ServiceNowClient(handler); + + var result = await client + .GetAllByQueryAsync(TableName, cancellationToken: TestContext.Current.CancellationToken) + .ConfigureAwait(true); + + result.Should().HaveCount(1_500, "an object-shaped ordering field must not break paging"); + } + + /// + /// With both representations present the raw value is the one to page on, since it carries the + /// underlying UTC timestamp rather than a timezone-and-format-dependent rendering of it. + /// + [Fact] + public async Task ObjectShapedPagingField_PagesOnTheRawValueNotTheDisplayValue() + { + // value says 10:00 UTC; display_value says something entirely different. + var fullPage = MakePageSharingOneTimestamp(new JObject + { + ["display_value"] = "31/12/2030 23:59:59", + ["value"] = "2026-01-01 10:00:00" + }); + + using var handler = new StubServiceNowHandler(totalCount: PageSize, [fullPage, []]); + using var client = new ServiceNowClient(handler); + + _ = await client + .GetAllByQueryAsync(TableName, cancellationToken: TestContext.Current.CancellationToken) + .ConfigureAwait(true); + + // The second request carries the paging window, built from whichever representation was used. + handler.RequestUris.Should().HaveCountGreaterThan(1); + handler.RequestUris[1].Should().Contain("2026-01-01 10:00:00", "the raw value is the correct boundary"); + handler.RequestUris[1].Should().NotContain("2030", "the display value must not be used as the boundary"); + } + + /// + /// A value carrying its own offset used to be corrupted by concatenating "Z" onto it. It should now be + /// converted to UTC properly. + /// + [Fact] + public async Task PagingFieldWithAnExplicitOffset_IsConvertedToUtc() + { + // 05:00 at +05:00 is midnight UTC. + var fullPage = MakePageSharingOneTimestamp("2026-01-02T05:00:00+05:00"); + + using var handler = new StubServiceNowHandler(totalCount: PageSize, [fullPage, []]); + using var client = new ServiceNowClient(handler); + + _ = await client + .GetAllByQueryAsync(TableName, cancellationToken: TestContext.Current.CancellationToken) + .ConfigureAwait(true); + + handler.RequestUris.Should().HaveCountGreaterThan(1); + handler.RequestUris[1].Should().Contain("2026-01-02 00:00:00", "the offset should be applied, not ignored"); + } + + /// + /// An ordering field that is not a date at all should say so clearly, naming the field and the value, + /// rather than surfacing a bare FormatException from inside a LINQ Max(). + /// + [Fact] + public async Task UnparseablePagingField_ThrowsNamingTheFieldAndValue() + { + var fullPage = MakePageSharingOneTimestamp("not a date at all"); + + using var handler = new StubServiceNowHandler(totalCount: PageSize, [fullPage, []]); + using var client = new ServiceNowClient(handler); + + var act = async () => await client + .GetAllByQueryAsync(TableName, cancellationToken: TestContext.Current.CancellationToken) + .ConfigureAwait(true); + + (await act.Should().ThrowAsync().ConfigureAwait(true)) + .WithMessage("*sys_created_on*not a date at all*"); + } + + /// + /// A plain UTC string, which is what comes back with no sysparm_display_value, must keep working exactly + /// as before. This is the overwhelmingly common case. + /// + [Fact] + public async Task PlainUtcPagingField_StillPagesAsBefore() + { + using var handler = new StubServiceNowHandler(totalCount: 1_500, + [ + MakePlainPage(0, PageSize), + MakePlainPage(PageSize, 500), + [] + ]); + + using var client = new ServiceNowClient(handler); + + var result = await client + .GetAllByQueryAsync(TableName, cancellationToken: TestContext.Current.CancellationToken) + .ConfigureAwait(true); + + result.Should().HaveCount(1_500); + } + + /// + /// A full page of distinct records that all share one ordering-field value, so that the paging window the + /// client derives from the page is deterministic and can be asserted on. The sys_ids must differ or the + /// client's de-duplication would collapse the page to a single row. + /// + private static List MakePageSharingOneTimestamp(JToken sharedCreatedOn) + => [.. Enumerable.Range(0, PageSize).Select(i => new JObject + { + ["sys_id"] = $"sys{i:D8}", + ["sys_created_on"] = sharedCreatedOn.DeepClone() + })]; + + private static List MakePlainPage(int startIndex, int count) + => [.. Enumerable.Range(0, count).Select(i => new JObject + { + ["sys_id"] = $"sys{startIndex + i:D8}", + ["sys_created_on"] = _baseTime.AddSeconds(startIndex + i).ToString("yyyy-MM-dd HH:mm:ss", CultureInfo.InvariantCulture) + })]; + + /// + /// The shape returned when sysparm_display_value=all is requested. + /// + private static List MakeObjectShapedPage(int startIndex, int count) + => [.. Enumerable.Range(0, count).Select(i => + { + var created = _baseTime.AddSeconds(startIndex + i); + return new JObject + { + ["sys_id"] = $"sys{startIndex + i:D8}", + ["sys_created_on"] = new JObject + { + ["display_value"] = created.ToString("dd/MM/yyyy HH:mm:ss", CultureInfo.InvariantCulture), + ["value"] = created.ToString("yyyy-MM-dd HH:mm:ss", CultureInfo.InvariantCulture) + } + }; + })]; +} diff --git a/ServiceNow.Api.Test/PagingTerminationTests.cs b/ServiceNow.Api.Test/PagingTerminationTests.cs index d3cc9c5..b840c08 100644 --- a/ServiceNow.Api.Test/PagingTerminationTests.cs +++ b/ServiceNow.Api.Test/PagingTerminationTests.cs @@ -2,8 +2,6 @@ using Newtonsoft.Json.Linq; using ServiceNow.Api.Exceptions; using System.Globalization; -using System.Net; -using System.Text; using Xunit; namespace ServiceNow.Api.Test; @@ -201,31 +199,4 @@ private static List MakePage(int startIndex, int count, bool sameTimest return page; } - - /// - /// Serves a fixed sequence of pages, and reports a fixed X-Total-Count. The query is ignored: - /// these tests are about the termination and validation logic, not query construction. - /// - private sealed class StubServiceNowHandler(int totalCount, IReadOnlyList> pages) : HttpMessageHandler - { - private int _requestCount; - - public int RequestCount => _requestCount; - - protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) - { - var index = _requestCount++; - var rows = index < pages.Count ? pages[index] : []; - - var payload = new JObject { ["result"] = new JArray(rows) }; - - var response = new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(payload.ToString(), Encoding.UTF8, "application/json") - }; - response.Headers.Add("X-Total-Count", totalCount.ToString(CultureInfo.InvariantCulture)); - - return Task.FromResult(response); - } - } } diff --git a/ServiceNow.Api.Test/StubServiceNowHandler.cs b/ServiceNow.Api.Test/StubServiceNowHandler.cs new file mode 100644 index 0000000..99476ba --- /dev/null +++ b/ServiceNow.Api.Test/StubServiceNowHandler.cs @@ -0,0 +1,45 @@ +using Newtonsoft.Json.Linq; +using System.Globalization; +using System.Net; +using System.Text; +using System.Web; + +namespace ServiceNow.Api.Test; + +/// +/// Serves a fixed sequence of pages and reports a fixed X-Total-Count, so that paging behaviour can be +/// exercised deterministically without a live ServiceNow instance. The query is ignored when choosing what +/// to return: these tests are about how responses are interpreted, not about query construction. The +/// requested URLs are recorded so that a test can assert on what the client asked for. +/// +internal sealed class StubServiceNowHandler(int totalCount, IReadOnlyList> pages) : HttpMessageHandler +{ + private readonly List _requestUris = []; + + public int RequestCount => _requestUris.Count; + + /// + /// The path and query of every request made, in order. + /// + public IReadOnlyList RequestUris => _requestUris; + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + var index = _requestUris.Count; + + // HttpUtility.UrlDecode is the exact inverse of the HttpUtility.UrlEncode the client uses, so a + // space encoded as '+' comes back as a space. Uri.UnescapeDataString would leave it as '+'. + _requestUris.Add(HttpUtility.UrlDecode(request.RequestUri?.PathAndQuery ?? string.Empty)); + + var rows = index < pages.Count ? pages[index] : []; + var payload = new JObject { ["result"] = new JArray(rows) }; + + var response = new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(payload.ToString(), Encoding.UTF8, "application/json") + }; + response.Headers.Add("X-Total-Count", totalCount.ToString(CultureInfo.InvariantCulture)); + + return Task.FromResult(response); + } +} diff --git a/ServiceNow.Api/ServiceNowClient.cs b/ServiceNow.Api/ServiceNowClient.cs index 8421bd3..572a724 100644 --- a/ServiceNow.Api/ServiceNowClient.cs +++ b/ServiceNow.Api/ServiceNowClient.cs @@ -6,6 +6,7 @@ using ServiceNow.Api.MetaData; using ServiceNow.Api.Tables; using System.Diagnostics; +using System.Globalization; using System.Net.Http.Headers; using System.Text; using System.Text.RegularExpressions; @@ -372,9 +373,7 @@ internal async Task> GetAllByQueryInternalJObjectAsync( } // At this point, we can be sure that we have the paging field in the data - maxDateTimeRetrieved = items.Max(jObject => - // Parse and enforce source as being UTC (Z) - DateTimeOffset.Parse((jObject[orderByField!]?.ToString() ?? string.Empty) + "Z")); + maxDateTimeRetrieved = items.Max(jObject => ParsePagingFieldValue(jObject, orderByField!, tableName)); if (previousMaxDateTimeRetrieved == maxDateTimeRetrieved) { @@ -496,6 +495,56 @@ private async Task> GetPageByQueryInternalAsync( return pageResult; } + /// + /// Reads the ordering field out of a returned row and converts it to a UTC DateTimeOffset, for use as the + /// paging window boundary. + /// + /// + /// Two things make this less straightforward than it looks, both caused by sysparm_display_value. + /// + /// With sysparm_display_value=all every field is returned as an object of the form + /// { "display_value": ..., "value": ... } rather than a scalar, so calling ToString() on it yields JSON. + /// The raw "value" is preferred here, because it carries the underlying UTC timestamp. + /// + /// The value is also parsed with the invariant culture rather than the host's, since a display-formatted + /// date such as 04/08/2026 would otherwise be interpreted differently depending on where the code runs, + /// producing a wrong window rather than an error. AssumeUniversal replaces the previous approach of + /// concatenating "Z" onto the string, which corrupted any value that already carried an offset. + /// + private static DateTimeOffset ParsePagingFieldValue(JObject jObject, string orderByField, string tableName) + { + var token = jObject[orderByField]; + + // sysparm_display_value=all returns { display_value, value }: prefer the raw value. + if (token is JObject valueObject) + { + token = valueObject["value"] ?? valueObject["display_value"]; + } + + // Newtonsoft recognises ISO-8601 text during deserialisation and converts it to a date value before + // we ever see it. Calling ToString() on that would render it in the HOST's culture and timezone, + // which then reads back wrongly: an en-GB host turns 2026-01-02T05:00:00+05:00 into "02/01/2026 + // 07:00:00", which the invariant culture reads as 1 February. Take the value as a date directly. + if (token?.Type == JTokenType.Date) + { + return token.ToObject().ToUniversalTime(); + } + + var text = token?.ToString(); + + return !string.IsNullOrWhiteSpace(text) + && DateTimeOffset.TryParse( + text, + CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal, + out var parsed) + ? parsed + : throw new ServiceNowApiException( + $"Could not interpret the paging field '{orderByField}' on table '{tableName}' as a date and time. " + + $"The value was '{text ?? ""}'. Paging requires a date/time field, so either set the " + + $"{nameof(Options.PagingFieldName)} option (or the customOrderByField parameter) to one, or use a paged query instead."); + } + private static string? BuildFieldListQueryParameter(List? fieldList) => fieldList?.Any() == true ? $"sysparm_fields={HttpUtility.UrlEncode(string.Join(",", fieldList))}" : null;