Skip to content

Commit 4d880ad

Browse files
niemyjskiCopilot
andcommitted
fix: address STJ migration bugs and PR review feedback
- Fix ConvertJsonElement ternary type coercion: (object)l cast prevents implicit long→double widening in switch expression - Make ObjectToInferredTypesConverter configurable with preferInt64 flag for ES serializer to match JSON.NET DataObjectConverter behavior - Fix ElasticSystemTextJsonSerializer: remove ReadStreamToSpan lifetime bug (span backed by disposed MemoryStream), deserialize from stream directly with MemoryStream fast-path - Fix Serialize<T> indentation: pass JsonWriterOptions to Utf8JsonWriter so SerializationFormatting.Indented actually produces indented output - Handle exponent notation (1e5) as floating-point in ReadNumber - Use double consistently (not decimal) for floating-point to match JSON.NET behavior - Fix RenameAll return value: return whether any renames occurred - Add using var to MemoryStream in EventController and EventPostsJob - Handle empty response bodies in SendRequestAsAsync (STJ throws on empty input, Newtonsoft returned default) - Fix SerializerTests: put unknown properties at root level to test JsonExtensionData→ConvertJsonElement path correctly - Revert AGENTS.md to main Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 27cd572 commit 4d880ad

File tree

13 files changed

+155
-146
lines changed

13 files changed

+155
-146
lines changed

AGENTS.md

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -53,26 +53,3 @@ Load from `.github/skills/<name>/SKILL.md` when working in that domain:
5353
- Use `npm ci` (not `npm install`)
5454
- Never commit secrets — use environment variables
5555
- NuGet feeds are in `NuGet.Config` — don't add sources
56-
57-
## Serialization Architecture
58-
59-
The project uses **System.Text.Json (STJ)** exclusively. NEST still brings in Newtonsoft.Json transitively, but all application-level serialization uses STJ:
60-
61-
| Component | Serializer | Notes |
62-
| -------------- | --------------------------------- | -------------------------------------------- |
63-
| Elasticsearch | `ElasticSystemTextJsonSerializer` | Custom `IElasticsearchSerializer` using STJ |
64-
| Event Upgrader | `System.Text.Json.Nodes` | JsonObject/JsonArray for mutable DOM |
65-
| Data Storage | `SystemTextJsonSerializer` | Via Foundatio's STJ support |
66-
| API | STJ (built-in) | ASP.NET Core default with custom options |
67-
68-
**Key files:**
69-
70-
- `ElasticSystemTextJsonSerializer.cs` - Custom `IElasticsearchSerializer` for NEST
71-
- `JsonNodeExtensions.cs` - STJ equivalents of JObject helpers
72-
- `ObjectToInferredTypesConverter.cs` - Handles JObject/JToken from NEST during STJ serialization
73-
- `V*_EventUpgrade.cs` - Event version upgraders using JsonObject
74-
75-
**Security:**
76-
77-
- Safe JSON encoding used everywhere (escapes `<`, `>`, `&`, `'` for XSS protection)
78-
- No `UnsafeRelaxedJsonEscaping` in the codebase

src/Exceptionless.Core/Extensions/DataDictionaryExtensions.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public static class DataDictionaryExtensions
7878
if (result is not null)
7979
return result;
8080
}
81-
catch
81+
catch (Exception ex) when (ex is JsonException or InvalidOperationException or FormatException)
8282
{
8383
// Ignored - fall through to next handler
8484
}
@@ -94,7 +94,7 @@ public static class DataDictionaryExtensions
9494
if (result is not null)
9595
return result;
9696
}
97-
catch
97+
catch (Exception ex) when (ex is JsonException or InvalidOperationException or FormatException)
9898
{
9999
// Ignored - fall through to next handler
100100
}

src/Exceptionless.Core/Extensions/JsonNodeExtensions.cs

Lines changed: 1 addition & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
1-
using System.Text.Encodings.Web;
21
using System.Text.Json;
32
using System.Text.Json.Nodes;
4-
using System.Text.Unicode;
53

64
namespace Exceptionless.Core.Extensions;
75

@@ -11,28 +9,6 @@ namespace Exceptionless.Core.Extensions;
119
/// </summary>
1210
public static class JsonNodeExtensions
1311
{
14-
/// <summary>
15-
/// XSS-safe encoder for JSON output formatting.
16-
/// This encoder ensures proper XSS protection while allowing Unicode characters
17-
/// for internationalization support.
18-
///
19-
/// Security features:
20-
/// - HTML-sensitive characters (&lt;, &gt;, &amp;) are escaped for XSS protection
21-
/// - Single quotes are escaped as \u0027 (per ECMAScript spec)
22-
/// - Control characters are escaped for security
23-
/// </summary>
24-
private static readonly JavaScriptEncoder SafeJsonEncoder = JavaScriptEncoder.Create(new TextEncoderSettings(UnicodeRanges.All));
25-
26-
/// <summary>
27-
/// JSON options with safe XSS encoding for tests.
28-
/// Validates that dangerous characters (&lt;, &gt;, &amp;, ') are properly escaped.
29-
/// Production code should use <see cref="JsonSerializerOptions"/> from DI.
30-
/// </summary>
31-
internal static readonly JsonSerializerOptions SafeSerializerOptions = new()
32-
{
33-
Encoder = SafeJsonEncoder
34-
};
35-
3612
/// <summary>
3713
/// Checks if a JsonNode is null or empty (no values for objects/arrays).
3814
/// </summary>
@@ -222,7 +198,7 @@ public static bool RenameAll(this JsonObject target, string currentName, string
222198
obj.Rename(currentName, newName);
223199
}
224200

225-
return true;
201+
return objectsWithProperty.Count > 0;
226202
}
227203

228204
/// <summary>
@@ -293,14 +269,6 @@ public static bool RenameAll(this JsonObject target, string currentName, string
293269
yield return desc;
294270
}
295271

296-
/// <summary>
297-
/// Converts an object to a JsonNode using System.Text.Json serialization.
298-
/// </summary>
299-
public static JsonNode? ToJsonNode<T>(T value, JsonSerializerOptions options)
300-
{
301-
return JsonSerializer.SerializeToNode(value, options);
302-
}
303-
304272
/// <summary>
305273
/// Checks if a JsonNode has any values (for objects: has properties, for arrays: has items).
306274
/// </summary>
@@ -331,14 +299,6 @@ public static bool HasValues(this JsonNode? node)
331299
return array.Deserialize<List<T>>(options);
332300
}
333301

334-
/// <summary>
335-
/// Creates a JsonValue from a primitive value.
336-
/// </summary>
337-
public static JsonValue? CreateValue<T>(T value)
338-
{
339-
return JsonValue.Create(value);
340-
}
341-
342302
/// <summary>
343303
/// Converts a JsonNode to a pretty-printed JSON string.
344304
/// Uses 2-space indentation. Normalizes dates to match existing data format (Z → +00:00).

src/Exceptionless.Core/Jobs/EventPostsJob.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ private async Task RetryEventsAsync(List<PersistentEvent> eventsToRetry, EventPo
302302
{
303303
try
304304
{
305-
var stream = new MemoryStream(ev.GetBytes(_serializer));
305+
using var stream = new MemoryStream(ev.GetBytes(_serializer));
306306

307307
// Put this single event back into the queue so we can retry it separately.
308308
await _eventPostService.EnqueueAsync(new EventPost(false)

src/Exceptionless.Core/Models/Event.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ void IJsonOnDeserialized.OnDeserialized()
9898
return element.ValueKind switch
9999
{
100100
JsonValueKind.String => element.GetString(),
101-
JsonValueKind.Number => element.TryGetInt64(out long l) ? l : element.GetDouble(),
101+
JsonValueKind.Number => element.TryGetInt64(out long l) ? (object)l : element.GetDouble(),
102102
JsonValueKind.True => true,
103103
JsonValueKind.False => false,
104104
JsonValueKind.Null or JsonValueKind.Undefined => null,

src/Exceptionless.Core/Plugins/EventUpgrader/Default/V2_EventUpgrade.cs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,11 +150,8 @@ private void RenameAndValidateExtraExceptionProperties(string? id, JsonObject er
150150
var extraProperties = JsonNode.Parse(json) as JsonObject;
151151
if (extraProperties is not null)
152152
{
153-
foreach (var property in extraProperties.ToList())
153+
foreach (var property in extraProperties.ToList().Where(p => !p.Value.IsNullOrEmpty()))
154154
{
155-
if (property.Value.IsNullOrEmpty())
156-
continue;
157-
158155
string dataKey = property.Key;
159156
if (extendedData[dataKey] is not null)
160157
dataKey = "_" + dataKey;
@@ -165,7 +162,7 @@ private void RenameAndValidateExtraExceptionProperties(string? id, JsonObject er
165162
}
166163
}
167164
}
168-
catch (Exception) { }
165+
catch (JsonException) { }
169166

170167
if (ext.IsNullOrEmpty())
171168
return;

src/Exceptionless.Core/Serialization/ElasticSystemTextJsonSerializer.cs

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,19 @@ private static JsonSerializerOptions CreateOptions(JsonSerializerOptions? baseOp
5959
options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull;
6060
options.WriteIndented = writeIndented;
6161

62-
// Insert Elasticsearch converters at the beginning for priority
62+
// Replace the default ObjectToInferredTypesConverter with one that returns Int64
63+
// for all integers, matching JSON.NET DataObjectConverter behavior. This ensures
64+
// Event.Data values round-trip through Elasticsearch with consistent types.
65+
var defaultConverter = options.Converters.FirstOrDefault(c => c is ObjectToInferredTypesConverter);
66+
if (defaultConverter is not null)
67+
options.Converters.Remove(defaultConverter);
68+
options.Converters.Insert(0, new ObjectToInferredTypesConverter(preferInt64: true));
69+
70+
// Insert Elasticsearch converters for priority
6371
// Order matters: more specific converters should come first
64-
options.Converters.Insert(0, new DynamicDictionaryConverter());
65-
options.Converters.Insert(1, new Iso8601DateTimeOffsetConverter());
66-
options.Converters.Insert(2, new Iso8601DateTimeConverter());
72+
options.Converters.Insert(1, new DynamicDictionaryConverter());
73+
options.Converters.Insert(2, new Iso8601DateTimeOffsetConverter());
74+
options.Converters.Insert(3, new Iso8601DateTimeConverter());
6775

6876
return options;
6977
}
@@ -79,8 +87,11 @@ private JsonSerializerOptions GetOptions(SerializationFormatting formatting) =>
7987
if (IsEmptyStream(stream))
8088
return null;
8189

82-
var buffer = ReadStreamToSpan(stream);
83-
return JsonSerializer.Deserialize(buffer, type, _optionsCompact.Value);
90+
// Fast path: MemoryStream with accessible buffer avoids buffering
91+
if (stream is MemoryStream ms && ms.TryGetBuffer(out var segment))
92+
return JsonSerializer.Deserialize(segment.AsSpan((int)ms.Position), type, _optionsCompact.Value);
93+
94+
return JsonSerializer.Deserialize(stream, type, _optionsCompact.Value);
8495
}
8596

8697
/// <inheritdoc />
@@ -89,15 +100,22 @@ private JsonSerializerOptions GetOptions(SerializationFormatting formatting) =>
89100
if (IsEmptyStream(stream))
90101
return default;
91102

92-
var buffer = ReadStreamToSpan(stream);
93-
return JsonSerializer.Deserialize<T>(buffer, _optionsCompact.Value);
103+
// Fast path: MemoryStream with accessible buffer avoids buffering
104+
if (stream is MemoryStream ms && ms.TryGetBuffer(out var segment))
105+
return JsonSerializer.Deserialize<T>(segment.AsSpan((int)ms.Position), _optionsCompact.Value);
106+
107+
return JsonSerializer.Deserialize<T>(stream, _optionsCompact.Value);
94108
}
95109

96110
/// <inheritdoc />
97111
public void Serialize<T>(T data, Stream stream, SerializationFormatting formatting = SerializationFormatting.None)
98112
{
99-
using var writer = new Utf8JsonWriter(stream);
100113
var options = GetOptions(formatting);
114+
using var writer = new Utf8JsonWriter(stream, new JsonWriterOptions
115+
{
116+
Indented = formatting == SerializationFormatting.Indented,
117+
Encoder = options.Encoder
118+
});
101119

102120
if (data is null)
103121
{
@@ -153,31 +171,10 @@ public Task SerializeAsync<T>(
153171

154172
#endregion
155173

156-
#region Stream Helpers
157-
158174
private static bool IsEmptyStream(Stream? stream)
159175
{
160176
return stream is null || stream == Stream.Null || (stream.CanSeek && stream.Length == 0);
161177
}
162-
163-
private static ReadOnlySpan<byte> ReadStreamToSpan(Stream stream)
164-
{
165-
// Fast path: if already a MemoryStream with accessible buffer, use it directly
166-
if (stream is MemoryStream ms && ms.TryGetBuffer(out var segment))
167-
{
168-
return segment.AsSpan();
169-
}
170-
171-
// Slow path: copy to new buffer
172-
using var buffer = stream.CanSeek
173-
? new MemoryStream((int)stream.Length)
174-
: new MemoryStream();
175-
176-
stream.CopyTo(buffer);
177-
return buffer.TryGetBuffer(out var seg) ? seg.AsSpan() : buffer.ToArray();
178-
}
179-
180-
#endregion
181178
}
182179

183180
#region Elasticsearch-Specific Converters

src/Exceptionless.Core/Serialization/ObjectToInferredTypesConverter.cs

Lines changed: 47 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ namespace Exceptionless.Core.Serialization;
1818
/// </para>
1919
/// <list type="bullet">
2020
/// <item><description><c>true</c>/<c>false</c> → <see cref="bool"/></description></item>
21-
/// <item><description>Numbers → <see cref="long"/> (if fits) or <see cref="double"/></description></item>
21+
/// <item><description>Numbers → <see cref="int"/> (if fits), <see cref="long"/>, or <see cref="decimal"/>; with <c>preferInt64</c>, always <see cref="long"/> for integers and <see cref="double"/> for floats</description></item>
2222
/// <item><description>Strings with ISO 8601 date format → <see cref="DateTimeOffset"/></description></item>
2323
/// <item><description>Other strings → <see cref="string"/></description></item>
2424
/// <item><description><c>null</c> → <c>null</c></description></item>
@@ -44,6 +44,25 @@ namespace Exceptionless.Core.Serialization;
4444
/// <seealso href="https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to#deserialize-inferred-types-to-object-properties"/>
4545
public sealed class ObjectToInferredTypesConverter : JsonConverter<object?>
4646
{
47+
private readonly bool _preferInt64;
48+
49+
/// <summary>
50+
/// Initializes a new instance with default settings (integers that fit Int32 are returned as <see cref="int"/>).
51+
/// </summary>
52+
public ObjectToInferredTypesConverter() : this(preferInt64: false) { }
53+
54+
/// <summary>
55+
/// Initializes a new instance with configurable integer handling.
56+
/// </summary>
57+
/// <param name="preferInt64">
58+
/// When <c>true</c>, all integers are returned as <see cref="long"/> to match JSON.NET behavior.
59+
/// Used by the Elasticsearch serializer to maintain compatibility with <c>DataObjectConverter</c>.
60+
/// </param>
61+
public ObjectToInferredTypesConverter(bool preferInt64)
62+
{
63+
_preferInt64 = preferInt64;
64+
}
65+
4766
/// <inheritdoc />
4867
public override object? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
4968
{
@@ -103,39 +122,45 @@ public override void Write(Utf8JsonWriter writer, object? value, JsonSerializerO
103122
/// <item><description>Serializing back would lose the decimal representation</description></item>
104123
/// </list>
105124
/// </remarks>
106-
private static object ReadNumber(ref Utf8JsonReader reader)
125+
private object ReadNumber(ref Utf8JsonReader reader)
107126
{
108127
// Check the raw text to preserve decimal vs integer representation
109128
// This is critical for data integrity - 0.0 should stay as double, not become 0L
110129
ReadOnlySpan<byte> rawValue = reader.HasValueSequence
111130
? reader.ValueSequence.ToArray()
112131
: reader.ValueSpan;
113132

114-
// If the raw text contains a decimal point, treat as floating-point
115-
if (rawValue.Contains((byte)'.'))
133+
// If the raw text contains a decimal point or exponent, treat as floating-point
134+
if (rawValue.Contains((byte)'.') || rawValue.Contains((byte)'e') || rawValue.Contains((byte)'E'))
116135
{
117-
// Try decimal for precise values (e.g., financial data) before double
118-
if (reader.TryGetDecimal(out decimal d))
119-
return d;
136+
if (_preferInt64)
137+
return reader.GetDouble();
120138

121-
// Fall back to double for floating-point
122-
return reader.GetDouble();
139+
return reader.GetDecimal();
123140
}
124141

125142
// No decimal point - this is an integer
126-
// Try int32 first for smaller values, then long for larger integers
127-
if (reader.TryGetInt32(out int i))
128-
return i;
143+
if (_preferInt64)
144+
{
145+
// Match JSON.NET DataObjectConverter behavior: always return Int64
146+
if (reader.TryGetInt64(out long l))
147+
return l;
148+
}
149+
else
150+
{
151+
// Default STJ behavior: return smallest fitting integer type
152+
if (reader.TryGetInt32(out int i))
153+
return i;
129154

130-
if (reader.TryGetInt64(out long l))
131-
return l;
155+
if (reader.TryGetInt64(out long l))
156+
return l;
157+
}
132158

133-
// For very large integers, try decimal first to preserve precision
134-
if (reader.TryGetDecimal(out decimal dec))
135-
return dec;
159+
// For very large integers that don't fit in long, fall back to decimal/double
160+
if (_preferInt64)
161+
return reader.GetDouble();
136162

137-
// Fall back to double only if decimal also fails
138-
return reader.GetDouble();
163+
return reader.GetDecimal();
139164
}
140165

141166
/// <summary>
@@ -160,7 +185,7 @@ private static object ReadNumber(ref Utf8JsonReader reader)
160185
/// Uses <see cref="StringComparer.OrdinalIgnoreCase"/> for property name matching,
161186
/// consistent with <see cref="DataDictionary"/> behavior.
162187
/// </remarks>
163-
private static Dictionary<string, object?> ReadObject(ref Utf8JsonReader reader, JsonSerializerOptions options)
188+
private Dictionary<string, object?> ReadObject(ref Utf8JsonReader reader, JsonSerializerOptions options)
164189
{
165190
var dictionary = new Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase);
166191

@@ -186,7 +211,7 @@ private static object ReadNumber(ref Utf8JsonReader reader)
186211
/// <summary>
187212
/// Recursively reads a JSON array into a <see cref="List{T}"/> of objects.
188213
/// </summary>
189-
private static List<object?> ReadArray(ref Utf8JsonReader reader, JsonSerializerOptions options)
214+
private List<object?> ReadArray(ref Utf8JsonReader reader, JsonSerializerOptions options)
190215
{
191216
var list = new List<object?>();
192217

@@ -204,7 +229,7 @@ private static object ReadNumber(ref Utf8JsonReader reader)
204229
/// <summary>
205230
/// Reads a single JSON value of any type, dispatching to the appropriate reader method.
206231
/// </summary>
207-
private static object? ReadValue(ref Utf8JsonReader reader, JsonSerializerOptions options)
232+
private object? ReadValue(ref Utf8JsonReader reader, JsonSerializerOptions options)
208233
{
209234
return reader.TokenType switch
210235
{

src/Exceptionless.Web/Controllers/EventController.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1115,7 +1115,7 @@ private async Task<ActionResult> GetSubmitEventAsync(string? projectId = null, i
11151115
charSet = contentTypeHeader.Charset.ToString();
11161116
}
11171117

1118-
var stream = new MemoryStream(ev.GetBytes(_serializer));
1118+
using var stream = new MemoryStream(ev.GetBytes(_serializer));
11191119
await _eventPostService.EnqueueAsync(new EventPost(_appOptions.EnableArchive)
11201120
{
11211121
ApiVersion = apiVersion,

0 commit comments

Comments
 (0)