Add support for remote config settings #49

haacked · 2025-02-13T18:49:27Z

This PR also improves exception handling so that exceptions thrown from methods retrieving flags are unlikely. Error situations are logged instead.

This also changes FeatureFlag.Payload to be a JsonDocument.

greptile-apps

PR Summary

This PR adds remote config support and improves error handling in the PostHog .NET SDK, with significant changes to feature flag payload handling.

Changed FeatureFlag.Payload from string to JsonDocument for more robust JSON handling in /src/PostHog/Features/FeatureFlag.cs
Added new GetRemoteConfigPayloadAsync method in /src/PostHog/Api/PostHogApiClient.cs for retrieving remote config settings
Improved error handling by catching broader exceptions and adding structured logging throughout the codebase
Added UnauthorizedApiResult record in /src/PostHog/Api/ErrorApiResult.cs for better handling of 401 responses
Bumped version to 1.0.0-beta.6 in /Directory.Build.props to reflect new functionality

_{29 file(s) reviewed, 24 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-02-13T18:50:20Z

src/PostHog/Api/LocalEvaluationApiResult.cs

 /// <summary>
 /// A filter that filters on a property.
 /// </summary>
-/// <param name="Type">The type of filter. Either "person" or "group".</param>
-public record PropertyFilter(
-    FilterType Type,
-    string Key,
-    PropertyFilterValue Value,
-    ComparisonOperator? Operator = null,
-    [property: JsonPropertyName("group_type_index")]
-    int? GroupTypeIndex = null,
-    bool Negation = false) : Filter(Type)
+/// <summary>
+/// A filter that filters on a property.
+/// </summary>


syntax: Duplicate XML summary comment for PropertyFilter class

Suggested change

/// <summary>

/// A filter that filters on a property.

/// </summary>

/// <param name="Type">The type of filter. Either "person" or "group".</param>

public record PropertyFilter(

FilterType Type,

string Key,

PropertyFilterValue Value,

ComparisonOperator? Operator = null,

[property: JsonPropertyName("group_type_index")]

int? GroupTypeIndex = null,

bool Negation = false) : Filter(Type)

/// <summary>

/// A filter that filters on a property.

/// </summary>

/// <summary>

/// A filter that filters on a property.

/// </summary>

greptile-apps · 2025-02-13T18:50:23Z

src/PostHog/Api/ErrorApiResult.cs

+/// <param name="Type">The type of error.</param>
+/// <param name="Code">The error code.</param>
+/// <param name="Detail">Information about the error.</param>
+/// <param name="Attr">???</param>


style: The Attr parameter has unclear purpose and type. Consider documenting its purpose or removing if unused. Using object type is too permissive and could lead to runtime errors.

greptile-apps · 2025-02-13T18:50:24Z

src/PostHog/Api/ErrorApiResult.cs

+/// <param name="Code">The error code.</param>
+/// <param name="Detail">Information about the error.</param>
+/// <param name="Attr">???</param>
+internal record UnauthorizedApiResult(string Type, string Code, string Detail, object Attr);


style: Consider making properties nullable with string? since API responses may not always include all fields.

greptile-apps · 2025-02-13T18:50:25Z

src/PostHog/Api/ErrorApiResult.cs

+/// <param name="Attr">???</param>
+internal record UnauthorizedApiResult(string Type, string Code, string Detail, object Attr);


syntax: The XML documentation uses param tags incorrectly for record properties. Should use property tags instead: <property name="Type">The type of error.</property>

greptile-apps · 2025-02-13T18:50:40Z

src/PostHog/Api/PostHogApiClient.cs

+        {
+            _logger.LogErrorUnableToGetFeatureFlagsAndPayloads(e);


logic: LogErrorUnableToGetFeatureFlagsAndPayloads is used but not defined in the logger extensions

greptile-apps · 2025-02-13T18:53:36Z

tests/UnitTests/Features/RemoteConfigTests.cs

+        var container = new TestContainer("fake-personal-api-key");
+        var client = container.Activate<PostHogClient>();
+
+        var result = await client.GetRemoteConfigPayloadAsync("non-existent-key");


style: missing cancellation token parameter - should test behavior with cancellation

greptile-apps · 2025-02-13T18:53:53Z

tests/UnitTests/Json/FilterSerializationTests.cs

                       ]
-                   } 
+                   }
                   """;
        var result = await JsonSerializerHelper.DeserializeFromCamelCaseJsonStringAsync<Filter>(json);

        var propertyFilter = Assert.IsType<FilterSet>(result);


style: variable name 'propertyFilter' is misleading since it's actually a FilterSet

Suggested change

var propertyFilter = Assert.IsType<FilterSet>(result);

var filterSet = Assert.IsType<FilterSet>(result);

greptile-apps · 2025-02-13T18:54:17Z

tests/UnitTests/Library/JsonAssert.cs

+using System.Text.Json;
+using System.Text.Json.Nodes;
+
+public static class JsonAssert


style: Class is missing XML documentation comments which are present on other public types in the codebase

greptile-apps · 2025-02-13T18:54:18Z

tests/UnitTests/Library/JsonAssert.cs

+    public static void Equal<T>(T expectedJson, JsonDocument? actualJson) =>
+        Equal(JsonSerializer.SerializeToDocument(expectedJson, typeof(T)), actualJson);


logic: No null check on expectedJson parameter. Could throw NullReferenceException when T is a reference type

Suggested change

public static void Equal<T>(T expectedJson, JsonDocument? actualJson) =>

Equal(JsonSerializer.SerializeToDocument(expectedJson, typeof(T)), actualJson);

public static void Equal<T>(T expectedJson, JsonDocument? actualJson)

{

if (expectedJson is null)

throw new ArgumentNullException(nameof(expectedJson));

Equal(JsonSerializer.SerializeToDocument(expectedJson, typeof(T)), actualJson);

}

greptile-apps · 2025-02-13T18:54:19Z

tests/UnitTests/Library/JsonAssert.cs

+    public static void Equal(
+        IReadOnlyDictionary<string, JsonDocument> expectedJson,
+        IReadOnlyDictionary<string, JsonDocument>? actualJson)
+    {


logic: Method should validate that expectedJson is not null since it's not marked as nullable

Suggested change

public static void Equal(

IReadOnlyDictionary<string, JsonDocument> expectedJson,

IReadOnlyDictionary<string, JsonDocument>? actualJson)

{

public static void Equal(

IReadOnlyDictionary<string, JsonDocument> expectedJson,

IReadOnlyDictionary<string, JsonDocument>? actualJson)

{

ArgumentNullException.ThrowIfNull(expectedJson);

haacked added 6 commits February 12, 2025 16:11

Improve exception handling

1275228

Payload values are now JsonDocuments

3e88a21

Refactor and document API types

81879e4

Add greptile config

74665e7

Implement remote config setting

eb2e757

Bump version to 1.0.0-beta.6

194c9e7

haacked merged commit f7f0157 into main Feb 13, 2025
4 checks passed

haacked deleted the haacked/payload-refactorings branch February 13, 2025 18:53

greptile-apps bot reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for remote config settings #49

Add support for remote config settings #49

Uh oh!

haacked commented Feb 13, 2025

Uh oh!

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

greptile-apps bot Feb 13, 2025

Uh oh!

Uh oh!

		/// <param name="Attr">???</param>
		internal record UnauthorizedApiResult(string Type, string Code, string Detail, object Attr);

	var propertyFilter = Assert.IsType<FilterSet>(result);
	var filterSet = Assert.IsType<FilterSet>(result);

		public static void Equal<T>(T expectedJson, JsonDocument? actualJson) =>
		Equal(JsonSerializer.SerializeToDocument(expectedJson, typeof(T)), actualJson);

Add support for remote config settings #49

Add support for remote config settings #49

Uh oh!

Conversation

haacked commented Feb 13, 2025

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!