From 34525c11e8a3816ffe2941d18862a81205d7d11b Mon Sep 17 00:00:00 2001
From: James Manning <james.manning@gmail.com>
Date: Thu, 8 Aug 2024 23:13:12 -0400
Subject: [PATCH] add support for using sysparm_display_value=all

Currently the use of sysparm_display_value=all breaks the paging logic which assumes that the property value found via the 'orderByField' will be something we can append a "Z" to then DateTimeOffset.Parse.

This fails when you're using sysparm_display_value=all since the value of the orderByField will be a JObject with properties of value and display_value so you end up with an exception during the query similar to this:

String '{
  "display_value": "2024-03-04 21:15:20",
  "value": "2024-03-04 21:15:20"
}Z' was not recognized as a valid DateTime.

This changes the parsing to use DateTimeStyles.AssumeUniversal instead of appending a "Z" and to fall back to DateTimeOffset.MinValue for failure cases instead of throwing an exception.  Since it throws an exception, it makes it difficult for query callers that require using sysparm_display_value=all
---
 ServiceNow.Api.Test/QueryTests.cs  | 16 ++++++++
 ServiceNow.Api/ServiceNowClient.cs | 61 ++++++++++++++++++++++++++----
 2 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/ServiceNow.Api.Test/QueryTests.cs b/ServiceNow.Api.Test/QueryTests.cs
index 9c26315..a9f32bd 100644
--- a/ServiceNow.Api.Test/QueryTests.cs
+++ b/ServiceNow.Api.Test/QueryTests.cs
@@ -48,6 +48,22 @@ public async Task InternalPagingTestAsync()
 		Assert.Empty(dupes);
 	}
 
+	[Theory]
+	[InlineData(null, "core_company")]
+	[InlineData("sysparm_display_value=all", "core_company")]
+	public async Task DisplayValueAllTestAsync(string? extraQueryString, string tableName)
+	{
+		// This test fails if the sysparm_display_value=all is specified since the
+		// property value is an object and not something that can parse as a DateTimeOffset
+		const string query = "";
+		const string customOrderByField = "sys_created_on";
+		var fieldList = new List<string> { };
+
+		var result = await Client.GetAllByQueryAsync(tableName, query, fieldList, extraQueryString, customOrderByField).ConfigureAwait(false);
+		Assert.NotNull(result);
+		Assert.NotEmpty(result);
+	}
+
 	[Fact]
 	public async Task PagingTestAsync()
 	{
diff --git a/ServiceNow.Api/ServiceNowClient.cs b/ServiceNow.Api/ServiceNowClient.cs
index 8ad8b32..2ff99ee 100644
--- a/ServiceNow.Api/ServiceNowClient.cs
+++ b/ServiceNow.Api/ServiceNowClient.cs
@@ -9,6 +9,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Globalization;
 using System.IO;
 using System.Linq;
 using System.Net.Http;
@@ -243,12 +244,12 @@ internal async Task<List<JObject>> GetAllByQueryInternalJObjectAsync(
 		string? customOrderByField,
 		CancellationToken cancellationToken)
 	{
-		var orderByField = string.IsNullOrWhiteSpace(customOrderByField)
+		var orderByField = (string.IsNullOrWhiteSpace(customOrderByField)
 			? _options.PagingFieldName
-			: customOrderByField;
-
+			: customOrderByField)
+			?? throw new ServiceNowApiException("value for PagingFieldName option is required");
 		string orderByCommand;
-		if (orderByField?.StartsWith("-", StringComparison.Ordinal) ?? false)
+		if (orderByField.StartsWith("-", StringComparison.Ordinal))
 		{
 			orderByCommand = "ORDERBYDESC";
 			orderByField = orderByField.Substring(1);
@@ -346,9 +347,7 @@ internal async Task<List<JObject>> GetAllByQueryInternalJObjectAsync(
 				}
 
 				// At this point, we can be sure that we have the paging field in the data
-				maxDateTimeRetrieved = response.Items.Max(jObject =>
-					// Parse and enforce source as being UTC (Z)
-					DateTimeOffset.Parse((jObject[orderByField]?.ToString() ?? string.Empty) + "Z"));
+				maxDateTimeRetrieved = response.Items.Max(jObject => ParseDateTimeOffsetField(jObject, orderByField));
 
 				if (previousMaxDateTimeRetrieved == maxDateTimeRetrieved)
 				{
@@ -414,6 +413,54 @@ internal async Task<List<JObject>> GetAllByQueryInternalJObjectAsync(
 		return finalResult.Items;
 	}
 
+	private DateTimeOffset ParseDateTimeOffsetField(JObject jObject, string orderByField)
+	{
+		if (jObject.TryGetValue(orderByField, out var jtoken) == false)
+		{
+			// field was not found
+			return DateTimeOffset.MinValue;
+		}
+
+		var jtokenToParse = GetJTokenToParse(jtoken);
+		var stringValue = jtokenToParse.ToObject<string>();
+		var parsed = DateTimeOffset.TryParse(stringValue, null, DateTimeStyles.AssumeUniversal, out DateTimeOffset result);
+		return parsed ? result : DateTimeOffset.MinValue;
+	}
+
+	private JToken GetJTokenToParse(JToken jtoken)
+	{
+		if (jtoken.Type == JTokenType.Object)
+		{
+			var jobject = jtoken.ToObject<JObject>();
+			return GetJTokenForObject(jobject);
+		}
+
+		// anything not an object we'll just parse directly
+		return jtoken;
+	}
+
+	private static JToken GetJTokenForObject(JObject? jobject)
+	{
+		if (jobject == null)
+		{
+			return string.Empty;
+		}
+
+		// needed to handle the case of passing extraQueryString of sysparm_display_value=all where the
+		// response values are objects with keys of "value" and "display_value"
+		if (jobject.TryGetValue("value", out var propValue))
+		{
+			// this is the expected case, a value property should be present
+			return propValue;
+		}
+
+		// use display_value property as a fallback, then whatever property we can find
+		return jobject.TryGetValue("display_value", out propValue)
+			? propValue
+			: jobject.First ?? string.Empty;
+	}
+
+
 	public Task<Page<JObject>> GetPageByQueryAsync(
 		int skip,
 		int take,