Skip to content

Commit e6a3b35

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 e6a3b35

File tree

10 files changed

+87
-87
lines changed

10 files changed

+87
-87
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/JsonNodeExtensions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ public static bool RenameAll(this JsonObject target, string currentName, string
222222
obj.Rename(currentName, newName);
223223
}
224224

225-
return true;
225+
return objectsWithProperty.Count > 0;
226226
}
227227

228228
/// <summary>

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/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: 41 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="double"/>; with <c>preferInt64</c>, always <see cref="long"/></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,38 +122,38 @@ 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;
120-
121-
// Fall back to double for floating-point
122136
return reader.GetDouble();
123137
}
124138

125139
// 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;
129-
130-
if (reader.TryGetInt64(out long l))
131-
return l;
140+
if (_preferInt64)
141+
{
142+
// Match JSON.NET DataObjectConverter behavior: always return Int64
143+
if (reader.TryGetInt64(out long l))
144+
return l;
145+
}
146+
else
147+
{
148+
// Default STJ behavior: return smallest fitting integer type
149+
if (reader.TryGetInt32(out int i))
150+
return i;
132151

133-
// For very large integers, try decimal first to preserve precision
134-
if (reader.TryGetDecimal(out decimal dec))
135-
return dec;
152+
if (reader.TryGetInt64(out long l))
153+
return l;
154+
}
136155

137-
// Fall back to double only if decimal also fails
156+
// For very large integers that don't fit in long, fall back to double
138157
return reader.GetDouble();
139158
}
140159

@@ -160,7 +179,7 @@ private static object ReadNumber(ref Utf8JsonReader reader)
160179
/// Uses <see cref="StringComparer.OrdinalIgnoreCase"/> for property name matching,
161180
/// consistent with <see cref="DataDictionary"/> behavior.
162181
/// </remarks>
163-
private static Dictionary<string, object?> ReadObject(ref Utf8JsonReader reader, JsonSerializerOptions options)
182+
private Dictionary<string, object?> ReadObject(ref Utf8JsonReader reader, JsonSerializerOptions options)
164183
{
165184
var dictionary = new Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase);
166185

@@ -186,7 +205,7 @@ private static object ReadNumber(ref Utf8JsonReader reader)
186205
/// <summary>
187206
/// Recursively reads a JSON array into a <see cref="List{T}"/> of objects.
188207
/// </summary>
189-
private static List<object?> ReadArray(ref Utf8JsonReader reader, JsonSerializerOptions options)
208+
private List<object?> ReadArray(ref Utf8JsonReader reader, JsonSerializerOptions options)
190209
{
191210
var list = new List<object?>();
192211

@@ -204,7 +223,7 @@ private static object ReadNumber(ref Utf8JsonReader reader)
204223
/// <summary>
205224
/// Reads a single JSON value of any type, dispatching to the appropriate reader method.
206225
/// </summary>
207-
private static object? ReadValue(ref Utf8JsonReader reader, JsonSerializerOptions options)
226+
private object? ReadValue(ref Utf8JsonReader reader, JsonSerializerOptions options)
208227
{
209228
return reader.TokenType switch
210229
{

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,

tests/Exceptionless.Tests/IntegrationTestsBase.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,11 @@ protected async Task<HttpResponseMessage> SendRequestAsync(Action<AppSendBuilder
235235
{
236236
var response = await SendRequestAsync(configure);
237237

238+
// Handle empty response bodies gracefully (e.g., Ok() with no content).
239+
// STJ throws on empty input whereas Newtonsoft returned default(T).
240+
if (response.Content.Headers.ContentLength == 0)
241+
return default;
242+
238243
// All errors are returned as problem details so if we are expecting Problem Details we shouldn't ensure success.
239244
bool ensureSuccess = !typeof(Microsoft.AspNetCore.Mvc.ProblemDetails).IsAssignableFrom(typeof(T));
240245
return await response.DeserializeAsync<T>(ensureSuccess);

tests/Exceptionless.Tests/Serializer/Models/DataDictionaryTests.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ public void Deserialize_DataDictionaryWithMixedTypesAfterRoundTrip_PreservesAllT
391391
// Assert
392392
Assert.NotNull(deserialized);
393393
Assert.Equal("hello", deserialized["string_value"]);
394-
Assert.Equal(42, deserialized["int_value"]); // JSON integers deserialize to int when they fit
394+
Assert.Equal(42, deserialized["int_value"]);
395395
Assert.True(deserialized["bool_value"] as bool?);
396396
}
397397

@@ -584,7 +584,7 @@ public void Deserialize_MixedDataTypesAfterRoundTrip_PreservesAllTypes()
584584
Assert.Equal("user@test.com", userInfo.Identity);
585585

586586
Assert.Equal("1.0.0", deserialized["@version"]);
587-
Assert.Equal(42, deserialized["count"]); // JSON integers deserialize to int when they fit
587+
Assert.Equal(42, deserialized["count"]);
588588
Assert.True(deserialized["enabled"] as bool?);
589589
}
590590

@@ -613,7 +613,7 @@ public void Deserialize_NestedDataDictionaryAfterRoundTrip_PreservesNestedData()
613613
Assert.Equal("user@test.com", result.Identity);
614614
Assert.NotNull(result.Data);
615615
Assert.Equal("custom_value", result.Data["custom_field"]);
616-
Assert.Equal(100, result.Data["score"]); // JSON integers deserialize to int when they fit
616+
Assert.Equal(100, result.Data["score"]);
617617
}
618618

619619
[Fact]

tests/Exceptionless.Tests/Serializer/SerializerTests.cs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ public void CanDeserializeEventWithData()
2626
var ev = _serializer.Deserialize<Event>(json);
2727

2828
// Assert
29-
Assert.NotNull(ev?.Data);
29+
Assert.NotNull(ev);
30+
Assert.NotNull(ev.Data);
3031
Assert.Single(ev.Data);
3132
Assert.Equal("Hello", ev.Message);
3233
Assert.Equal("SomeVal", ev.Data["Blah"]);
@@ -35,21 +36,22 @@ public void CanDeserializeEventWithData()
3536
[Fact]
3637
public void CanRoundTripEventWithUnknownProperties()
3738
{
38-
// Arrange
39+
// Arrange - unknown properties at root go through [JsonExtensionData] → ConvertJsonElement
3940
/* language=json */
40-
const string json = """{"tags":["One","Two"],"reference_id":"12","message":"Hello","data":{"SomeString":"Hi","SomeBool":false,"SomeNum":1}}""";
41+
const string json = """{"tags":["One","Two"],"reference_id":"12","message":"Hello","SomeString":"Hi","SomeBool":false,"SomeNum":1}""";
4142

4243
// Act
4344
var ev = _serializer.Deserialize<Event>(json);
4445
string roundTrippedJson = _serializer.SerializeToString(ev);
4546
var roundTripped = _serializer.Deserialize<Event>(roundTrippedJson);
4647

4748
// Assert
48-
Assert.NotNull(ev?.Data);
49+
Assert.NotNull(ev);
50+
Assert.NotNull(ev.Data);
4951
Assert.Equal(3, ev.Data.Count);
5052
Assert.Equal("Hi", ev.Data["SomeString"]);
5153
Assert.Equal(false, ev.Data["SomeBool"]);
52-
Assert.Equal(1, ev.Data["SomeNum"]);
54+
Assert.Equal(1L, ev.Data["SomeNum"]);
5355
Assert.Equal("Hello", ev.Message);
5456
Assert.NotNull(ev.Tags);
5557
Assert.Equal(2, ev.Tags.Count);

0 commit comments

Comments
 (0)