Merged
Conversation
The remote config endpoint returns JSON encoded in a string.
`"{\"foo\": \"bar\",\"baz\": 42}"`
Instead of:
`{"foo": "bar","baz": 42}`
However, we may change that in the future.
So this is implemented in a forward-compatible way to handle both options.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PR Summary
This PR enhances the PostHog .NET client's handling of remote config settings to support both string-encoded and raw JSON formats from the API endpoint.
- Added forward-compatible JSON parsing in
PostHogClient.csto handle both"{\"foo\": \"bar\"}"and{"foo": "bar"}formats - Implemented comprehensive test cases in
RemoteConfigTests.cscovering various JSON response scenarios (raw, string-encoded, double-encoded) - Added helper method
TryParseJsonwith proper error handling for safely parsing string-encoded JSON - Updated version to 1.0.0-beta.10 to reflect the remote config handling improvements
- Enhanced sample web application to demonstrate both encrypted and unencrypted remote config settings
6 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| await posthog.GetRemoteConfigPayloadAsync( | ||
| "unencrypted-remote-config-setting"); | ||
|
|
||
| > @Model.UnencryptedRemoteConfigSetting |
Contributor
There was a problem hiding this comment.
style: The '>' character before @Model.UnencryptedRemoteConfigSetting seems unnecessary and inconsistent with the encrypted setting display below
|
|
||
| NonExistentFlag = await posthog.IsFeatureEnabledAsync("non-existent-flag", UserId); | ||
|
|
||
| UnencryptedRemoteConfigSetting = (await posthog.GetRemoteConfigPayloadAsync("unencrypted-remote-config-setting", HttpContext.RequestAborted))?.RootElement.GetRawText(); |
Contributor
There was a problem hiding this comment.
logic: GetRawText() may throw if RootElement is invalid JSON. Consider wrapping in try-catch
Comment on lines
+111
to
+113
| UnencryptedRemoteConfigSetting = (await posthog.GetRemoteConfigPayloadAsync("unencrypted-remote-config-setting", HttpContext.RequestAborted))?.RootElement.GetRawText(); | ||
|
|
||
| EncryptedRemoteConfigSetting = (await posthog.GetRemoteConfigPayloadAsync("encrypted-remote-config-setting", HttpContext.RequestAborted))?.RootElement.GetRawText(); |
Contributor
There was a problem hiding this comment.
style: These calls could be parallelized using Task.WhenAll for better performance
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.
The remote config endpoint returns JSON encoded in a string.
"{\"foo\": \"bar\",\"baz\": 42}"Instead of:
{"foo": "bar","baz": 42}However, we may change that in the future.
So this is implemented in a forward-compatible way to handle both options.