Fixes deserialization on events StreamLabels.Underlying and StreamLabels#283
Closed
PedroCavaleiro wants to merge 5 commits intomeenzen:mainfrom
Closed
Fixes deserialization on events StreamLabels.Underlying and StreamLabels#283PedroCavaleiro wants to merge 5 commits intomeenzen:mainfrom
PedroCavaleiro wants to merge 5 commits intomeenzen:mainfrom
Conversation
📝 WalkthroughWalkthroughMade multiple Streamlabels payload properties nullable and switched several string primitives to typed records/collections with a flexible JSON converter; added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Owner
|
Do you by any chance have the full json payloads which triggered the deserialization failures? That would allow us to add a test case here: |
Contributor
Author
|
I do, for both events, I'll add them later today when I get home. |
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Streamlabs.SocketClient/Messages/DataTypes/StreamlabelsUnderlyingMessageData.cs (1)
93-121:⚠️ Potential issue | 🟠 MajorApply
FlexibleObjectConverter<TopDonator>tomonthly,weekly,30day, andsessiontop monthly donator propertiesThe type mismatch identified is real and affects 10 properties (5 singular, 5 plural).
AllTimeTopMonthlyDonatorandAllTimeTopMonthlyDonatorsare correctly typed asTopDonatorandIReadOnlyCollection<TopDonator>respectively, but all other variants (MonthlyTopMonthlyDonator,WeeklyTopMonthlyDonator,ThirtyDayTopMonthlyDonator,SessionTopMonthlyDonatorand their plural forms) remain asrequired string.Test fixtures currently contain only empty strings for these fields, masking the issue. However, when the API returns objects (analogous to the pattern that occurs with
AllTimeTopMonthlyDonator), deserialization will fail withCannot get the value of a token type 'StartObject' as a string.Apply the same fix used for
MonthlyTopDonator(line 43–44): add[JsonConverter(typeof(FlexibleObjectConverter<TopDonator>))]attributes and change the singular variants topublic TopDonator? [PropertyName] { get; init; }, and the plural variants topublic IReadOnlyCollection<TopDonator>? [PropertyName] { get; init; }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Streamlabs.SocketClient/Messages/DataTypes/StreamlabelsUnderlyingMessageData.cs` around lines 93 - 121, The monthly/weekly/30day/session top monthly donator properties are wrongly typed as string; change each singular property (MonthlyTopMonthlyDonator, WeeklyTopMonthlyDonator, ThirtyDayTopMonthlyDonator, SessionTopMonthlyDonator) to nullable TopDonator? and each plural property (MonthlyTopMonthlyDonators, WeeklyTopMonthlyDonators, ThirtyDayTopMonthlyDonators, SessionTopMonthlyDonators) to nullable IReadOnlyCollection<TopDonator>? and add the [JsonConverter(typeof(FlexibleObjectConverter<TopDonator>))] attribute to each of those eight properties (matching the pattern used on MonthlyTopDonator), so deserialization accepts either strings or objects; leave AllTimeTopMonthlyDonator and AllTimeTopMonthlyDonators as-is.
🧹 Nitpick comments (2)
src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs (2)
36-43: SilentnullonJsonExceptiondiscards data without any diagnosticWhen a heuristically-detected JSON string fails to parse, the exception is swallowed and
nullis returned. This makes it hard to distinguish a legitimate "no data" null from a malformed payload.Consider at minimum a
Debug/Tracelog entry before returningnull, so failures are observable without impacting production behaviour.🤖 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 36 - 43, In FlexibleObjectConverter (the code path that tries JsonSerializer.Deserialize<T>(trimmed, options)), don’t silently swallow JsonException — log the exception and relevant context before returning null; update the catch (JsonException) block to write a Debug/Trace (or ILogger if available) entry that includes the exception message and the trimmed input (or at least its length/preview) and then return null so malformed payloads are observable during debugging.
53-56:Writerisks infinite recursion if the converter is ever registered globally
JsonSerializer.Serialize(writer, value, options)dispatches throughoptions.Converters. If this converter is added toJsonSerializerOptions.Convertersrather than only used via[JsonConverter]attribute, callingWritewould re-enter itself indefinitely.The standard mitigation is to use the
object-typed overload so the call skips the generic converter cache entry forT:🛡️ Safer Write pattern
public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) { - JsonSerializer.Serialize(writer, value, options); + JsonSerializer.Serialize(writer, (object?)value, typeof(T), options); }🤖 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 53 - 56, The Write method on FlexibleObjectConverter<T> can recurse if this converter is globally registered because JsonSerializer.Serialize(writer, value, options) will re-dispatch through options.Converters; change the implementation of Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) to call the non-generic/object overload so the generic converter cache is bypassed — e.g. use JsonSerializer.Serialize(writer, (object)value, typeof(T), options) (or JsonSerializer.Serialize(writer, (object)value, options) with explicit Type) to serialize without re-entering FlexibleObjectConverter<T>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/Streamlabs.SocketClient/Messages/DataTypes/StreamlabelsUnderlyingMessageData.cs`:
- Around line 93-121: The monthly/weekly/30day/session top monthly donator
properties are wrongly typed as string; change each singular property
(MonthlyTopMonthlyDonator, WeeklyTopMonthlyDonator, ThirtyDayTopMonthlyDonator,
SessionTopMonthlyDonator) to nullable TopDonator? and each plural property
(MonthlyTopMonthlyDonators, WeeklyTopMonthlyDonators,
ThirtyDayTopMonthlyDonators, SessionTopMonthlyDonators) to nullable
IReadOnlyCollection<TopDonator>? and add the
[JsonConverter(typeof(FlexibleObjectConverter<TopDonator>))] attribute to each
of those eight properties (matching the pattern used on MonthlyTopDonator), so
deserialization accepts either strings or objects; leave
AllTimeTopMonthlyDonator and AllTimeTopMonthlyDonators as-is.
---
Nitpick comments:
In `@src/Streamlabs.SocketClient/Converters/FlexibleObjectConverter.cs`:
- Around line 36-43: In FlexibleObjectConverter (the code path that tries
JsonSerializer.Deserialize<T>(trimmed, options)), don’t silently swallow
JsonException — log the exception and relevant context before returning null;
update the catch (JsonException) block to write a Debug/Trace (or ILogger if
available) entry that includes the exception message and the trimmed input (or
at least its length/preview) and then return null so malformed payloads are
observable during debugging.
- Around line 53-56: The Write method on FlexibleObjectConverter<T> can recurse
if this converter is globally registered because
JsonSerializer.Serialize(writer, value, options) will re-dispatch through
options.Converters; change the implementation of Write(Utf8JsonWriter writer, T
value, JsonSerializerOptions options) to call the non-generic/object overload so
the generic converter cache is bypassed — e.g. use
JsonSerializer.Serialize(writer, (object)value, typeof(T), options) (or
JsonSerializer.Serialize(writer, (object)value, options) with explicit Type) to
serialize without re-entering FlexibleObjectConverter<T>.
Contributor
Author
|
So it appears that when there are no values StreamLabs sends a empty string, I've created a custom converter that when applied to a property tries to do the following
For this to work I needed to make some properties non required and nullable As requested I added the test of the exact JSON that was causing the issue, all 3 are now passing |
Owner
|
Thanks for your contribution! I've merged these changes in #289 since I couldn't modify your branch. I'll release this as Regarding the custom JSON converter, yeah the Streamlabs api is seriously messed up. It makes no sense that they're sending an empty string for a missing object instead of null. The property naming and data types are also very inconsistent. Their backend likely is a messy Node.js service. That's actually the reason I made this library, to make using the Streamlabs API more ergonomic. I hope this works well for you. |
Owner
|
The Thanks again for opening this PR, you're the first contributor who's helped out with the project. If you need anything or find more issues, feel free to let me know. |
Contributor
Author
|
Thank you for your work. I will provide any more fixes if I find them. |
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.
A required property that shouldn't be on StreamLabels event and a incorrect property type on StreamLabels.Underlying event were fixed
Here's a screenshot of one of the issues
Summary by CodeRabbit
Bug Fixes
New Features
Tests