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;