-
Notifications
You must be signed in to change notification settings - Fork 17
Add migrating-newtonsoft-to-system-text-json skill #200
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
base: main
Are you sure you want to change the base?
Changes from all commits
04a5081
0a64de0
a9285a1
4a507df
9773e1c
c88d86e
2790f37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,249 @@ | ||||||||||||||||||||||||||
| ```skill | ||||||||||||||||||||||||||
| --- | ||||||||||||||||||||||||||
| name: migrating-newtonsoft-to-system-text-json | ||||||||||||||||||||||||||
| description: Migrate from Newtonsoft.Json to System.Text.Json, handling behavioral differences, custom converters, and common breaking changes. Use when converting a project from Newtonsoft.Json (Json.NET) to the built-in System.Text.Json serializer. | ||||||||||||||||||||||||||
| --- | ||||||||||||||||||||||||||
|
Comment on lines
+1
to
+5
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Migrating from Newtonsoft.Json to System.Text.Json | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## When to Use | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| - Migrating an existing project from Newtonsoft.Json to System.Text.Json | ||||||||||||||||||||||||||
| - Removing the Newtonsoft.Json dependency for performance or AOT compatibility | ||||||||||||||||||||||||||
| - Fixing serialization differences after switching to System.Text.Json | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## When Not to Use | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| - The project requires Newtonsoft.Json features that System.Text.Json cannot support (extremely rare edge cases like `$ref/$id` with deep graphs) | ||||||||||||||||||||||||||
| - The user is already using System.Text.Json and just needs help with it | ||||||||||||||||||||||||||
| - The user explicitly wants to keep Newtonsoft.Json | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## Inputs | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| | Input | Required | Description | | ||||||||||||||||||||||||||
| |-------|----------|-------------| | ||||||||||||||||||||||||||
| | Code using Newtonsoft.Json | Yes | Models, serialization calls, custom converters | | ||||||||||||||||||||||||||
| | .NET version | No | Determines which System.Text.Json features are available | | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## Workflow | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Step 1: Understand the critical behavioral differences | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| **System.Text.Json is NOT a drop-in replacement.** These behaviors differ by default: | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| | Behavior | Newtonsoft.Json | System.Text.Json | Impact | | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. STJ also escapes non-ASCII characters by default. Should recommend using relaxed encoder |
||||||||||||||||||||||||||
| |----------|----------------|-------------------|--------| | ||||||||||||||||||||||||||
| | **Property naming** | PascalCase by default (as declared) | **PascalCase by default** | Same ✓ (unless you used a custom ContractResolver) | | ||||||||||||||||||||||||||
| | **Missing properties** | Ignored silently | Ignored silently | Same ✓ | | ||||||||||||||||||||||||||
| | **Extra JSON properties** | Ignored by default | Ignored by default (can opt-in to throw in .NET 8+) | Same ✓ (stricter behavior available via options) | | ||||||||||||||||||||||||||
| | **Trailing commas** | Allowed | **Rejected by default** | Parse errors on valid-looking JSON | | ||||||||||||||||||||||||||
| | **Comments in JSON** | Allowed | **Rejected by default** | Config files break | | ||||||||||||||||||||||||||
| | **Number in string** (`"123"`) | Coerced automatically | **Throws by default** | Deserialization breaks! | | ||||||||||||||||||||||||||
| | **Enum serialization** | Numeric by default | Numeric by default | Same ✓, but converter syntax differs | | ||||||||||||||||||||||||||
| | **null → non-nullable value type** | Sets to default(T) | Sets to default(T) | Same ✓ (null becomes default(T)) | | ||||||||||||||||||||||||||
| | **Case sensitivity** | Case-insensitive | **Case-sensitive by default** | Property matching breaks | | ||||||||||||||||||||||||||
| | **Max depth** | 64 | 64 | Same ✓ | | ||||||||||||||||||||||||||
| | **Circular references** | `$ref/$id` with PreserveReferencesHandling | `ReferenceHandler.Preserve` (.NET 5+) | API differs | | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Step 2: Configure System.Text.Json to match Newtonsoft.Json behavior | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ```csharp | ||||||||||||||||||||||||||
| // In Program.cs (ASP.NET Core) — configure globally | ||||||||||||||||||||||||||
| builder.Services.ConfigureHttpJsonOptions(options => | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| ConfigureJsonOptions(options.SerializerOptions); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Also configure for controllers if using MVC | ||||||||||||||||||||||||||
| builder.Services.AddControllers() | ||||||||||||||||||||||||||
| .AddJsonOptions(options => | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| ConfigureJsonOptions(options.JsonSerializerOptions); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| static void ConfigureJsonOptions(JsonSerializerOptions options) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| // Match Newtonsoft.Json default behavior: | ||||||||||||||||||||||||||
| options.PropertyNamingPolicy = JsonNamingPolicy.CamelCase; // Newtonsoft default | ||||||||||||||||||||||||||
| options.PropertyNameCaseInsensitive = true; // Newtonsoft default | ||||||||||||||||||||||||||
|
Comment on lines
+64
to
+68
|
||||||||||||||||||||||||||
| options.NumberHandling = JsonNumberHandling.AllowReadingFromString; // Newtonsoft coerces | ||||||||||||||||||||||||||
| options.ReadCommentHandling = JsonCommentHandling.Skip; // Newtonsoft allows | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, this is a potentially dangerous setting and should not be encouraged unless the application has thought through the consequences. We chose not to allow this by default in S.T.J because it could lead to desynced deserialization attacks. In these attacks, the frontend and the backend disagree on the contents of the payload. This could be because they disagree on where comments start or end, potentially allowing sensitive values to be smuggled through what appears to be an ignorable comment. (For example, Newtonsoft.Json, JSON5, and S.T.J all have different concepts on what "end of line" means for a single-line comment.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To defend against this attack, the application should:
|
||||||||||||||||||||||||||
| options.AllowTrailingCommas = true; // Newtonsoft allows | ||||||||||||||||||||||||||
| options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull; // Common Newtonsoft setting | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Comment on lines
+72
to
+73
|
||||||||||||||||||||||||||
| options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull; // Common Newtonsoft setting | |
| // Additional commonly used Newtonsoft.Json-like settings (not defaults): | |
| options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull; // Common Newtonsoft setting (NullValueHandling.Ignore) |
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.
Setting Preserve significantly increases the attack surface of JSON deserialization. Most call sites do not know how to properly reason about the increased attack surface and could find themselves in a vulnerable state. This is why S.T.J does not support $ref / $id in its default configuration.
It's fine if people want to set this, but they should do it only if they truly need it and have thought through the consequences of setting it. It shouldn't be promoted as a "try it and see if it solves your problem" mechanism. (What happens in practice is that somebody will set it, find that it doesn't solve their problem, then move on to some other diagnostic step without reverting this change.)
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.
To defend against this attack, the application should consider what happens if the adversary controls the edges in the object graph, not just the values of the nodes in the graph. For example, if you're deserializing a graph that represents a family tree, what happens if a node sets itself as its own parent? What happens if a node has two children, but both point to the same object in memory? (This last example isn't recursive; it's an example of a graph that has a many:1 relationship between edges and nodes.)
Both examples could violate business logic that the application otherwise tries to uphold. Allowing an untrusted client to control the edges in the graph represents a broader attack surface and involves more subtle reasoning compared to an untrusted client that can only control values within the graph.
Copilot
AI
Mar 4, 2026
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.
[JsonExtensionData] doesn't have to be Dictionary<string, JsonElement> only—System.Text.Json also supports extension data on other shapes (e.g., IDictionary<string, object> and, in newer TFMs, JsonNode/JsonObject variants). If the intent is to recommend JsonElement as the closest replacement for JToken, consider rephrasing this as guidance rather than a hard requirement so the doc stays technically correct.
| | `[JsonExtensionData]` | `[JsonExtensionData]` + must be `Dictionary<string, JsonElement>` (NOT `JToken`) | | |
| | `[JsonExtensionData]` | `[JsonExtensionData]` + typically use `Dictionary<string, JsonElement>` as closest to `JToken` (other supported shapes are also valid) | |
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.
I think just always recommend JsonNode and friends. It's closer than JsonDocument/JsonElement.
Copilot
AI
Mar 4, 2026
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.
This example configures JsonSerializerSettings with TypeNameHandling = TypeNameHandling.Auto, which is a known unsafe pattern when used with untrusted JSON because it lets an attacker control the concrete .NET type to instantiate, enabling gadget-based code execution or data tampering. Even though it is labeled as a security risk, including this as compilable example code may lead readers to copy it into production; consider replacing it with a non-usable anti-pattern illustration or a safer configuration that avoids TypeNameHandling for untrusted data.
| var settings = new JsonSerializerSettings | |
| { | |
| TypeNameHandling = TypeNameHandling.Auto // SECURITY RISK! | |
| }; | |
| // ❌ Insecure pattern shown for illustration only — DO NOT COPY THIS INTO PRODUCTION CODE. | |
| // The following configuration enables $type-based polymorphic deserialization, which is | |
| // vulnerable when used with untrusted JSON input: | |
| // | |
| // var settings = new JsonSerializerSettings | |
| // { | |
| // TypeNameHandling = TypeNameHandling.Auto | |
| // }; |
Copilot
AI
Mar 4, 2026
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.
The polymorphism section implies System.Text.Json will automatically emit a discriminator for [JsonDerivedType] types. STJ polymorphism requires explicit opt-in/configuration (e.g., JsonPolymorphic / JsonPolymorphismOptions / type-info resolver setup). Consider updating this snippet to show the required opt-in so readers don't assume the attributes alone are sufficient.
Copilot
AI
Mar 4, 2026
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.
This repeats the earlier claim that JsonExtensionData "must" use Dictionary<string, JsonElement> and that Dictionary<string, object> is invalid. System.Text.Json supports extension data on IDictionary<string, object> as well; if the goal is to prevent accidentally keeping JToken, rephrase this pitfall to focus on avoiding Newtonsoft types and explain the tradeoffs between object and JsonElement/JsonNode.
| | `JsonExtensionData` with `Dictionary<string, object>` | Must be `Dictionary<string, JsonElement>` — not `object` or `JToken` | | |
| | `JsonExtensionData` containing `JToken`/Newtonsoft types | Avoid `JToken`/`JObject` in extension data; use System.Text.Json types (`Dictionary<string, JsonElement>`/`JsonNode`) or `Dictionary<string, object>` with only CLR values | |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||
| scenarios: | ||||||
| - name: "Migrate model with Newtonsoft.Json attributes to System.Text.Json" | ||||||
| prompt: | | ||||||
| I'm migrating our ASP.NET Core 8 project from Newtonsoft.Json to System.Text.Json. Here's a model class that uses Newtonsoft attributes and a custom converter. Convert this to System.Text.Json: | ||||||
|
|
||||||
|
Comment on lines
+1
to
+5
|
||||||
| ```csharp | ||||||
| using Newtonsoft.Json; | ||||||
| using Newtonsoft.Json.Converters; | ||||||
| using Newtonsoft.Json.Linq; | ||||||
|
|
||||||
| public class Order | ||||||
| { | ||||||
| [JsonProperty("order_id")] | ||||||
| public int Id { get; set; } | ||||||
|
|
||||||
| [JsonProperty(Required = Required.Always)] | ||||||
| public string CustomerName { get; set; } | ||||||
|
|
||||||
| [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] | ||||||
| public string? Notes { get; set; } | ||||||
|
|
||||||
| [JsonConverter(typeof(StringEnumConverter))] | ||||||
| public OrderStatus Status { get; set; } | ||||||
|
|
||||||
| [JsonExtensionData] | ||||||
| public Dictionary<string, JToken>? AdditionalData { get; set; } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| Also show me how to configure the JSON options globally to match Newtonsoft.Json's default behavior (case insensitivity, trailing commas, number-from-string coercion). | ||||||
| assertions: | ||||||
| - type: "output_contains" | ||||||
| value: "JsonPropertyName" | ||||||
| - type: "output_matches" | ||||||
| pattern: "(JsonIgnore.*WhenWritingNull|JsonIgnoreCondition)" | ||||||
| - type: "output_matches" | ||||||
| pattern: "(PropertyNameCaseInsensitive|CamelCase|PropertyNamingPolicy)" | ||||||
| - type: "output_matches" | ||||||
| pattern: "(JsonElement|JsonNode)" | ||||||
| rubric: | ||||||
| - "Replaced [JsonProperty(\"order_id\")] with [JsonPropertyName(\"order_id\")]" | ||||||
| - "Replaced NullValueHandling.Ignore with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]" | ||||||
| - "Replaced [JsonConverter(typeof(StringEnumConverter))] with System.Text.Json equivalent (JsonStringEnumConverter)" | ||||||
| - "Changed [JsonExtensionData] Dictionary value type from JToken to JsonElement (critical difference!)" | ||||||
| - "Configured PropertyNameCaseInsensitive = true to match Newtonsoft default case-insensitive behavior" | ||||||
| - "Configured AllowTrailingCommas = true and NumberHandling = AllowReadingFromString for Newtonsoft compatibility" | ||||||
| - "Warned about behavioral differences (default PascalCase casing in STJ vs camelCase in Newtonsoft, strict parsing)" | ||||||
|
||||||
| - "Warned about behavioral differences (default PascalCase casing in STJ vs camelCase in Newtonsoft, strict parsing)" | |
| - "Warned about behavioral differences (for example, stricter parsing behavior in System.Text.Json compared to Newtonsoft.Json)" |
Copilot
AI
Mar 4, 2026
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.
expect_tools: ["bash"] forces the model to make at least one bash tool call or the scenario fails. This prompt is primarily a code-migration explanation and doesn't require shell usage, so this constraint may introduce unnecessary flakiness. Consider removing expect_tools (or only using it when the scenario includes a setup/fixture that actually needs bash).
| expect_tools: ["bash"] |
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.
This SKILL.md is wrapped in a fenced code block (
skill ...), which means the YAML frontmatter and the rest of the content won't be parsed/rendered like other skills in this repo. Existing skills start with raw YAML frontmatter at the top of the file (no code fence). Remove the surrounding code fence so the file begins with---frontmatter directly.