Skip to content

fix: serialize non-finite event properties as null - #348

Merged
marandaneto merged 6 commits into
mainfrom
fix/non-finite-event-properties-20261022
Oct 22, 2026
Merged

marandaneto merged 6 commits into
mainfrom
fix/non-finite-event-properties-20261022

Conversation

@marandaneto

@marandaneto marandaneto commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

💡 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 NumberHandlingJsonTypeInfoResolver instead 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 rejected Populate collections.

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:

  • Baseline serializer suite on 1dc25dc: 40 passed.
  • Added three focused regression tests before modifying production code. All failed for the reported reasons on 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 --nologo
  • Retained those regressions. Updated attribute-specific tests to the explicitly approved limitation, while preserving option-level handling, explicit custom converters, nested non-finite/null behavior, finite dictionary keys, and unsupported-value checks.
  • Focused serializer, compatibility, and gzip/plaintext Capture tests: 52 passed, including neighboring-event delivery and caller-input preservation.
  • Full net8.0 suites: 1,345 passed, 2 existing skips.
  • dotnet restore --locked-mode, full Release build (zero warnings/errors, including public API checks), bin/fmt --check, and git diff --check passed.

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

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

The float/double attribute-formatting compatibility limitation above is intentional and documented. No manual changelog entry or public API addition.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

The 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.

@marandaneto marandaneto self-assigned this Oct 22, 2026

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/PostHog/Json/JsonSerializerHelper.cs
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

posthog-dotnet Compliance Report

Date: 2026-10-22T14:45:46.941697+00:00
Duration: 124754ms

✅ All Tests Passed!

47/47 tests passed


Capture Tests

✅ 30/30 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 224ms
Format Validation.Event Has Uuid ✅ 122ms
Format Validation.Event Has Lib Properties ✅ 108ms
Format Validation.Distinct Id Is String ✅ 109ms
Format Validation.Token Is Present ✅ 112ms
Format Validation.Custom Properties Preserved ✅ 115ms
Format Validation.Event Has Timestamp ✅ 107ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 117ms
Retry Behavior.Retries On 503 ✅ 8120ms
Retry Behavior.Does Not Retry On 400 ✅ 2114ms
Retry Behavior.Does Not Retry On 401 ✅ 2112ms
Retry Behavior.Respects Retry After Header ✅ 8117ms
Retry Behavior.Implements Backoff ✅ 22133ms
Retry Behavior.Retries On 500 ✅ 6115ms
Retry Behavior.Retries On 502 ✅ 6115ms
Retry Behavior.Retries On 504 ✅ 6116ms
Retry Behavior.Max Retries Respected ✅ 22131ms
Deduplication.Generates Unique Uuids ✅ 114ms
Deduplication.Preserves Uuid On Retry ✅ 6116ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 13127ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 6114ms
Deduplication.No Duplicate Events In Batch ✅ 113ms
Deduplication.Different Events Have Different Uuids ✅ 110ms
Compression.Sends Gzip When Enabled ✅ 108ms
Batch Format.Uses Proper Batch Structure ✅ 108ms
Batch Format.Flush With No Events Sends Nothing ✅ 105ms
Batch Format.Multiple Events Batched Together ✅ 113ms
Error Handling.Does Not Retry On 403 ✅ 2109ms
Error Handling.Does Not Retry On 413 ✅ 2112ms
Error Handling.Retries On 408 ✅ 6115ms

Feature_Flags Tests

✅ 17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 136ms
Request Payload.Flags Request Uses V2 Query Param ✅ 107ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 107ms
Request Payload.Flags Request Omits Authorization Header ✅ 108ms
Request Payload.Token In Flags Body Matches Init ✅ 107ms
Request Payload.Groups Round Trip ✅ 113ms
Request Payload.Groups Default To Empty Object ✅ 107ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 110ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 108ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 109ms
Request Lifecycle.No Flags Request On Init Alone ✅ 2ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 107ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 213ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 110ms
Retry Behavior.Retries Flags On 502 ✅ 1112ms
Retry Behavior.Retries Flags On 504 ✅ 1110ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 210ms

@posthog posthog Bot removed the posthog-review label Oct 22, 2026
@marandaneto
marandaneto marked this pull request as ready for review October 22, 2026 13:28
@marandaneto
marandaneto requested a review from a team as a code owner October 22, 2026 13:28

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 1dc25dc: 3 Should fix.

Comment thread src/PostHog/Json/NumberHandlingJsonTypeInfoResolver.cs Outdated
Comment thread src/PostHog/Json/NumberHandlingJsonTypeInfoResolver.cs Outdated
Comment thread src/PostHog/Json/NumberHandlingJsonTypeInfoResolver.cs Outdated

@turnipdabeets turnipdabeets left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@marandaneto

Copy link
Copy Markdown
Member Author

@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.

@marandaneto
marandaneto enabled auto-merge (squash) October 22, 2026 14:42
@marandaneto
marandaneto merged commit 1cb39c3 into main Oct 22, 2026
21 of 22 checks passed
@marandaneto
marandaneto deleted the fix/non-finite-event-properties-20261022 branch October 22, 2026 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants