Repository navigation
fix: serialize non-finite event properties as null - #348
Conversation
There was a problem hiding this comment.
PostHog Review · Standard · reviewed eab4631: 1 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
posthog-dotnet Compliance ReportDate: 2026-10-22T14:45:46.941697+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
There was a problem hiding this comment.
The two converters carry the fix. NumberHandlingJsonTypeInfoResolver adds a reflection-based adapter just to preserve [JsonNumberHandling] on float members, which is an edge case for Dictionary<string, object> event properties. It has three open bot findings on 1dc25dc (extension data, [JsonIgnore], Populate), and it sits on the shared JsonSerializerHelper.Options, so it also runs when we deserialize every API response, including feature flags.
Could we drop the resolver and keep just the two converters? Event properties are Dictionary<string, object>, so [JsonNumberHandling] on float members of user models is an edge case we can document. If we do want to keep it, we should scope it to a capture-only options instance so flag/decide deserialization isn't affected. The three open bot findings on 1dc25dc suggest the adapter will keep growing.
|
@turnipdabeets Agreed—removed NumberHandlingJsonTypeInfoResolver and its forwarding converter in f9aae49. Only the two non-finite numeric converters remain, so no custom metadata adapter runs on flag/decide responses. The author explicitly approved dropping model/member JsonNumberHandling attribute compatibility; the package README and PR description now document that limitation and recommend strings or explicit custom converters for formatting. Option-level number handling remains supported. All three bot regressions were reproduced test-first and now pass; their threads are resolved. Locked restore, full Release build, formatting, 52 focused tests, and 1,345 net8.0 tests pass (2 existing skips). Legacy targets compile; the netcoreapp3.1 runtime is unavailable locally. |
💡 Motivation and Context
Numeric NaN and positive or negative infinity in event properties can prevent the selected event batch from serializing, including healthy neighboring events.
Write these float and double values as JSON null with two internal converters, including nested objects and arrays. Preserve finite numeric values, caller input, literal strings, option-level number handling, and explicit custom converters. Unsupported values still raise errors.
Following @turnipdabeets' review, remove the reflection-based
NumberHandlingJsonTypeInfoResolverinstead of growing the metadata adapter. It no longer runs on shared serialization or API-response deserialization. This also fixes the three reproduced adapter regressions: malformed extension data, metadata errors on ignored members, and rejectedPopulatecollections.Documented compatibility limitation: the float/double converters do not apply
[JsonNumberHandling]attributes on user models or their members, including collection elements. Finite values that previously used attribute-controlled string formatting now serialize as JSON numbers. Supply strings or an explicit[JsonConverter]for custom formatting. This tradeoff was explicitly approved by the author after the review; this PR description and tests document it. No public API additions.Select patch releases for PostHog, PostHog.AspNetCore, and PostHog.AI so each package can deliver the fix. No manual version or dependency-coupling changes.
The first CI Test failure was caused by a compression-fallback test temporarily replacing a global compressor delegate while a gzip Capture test ran. Isolate that mutating test class in a nonparallel xUnit collection. Keep assembly parallelism, compression checks, and delivery assertions. Read copied request bytes rather than disposing the fake transport's content stream. No production batching change was needed.
Related Python work: PostHog/posthog-python#1081. This is context, not a claim that the Python PR has merged. The shared capture/HTTP/batching specs do not explicitly define non-finite handling or .NET number-handling attributes; no divergence from an explicit contract was identified for this change.
💚 How did you test it?
Review follow-up on
f9aae49807365b4d5e0f00cffb91881e150d3df2:1dc25dc: 40 passed.1dc25dc: invalid extension-data JSON, ignored-member name collisions, and unsupported population metadata. The same command passed all three after removing the resolver:dotnet test tests/UnitTests/UnitTests.csproj -c Release -f net8.0 --no-restore --filter FullyQualifiedName~NonFiniteNumberCompatibilityTests --nologodotnet restore --locked-mode, full Release build (zero warnings/errors, including public API checks),bin/fmt --check, andgit diff --checkpassed.net8.0 tests ran on .NET 9 through the repository's roll-forward setting. The legacy netcoreapp3.1 target compiled but was not executed locally because the required runtime is unavailable on this ARM Mac. No live telemetry tests were used. Hosted CI results for the new commit are tracked in the PR checks.
Earlier original-fix RED/GREEN and compression reliability evidence remains in the commit/review history. Earlier autoreview findings concerned the now-deleted resolver; no new autoreview was run for this follow-up.
📝 Checklist
The float/double attribute-formatting compatibility limitation above is intentional and documented. No manual changelog entry or public API addition.
If releasing new changes
pnpm changesetto generate a changeset fileThe existing changeset explicitly selects all three NuGet packages as patch releases.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Human DRI: @marandaneto.
Implemented with the Pi worker, local shell/file tools, .NET tests, and the earlier isolated Pi autoreview. Original session: 2026-10-22T09-53-02-339Z_01a16307-4183-778d-a468-78cab970820a.
Review follow-up reused the dedicated PR worktree, reproduced all three bot findings test-first, removed the resolver as requested by the human reviewer and approved by the author, and published a new signed commit without rewriting history. Human review is required for the documented formatting tradeoff.