Merge pull request #75 from panoramicdata/fix/issue-74-paging-field-parsing

Fix paging when the ordering field is not a plain UTC string

Author: rolandbanks

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+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);
+
+	/// <summary>
+	/// The reported failure: with sysparm_display_value=all the ordering field is an object, not a scalar.
+	/// </summary>
+	[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");
+	}
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	[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");
+	}
+
+	/// <summary>
+	/// A value carrying its own offset used to be corrupted by concatenating "Z" onto it. It should now be
+	/// converted to UTC properly.
+	/// </summary>
+	[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");
+	}
+
+	/// <summary>
+	/// 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().
+	/// </summary>
+	[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<ServiceNowApiException>().ConfigureAwait(true))
+			.WithMessage("*sys_created_on*not a date at all*");
+	}
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	[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);
+	}
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	private static List<JObject> MakePageSharingOneTimestamp(JToken sharedCreatedOn)
+		=> [.. Enumerable.Range(0, PageSize).Select(i => new JObject
+		{
+			["sys_id"] = $"sys{i:D8}",
+			["sys_created_on"] = sharedCreatedOn.DeepClone()
+		})];
+
+	private static List<JObject> 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)
+		})];
+
+	/// <summary>
+	/// The shape returned when sysparm_display_value=all is requested.
+	/// </summary>
+	private static List<JObject> 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)
+				}
+			};
+		})];
+}

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<JObject> MakePage(int startIndex, int count, bool sameTimest
 
 		return page;
 	}
-
-	/// <summary>
-	/// 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.
-	/// </summary>
-	private sealed class StubServiceNowHandler(int totalCount, IReadOnlyList<List<JObject>> pages) : HttpMessageHandler
-	{
-		private int _requestCount;
-
-		public int RequestCount => _requestCount;
-
-		protected override Task<HttpResponseMessage> 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);
-		}
-	}
 }

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+internal sealed class StubServiceNowHandler(int totalCount, IReadOnlyList<List<JObject>> pages) : HttpMessageHandler
+{
+	private readonly List<string> _requestUris = [];
+
+	public int RequestCount => _requestUris.Count;
+
+	/// <summary>
+	/// The path and query of every request made, in order.
+	/// </summary>
+	public IReadOnlyList<string> RequestUris => _requestUris;
+
+	protected override Task<HttpResponseMessage> 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);
+	}
+}

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<List<JObject>> 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<Page<T>> GetPageByQueryInternalAsync<T>(
 		return pageResult;
 	}
 
+	/// <summary>
+	/// Reads the ordering field out of a returned row and converts it to a UTC DateTimeOffset, for use as the
+	/// paging window boundary.
+	/// </summary>
+	/// <remarks>
+	/// 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.
+	/// </remarks>
+	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<DateTimeOffset>().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 ?? "<null>"}'. 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<string>? fieldList)
 		=> fieldList?.Any() == true ? $"sysparm_fields={HttpUtility.UrlEncode(string.Join(",", fieldList))}" : null;