fix: handle more streamlabels event variations#289
Merged
Conversation
Fix deserialziation
Introduces a `FlexibleObjectConverter` to handle varying JSON input types for properties within Streamlabs messages. This converter allows deserialization from direct JSON objects/arrays, or from double-encoded JSON strings, preventing errors due to API inconsistencies. Applies the `FlexibleObjectConverter` to properties in `StreamlabelsUnderlyingMessageData` that exhibit this flexible behavior, such as donator objects and lists, making them nullable to accommodate unparseable string values. Adds a new test case to validate the robust deserialization of these flexible JSON structures.
📝 WalkthroughWalkthroughThis PR introduces a flexible JSON deserialization mechanism for handling stream-labels data. It adds a generic Changes
Sequence Diagram(s)sequenceDiagram
participant JsonReader as JSON Reader
participant Converter as FlexibleObjectConverter
participant Validator as IsJsonObjectOrArray()
participant Deserializer as JsonSerializer
participant Target as T (Target Type)
JsonReader->>Converter: Read(token)
alt StartObject or StartArray
Converter->>Deserializer: Deserialize directly
Deserializer->>Target: Create instance
else String Token
Converter->>Validator: Validate string content
alt Valid JSON structure
Validator-->>Converter: true
Converter->>Deserializer: Deserialize trimmed string
Deserializer->>Target: Create instance
else Invalid structure
Validator-->>Converter: false
Converter->>Target: Return null
end
else Other Token
Converter->>Target: Return null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
==========================================
- Coverage 80.18% 80.17% -0.01%
==========================================
Files 59 61 +2
Lines 777 812 +35
Branches 54 58 +4
==========================================
+ Hits 623 651 +28
- Misses 125 129 +4
- Partials 29 32 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs (2)
13-14: Consider sealing the class.Nothing prevents subclassing
FlexibleObjectConverter<T>. Unless extensibility is intentional, addingsealedremoves that surface area.♻️ Proposed change
-public class FlexibleObjectConverter<T> : JsonConverter<T> +public sealed class FlexibleObjectConverter<T> : JsonConverter<T>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs` around lines 13 - 14, The FlexibleObjectConverter<T> class is currently inheritable; if extensibility is not required, mark the class as sealed by adding the sealed modifier to its declaration (public sealed class FlexibleObjectConverter<T> : JsonConverter<T>) to prevent subclassing; keep it non-sealed only if you intentionally expect consumers to derive from FlexibleObjectConverter<T>.
25-26:Writewill infinitely recurse if this converter is ever applied at the class level ofT.
JsonSerializer.Serialize(writer, value, options)causes the serializer to look up a converter for the runtime type ofvalue. IfFlexibleObjectConverter<T>is ever placed on the class declaration ofT(instead of only on properties), this would callWrite→Serialize→Write→ … indefinitely. Since the converter is currently only used via property-level[JsonConverter]attributes it is safe today, but consider documenting this constraint or guarding against it.📝 Suggested doc comment
/// <summary> /// Provides a flexible JSON converter for <typeparamref name="T"/> ... +/// <para> +/// This converter must only be applied at the <em>property</em> level via <c>[JsonConverter(...)]</c>. +/// Applying it as a class-level attribute on <typeparamref name="T"/> itself will cause infinite +/// recursion during serialization. +/// </para> /// </summary>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs` around lines 25 - 26, The Write method on FlexibleObjectConverter<T> will recurse if the converter is applied to type T because JsonSerializer.Serialize(writer, value, options) will resolve this converter again; fix by using the overload that supplies the runtime type to JsonSerializer.Serialize (so the serializer uses the concrete runtime type instead of looking up the converter for T) when writing (use value.GetType() or fallback to typeof(T) for null), and add a brief XML doc comment on class FlexibleObjectConverter<T> noting it must not be applied at the type level (or that the runtime-type overload is used to prevent recursion).test/Streamlabs.SocketClient.Tests/Converters/FlexibleObjectConverterTests.cs (1)
23-99: Missing test forStartArraytoken.The converter's
StartArraybranch is used in production for everyIReadOnlyCollection<T>?property but has no coverage here. Add a test with an array-typedSamplePayloadto verify that path deserializes correctly.✅ Suggested test
private sealed class SampleListClass { [JsonConverter(typeof(FlexibleObjectConverter<IReadOnlyCollection<SamplePayload>>))] public IReadOnlyCollection<SamplePayload>? Items { get; set; } } [Test] public async Task Read_ArrayToken_DeserializesList() { // Arrange var json = """{"Items":[{"Name":"Alpha","Count":1},{"Name":"Beta","Count":2}]}"""; // Act var result = JsonSerializer.Deserialize<SampleListClass>(json); // Assert await Assert.That(result).IsNotNull(); await Assert.That(result!.Items).IsNotNull(); await Assert.That(result.Items!.Count).IsEqualTo(2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Streamlabs.SocketClient.Tests/Converters/FlexibleObjectConverterTests.cs` around lines 23 - 99, Add a unit test to cover the converter's StartArray branch by creating a SampleListClass with an Items property annotated with [JsonConverter(typeof(FlexibleObjectConverter<IReadOnlyCollection<SamplePayload>>))], then add a test method Read_ArrayToken_DeserializesList that deserializes json like {"Items":[{"Name":"Alpha","Count":1},{"Name":"Beta","Count":2}]} into SampleListClass and asserts the result and Items are not null and Items.Count == 2; use the same assertion style as existing tests and the JsonSerializer.Deserialize<SampleListClass> call to exercise FlexibleObjectConverter's array handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs`:
- Around line 16-23: The Read method in FlexibleObjectConverter<T> handles
JsonException differently between branches: DeserializeString swallows
JsonException and returns null, but the StartObject/StartArray branches call
JsonSerializer.Deserialize<T>(ref reader, options) and let exceptions propagate;
make them consistent by wrapping the StartObject and StartArray deserialize
calls in a try/catch(JsonException) that returns null on error (same behavior as
DeserializeString) so malformed/structurally-mismatched payloads for
FlexibleObjectConverter<TopDonator> don’t crash message deserialization.
---
Nitpick comments:
In `@src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs`:
- Around line 13-14: The FlexibleObjectConverter<T> class is currently
inheritable; if extensibility is not required, mark the class as sealed by
adding the sealed modifier to its declaration (public sealed class
FlexibleObjectConverter<T> : JsonConverter<T>) to prevent subclassing; keep it
non-sealed only if you intentionally expect consumers to derive from
FlexibleObjectConverter<T>.
- Around line 25-26: The Write method on FlexibleObjectConverter<T> will recurse
if the converter is applied to type T because JsonSerializer.Serialize(writer,
value, options) will resolve this converter again; fix by using the overload
that supplies the runtime type to JsonSerializer.Serialize (so the serializer
uses the concrete runtime type instead of looking up the converter for T) when
writing (use value.GetType() or fallback to typeof(T) for null), and add a brief
XML doc comment on class FlexibleObjectConverter<T> noting it must not be
applied at the type level (or that the runtime-type overload is used to prevent
recursion).
In
`@test/Streamlabs.SocketClient.Tests/Converters/FlexibleObjectConverterTests.cs`:
- Around line 23-99: Add a unit test to cover the converter's StartArray branch
by creating a SampleListClass with an Items property annotated with
[JsonConverter(typeof(FlexibleObjectConverter<IReadOnlyCollection<SamplePayload>>))],
then add a test method Read_ArrayToken_DeserializesList that deserializes json
like {"Items":[{"Name":"Alpha","Count":1},{"Name":"Beta","Count":2}]} into
SampleListClass and asserts the result and Items are not null and Items.Count ==
2; use the same assertion style as existing tests and the
JsonSerializer.Deserialize<SampleListClass> call to exercise
FlexibleObjectConverter's array handling.
Comment on lines
+16
to
+23
| public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => | ||
| reader.TokenType switch | ||
| { | ||
| JsonTokenType.StartObject => JsonSerializer.Deserialize<T>(ref reader, options), | ||
| JsonTokenType.StartArray => JsonSerializer.Deserialize<T>(ref reader, options), | ||
| JsonTokenType.String => DeserializeString(ref reader, options), | ||
| _ => null, | ||
| }; |
There was a problem hiding this comment.
Inconsistent exception handling between StartObject/StartArray and DeserializeString.
DeserializeString silently returns null on any JsonException, but the StartObject and StartArray branches propagate exceptions to the caller. Two realistic failure modes:
- The API sends an array for a property typed as
FlexibleObjectConverter<TopDonator>(not a collection).JsonSerializer.Deserialize<TopDonator>(ref reader, options)on an array token would throw aJsonExceptionthat crashes the whole message deserialization. - The API sends a structurally valid object/array token but with missing
requiredfields on the target type.
For the same resilience as DeserializeString, wrap the object/array branches consistently:
🛡️ Proposed fix — consistent exception handling
public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) =>
reader.TokenType switch
{
- JsonTokenType.StartObject => JsonSerializer.Deserialize<T>(ref reader, options),
- JsonTokenType.StartArray => JsonSerializer.Deserialize<T>(ref reader, options),
+ JsonTokenType.StartObject => TryDeserializeToken(ref reader, options),
+ JsonTokenType.StartArray => TryDeserializeToken(ref reader, options),
JsonTokenType.String => DeserializeString(ref reader, options),
_ => null,
};
+private static T? TryDeserializeToken(ref Utf8JsonReader reader, JsonSerializerOptions options)
+{
+ try
+ {
+ return JsonSerializer.Deserialize<T>(ref reader, options);
+ }
+ catch (JsonException)
+ {
+ return null;
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs` around
lines 16 - 23, The Read method in FlexibleObjectConverter<T> handles
JsonException differently between branches: DeserializeString swallows
JsonException and returns null, but the StartObject/StartArray branches call
JsonSerializer.Deserialize<T>(ref reader, options) and let exceptions propagate;
make them consistent by wrapping the StartObject and StartArray deserialize
calls in a try/catch(JsonException) that returns null on error (same behavior as
DeserializeString) so malformed/structurally-mismatched payloads for
FlexibleObjectConverter<TopDonator> don’t crash message deserialization.
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
2 participants
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.
supersedes #283
Summary by CodeRabbit
Release Notes