-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[Json] Remove unnecessary Debug.Asserts #116833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes unnecessary Debug.Assert calls from the JsonObjectConverter and JsonArrayConverter, streamlining error handling in debug builds.
- Removed Debug.Assert from JsonObjectConverter to simplify the handling of unexpected token types.
- Removed Debug.Assert from JsonArrayConverter to align behavior with throwing exceptions on invalid token types.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonObjectConverter.cs | Removed a Debug.Assert call in the default case for invalid JSON tokens. |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonArrayConverter.cs | Removed a Debug.Assert call in the default case for invalid JSON tokens. |
Comments suppressed due to low confidence (2)
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonObjectConverter.cs:71
- The removal of Debug.Assert improves clarity and consistency by relying solely on exception throwing for invalid token types. Ensure that thorough tests cover this error path across build configurations.
default:
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonArrayConverter.cs:33
- Removing Debug.Assert here aligns the error handling mechanism with that of JsonObjectConverter. Verify that the associated test coverage for invalid JSON arrays is sufficient in both debug and non-debug builds.
default:
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
...es/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonArrayConverter.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this case being reachable is intentional, so the assert should be fine to remove.
Based on git history, it looks like initially JsonNodeConverter
was used to deserialize JsonArray
and ensured only JSON arrays got to the Write method. Later JsonArrayConverter
was added to the metadata services so any JSON value can get here if it's being deserialized as a JsonArray
.
/ba-g dead lettered queue |
These
Debug.Asserts
are over-aggressive. It's perfectly valid to do something likeJsonSerializer.Deserialize<JsonArray>("\"str\":\"value\"");
. And we will get aJsonException
in non-debug builds.