fix(flags): honor versioned local property matching - #317
Draft
marandaneto wants to merge 2 commits into
Draft
Conversation
Contributor
posthog-dotnet Compliance ReportDate: 2026-09-05 15:09:19 UTC ✅ All Tests Passed!17/17 tests passed Feature_Flags Tests✅ 17/17 tests passed View Details
|
marandaneto
marked this pull request as ready for review
September 5, 2026 17:29
Contributor
Prompt To Fix All With AI### Issue 1
src/PostHog/Json/PropertyFilterValue.cs:205-207
**Nested decimals normalize differently**
Version 2 applies `StringifyDecimal` only when the property itself is a `decimal`. A decimal nested in a CLR collection still uses the generic invariant conversion, so `[1.00m]` becomes `"[1.00]"` while the wire-equivalent JSON `[1.00]` becomes `"[1.0]"`. As a result, local `exact` and `is_not` evaluations can return different flag values depending on whether the same property arrived as CLR objects or JSON.
### Issue 2
src/PostHog/Json/PropertyFilterValue.cs:233-235
**Large decimals lose precision**
`StringifyDecimal` reparses the serialized number through `StringifyJsonElement`, which converts values outside the 64-bit integer ranges to `double`. Distinct high-precision decimals near `decimal.MaxValue` can therefore collapse to the same string and incorrectly satisfy version-2 `exact` matching. The maximum-value test does not catch this because both operands undergo the same lossy conversion.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(flags): normalize decimal properties..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+205
to
+207
| var comparand = overrideValue is decimal decimalValue | ||
| ? StringifyDecimal(decimalValue) | ||
| : ToInvariantString(overrideValue); |
Contributor
There was a problem hiding this comment.
Version 2 applies StringifyDecimal only when the property itself is a decimal. A decimal nested in a CLR collection still uses the generic invariant conversion, so [1.00m] becomes "[1.00]" while the wire-equivalent JSON [1.00] becomes "[1.0]". As a result, local exact and is_not evaluations can return different flag values depending on whether the same property arrived as CLR objects or JSON.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/PostHog/Json/PropertyFilterValue.cs
Line: 205-207
Comment:
**Nested decimals normalize differently**
Version 2 applies `StringifyDecimal` only when the property itself is a `decimal`. A decimal nested in a CLR collection still uses the generic invariant conversion, so `[1.00m]` becomes `"[1.00]"` while the wire-equivalent JSON `[1.00]` becomes `"[1.0]"`. As a result, local `exact` and `is_not` evaluations can return different flag values depending on whether the same property arrived as CLR objects or JSON.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+233
to
+235
| // Preserve the wire number's integer/float distinction and normalize its scale and precision like JSON filters. | ||
| using var document = JsonDocument.Parse(JsonSerializer.Serialize(value)); | ||
| return StringifyJsonElement(document.RootElement); |
Contributor
There was a problem hiding this comment.
StringifyDecimal reparses the serialized number through StringifyJsonElement, which converts values outside the 64-bit integer ranges to double. Distinct high-precision decimals near decimal.MaxValue can therefore collapse to the same string and incorrectly satisfy version-2 exact matching. The maximum-value test does not catch this because both operands undergo the same lossy conversion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/PostHog/Json/PropertyFilterValue.cs
Line: 233-235
Comment:
**Large decimals lose precision**
`StringifyDecimal` reparses the serialized number through `StringifyJsonElement`, which converts values outside the 64-bit integer ranges to `double`. Distinct high-precision decimals near `decimal.MaxValue` can therefore collapse to the same string and incorrectly satisfy version-2 `exact` matching. The maximum-value test does not catch this because both operands undergo the same lossy conversion.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
marandaneto
marked this pull request as draft
September 6, 2026 07:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Motivation and Context
Local feature flag evaluation needs to honor the property matching version returned with flag definitions. Boolean coercion can otherwise produce different results from the service, such as matching
falseagainst"banana"or rejecting"true"from["true", "false"].This follows the backend change and shared matching contract.
property_matching_versionmetadata in the definitions response. Only exactly version 2 enables explicit matching. Missing, version 1, and other versions keep legacy behavior.is_notcomplements exact matching for known properties. Missing properties and unsupported null filters remain inconclusive.1.00mnow matches numeric filter[1.00], whose canonical representation is1.0. Preserve the distinction between integer1and floating-point1.0. This correction does not change legacy decimal matching or other operators.LocalEvaluatorsnapshot for person, group, recursive cohort, and dependency evaluation. Existing loader replacement already keeps metadata with definitions, so no cache-provider changes are needed. Version-only refreshes take effect, while 304 responses and failures retain the previous snapshot.PostHogpatch changeset.The optional harness coverage is related, but this PR does not opt an SDK adapter into it.
💚 How did you test it?
Independently reproduced the decimal regression on the previous PR head. Both
exactandis_notreturned the wrong result for1.00magainst[1.00]on both test targets, while the wire-equivalent JSON and legacy controls passed. Added permanent decimal cases for scale, negative values, zero, exponent formatting, high precision, large integers, scalar string matching, nonmatches, and legacy compatibility.Validation of the submitted source:
All 57 focused tests passed on each target. Coverage includes exact/is_not across missing, 1, 2, 0, and 3 versions, boolean and nested-array operands, known nulls, Unicode normalization, decimal normalization, person/group/cohort/dependency propagation, and cached snapshot retention. Real
PostHogClientsingle, bulk, and full-result APIs use mocked definitions HTTP across version-only reloads and assert that no remote/flagsrequest is sent.The full suite passed with 1,198 tests on net8.0 and 1,189 on netcoreapp3.1, with two existing cancellation-test skips per target. The first full run had an unrelated timing-sensitive failure in
RetriesOnGatewayHttpStatusCodeThenSucceedson netcoreapp3.1. That test passed in isolation and the full suite passed on rerun without changes. All three library targets built with zero warnings or errors, including public API analyzers. Focused whitespace and style checks passed with workspace-loading warnings only.Both test targets use repository-configured major roll-forward to installed .NET 9.0.19, not native .NET 8 or 3.1 runtimes. Full solution, live-service, and native-runtime testing were not run. Unrelated recursive FilterSet serialization behavior observed during earlier test development was not changed.
git diff --checkand the required isolated committed-branch autoreview passed onee37c1c8404e1e02a51c236278b5f9ed257d0c7cagainsteadb15eaa78ee86469f9841eb23b8b14770b9e96, with no actionable findings. Local checks are not a claim that CI passed.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset fileThe existing
.changeset/versioned-property-matching.mdselects a patch release forPostHog. It was preserved, not regenerated withpnpm changesetduring publication.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented and published with Pi agent assistance using Git, dotnet, GitHub CLI, and the isolated autoreview helper. The human-directed task was to apply the shared versioned matching contract and fix confirmed review findings in this draft. Local session artifacts are not publicly linked.
Kept the existing immutable evaluator snapshot as the metadata ownership boundary rather than adding cache infrastructure. Preserved nullable response metadata and the public legacy-default helper. The decimal correction reuses existing JSON normalization only for explicit matching. Human review is required before merge.