-
Notifications
You must be signed in to change notification settings - Fork 369
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
Closed
mrsharm
wants to merge
13
commits into
dotnet:main
from
mrsharm:musharm/migrating-newtonsoft-to-stj-skill
Closed
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
e47051f
Add migrating-newtonsoft-to-system-text-json skill (+13.8% eval impro…
mrsharm 26d849b
Remove negative scenario and expand rubric with AllowTrailingCommas, …
mrsharm 3f5f6f8
Update src/dotnet/skills/migrating-newtonsoft-to-system-text-json/SKI…
mrsharm eae7189
Update src/dotnet/skills/migrating-newtonsoft-to-system-text-json/SKI…
mrsharm 36b50cc
Update src/dotnet/skills/migrating-newtonsoft-to-system-text-json/SKI…
mrsharm 700e751
Migrate migrating-newtonsoft-to-system-text-json to plugins/ director…
mrsharm f9c0d31
Remove PR-FEEDBACK.md
mrsharm c316e8f
Merge branch 'main' into musharm/migrating-newtonsoft-to-stj-skill
1f79841
Address all PR feedback: security caveats, accuracy fixes, structural…
def169f
Merge branch 'main' into musharm/migrating-newtonsoft-to-stj-skill
b9e8364
Move skill from dotnet to dotnet-upgrade plugin
29075f6
Address PR feedback: security-first config, trim token overhead, add …
mrsharm ff2fb3e
Add migration review scenario, replace failing diagnosis scenario
mrsharm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
249 changes: 249 additions & 0 deletions
249
plugins/dotnet/skills/migrating-newtonsoft-to-system-text-json/SKILL.md
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
| 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. | ||
| --- | ||
|
danmoseley marked this conversation as resolved.
Outdated
danmoseley marked this conversation as resolved.
Outdated
|
||
|
|
||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| # 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 | | ||
| |----------|----------------|-------------------|--------| | ||
| | **Property naming** | PascalCase by default (as declared) | **PascalCase by default** | Same ✓ (unless you used a custom ContractResolver) | | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| | **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 | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| options.NumberHandling = JsonNumberHandling.AllowReadingFromString; // Newtonsoft coerces | ||
| options.ReadCommentHandling = JsonCommentHandling.Skip; // Newtonsoft allows | ||
| options.AllowTrailingCommas = true; // Newtonsoft allows | ||
| options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull; // Common Newtonsoft setting | ||
|
|
||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| // Enum string serialization (replaces StringEnumConverter) | ||
| options.Converters.Add(new JsonStringEnumConverter(JsonNamingPolicy.CamelCase)); | ||
|
|
||
| // Handle circular references (replaces PreserveReferencesHandling) | ||
| options.ReferenceHandler = ReferenceHandler.IgnoreCycles; // or Preserve for $ref/$id | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| } | ||
| ``` | ||
|
|
||
| ### Step 3: Replace attribute mappings | ||
|
|
||
| | Newtonsoft.Json Attribute | System.Text.Json Equivalent | | ||
| |--------------------------|----------------------------| | ||
| | `[JsonProperty("name")]` | `[JsonPropertyName("name")]` | | ||
| | `[JsonIgnore]` | `[JsonIgnore]` (same name, different namespace!) | | ||
| | `[JsonProperty(Required = Required.Always)]` | `[JsonRequired]` (.NET 7+) | | ||
| | `[JsonProperty(NullValueHandling = NullValueHandling.Ignore)]` | `[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]` | | ||
| | `[JsonProperty(DefaultValueHandling = DefaultValueHandling.Ignore)]` | `[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]` | | ||
| | `[JsonConverter(typeof(MyConverter))]` | `[JsonConverter(typeof(MyConverter))]` (different base class!) | | ||
| | `[JsonConstructor]` | `[JsonConstructor]` (same name, different namespace) | | ||
| | `[JsonExtensionData]` | `[JsonExtensionData]` + must be `Dictionary<string, JsonElement>` (NOT `JToken`) | | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
|
|
||
| **Regex for finding Newtonsoft attributes:** | ||
| ```bash | ||
| # Find all files using Newtonsoft attributes | ||
| grep -rn "using Newtonsoft.Json" --include="*.cs" | ||
| grep -rn "\[JsonProperty\|JsonConverter\|JsonIgnore\|JsonConstructor" --include="*.cs" | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| ``` | ||
|
|
||
| ### Step 4: Convert custom JsonConverters | ||
|
|
||
| **Newtonsoft converter pattern:** | ||
| ```csharp | ||
| // OLD: Newtonsoft.Json | ||
| public class UnixDateTimeConverter : Newtonsoft.Json.JsonConverter<DateTime> | ||
| { | ||
| public override DateTime ReadJson(JsonReader reader, Type objectType, | ||
| DateTime existingValue, bool hasExistingValue, JsonSerializer serializer) | ||
| { | ||
| var timestamp = (long)reader.Value!; | ||
| return DateTimeOffset.FromUnixTimeSeconds(timestamp).DateTime; | ||
| } | ||
|
|
||
| public override void WriteJson(JsonWriter writer, DateTime value, | ||
| JsonSerializer serializer) | ||
| { | ||
| var timestamp = new DateTimeOffset(value).ToUnixTimeSeconds(); | ||
| writer.WriteValue(timestamp); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **System.Text.Json converter pattern:** | ||
| ```csharp | ||
| // NEW: System.Text.Json | ||
| public class UnixDateTimeConverter : System.Text.Json.Serialization.JsonConverter<DateTime> | ||
| { | ||
| public override DateTime Read(ref Utf8JsonReader reader, Type typeToConvert, | ||
| JsonSerializerOptions options) | ||
| { | ||
| var timestamp = reader.GetInt64(); // Note: strongly typed reader methods | ||
| return DateTimeOffset.FromUnixTimeSeconds(timestamp).DateTime; | ||
| } | ||
|
|
||
| public override void Write(Utf8JsonWriter writer, DateTime value, | ||
| JsonSerializerOptions options) | ||
| { | ||
| var timestamp = new DateTimeOffset(value).ToUnixTimeSeconds(); | ||
| writer.WriteNumberValue(timestamp); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Key differences in converter API:** | ||
| - Reader is `ref Utf8JsonReader` (struct, passed by ref) — NOT a class | ||
| - Writer is `Utf8JsonWriter` — write methods are `WriteStringValue`, `WriteNumberValue`, `WriteBooleanValue` (typed) | ||
| - No `serializer` parameter — use `options` and call `JsonSerializer.Serialize/Deserialize` for nested objects | ||
| - For polymorphic deserialization: use `JsonTypeInfo` and `[JsonDerivedType]` (.NET 7+) instead of custom type handling | ||
|
|
||
| ### Step 5: Replace JToken/JObject/JArray with JsonDocument/JsonElement | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
|
|
||
| | Newtonsoft.Json | System.Text.Json | Notes | | ||
| |----------------|-------------------|-------| | ||
| | `JToken.Parse(json)` | `JsonDocument.Parse(json)` | **JsonDocument is IDisposable!** Must wrap in `using` | | ||
| | `JObject obj = ...` | `JsonElement obj = doc.RootElement` | JsonElement is a struct (no allocation) | | ||
| | `obj["key"]` | `obj.GetProperty("key")` | Throws if missing; use `TryGetProperty` for safe access | | ||
| | `obj["key"]?.Value<int>()` | `obj.GetProperty("key").GetInt32()` | Type-specific getters | | ||
| | `obj.Add("key", value)` | **Not possible** — JsonElement is read-only | Use `JsonNode` (System.Text.Json.Nodes) for mutable DOM | | ||
|
|
||
| **For mutable DOM operations, use JsonNode (NOT JsonDocument):** | ||
| ```csharp | ||
| // Mutable DOM — replaces JObject/JArray mutation patterns | ||
| var node = JsonNode.Parse(json)!; | ||
| node["newProperty"] = "value"; // Add/set properties | ||
| node["nested"] = new JsonObject // Create nested objects | ||
| { | ||
| ["key"] = 42 | ||
| }; | ||
| var result = node.ToJsonString(); // Serialize back | ||
| ``` | ||
|
|
||
| ### Step 6: Handle polymorphic serialization | ||
|
|
||
| **Newtonsoft.Json (uses $type discriminator):** | ||
| ```csharp | ||
| var settings = new JsonSerializerSettings | ||
| { | ||
| TypeNameHandling = TypeNameHandling.Auto // SECURITY RISK! | ||
| }; | ||
| ``` | ||
|
|
||
| **System.Text.Json (.NET 7+ — type discriminators):** | ||
| ```csharp | ||
| [JsonDerivedType(typeof(CreditCardPayment), typeDiscriminator: "credit")] | ||
| [JsonDerivedType(typeof(BankTransferPayment), typeDiscriminator: "bank")] | ||
| public abstract class Payment | ||
| { | ||
| public decimal Amount { get; set; } | ||
| } | ||
|
|
||
| public class CreditCardPayment : Payment | ||
| { | ||
| public string CardNumber { get; set; } = ""; | ||
| } | ||
|
|
||
| // Serializes as: {"$type":"credit","amount":99.99,"cardNumber":"..."} | ||
| // Note: System.Text.Json uses "$type" by default (configurable) | ||
| ``` | ||
|
|
||
| ### Step 7: Update package references | ||
|
|
||
| ```xml | ||
| <!-- Remove from .csproj --> | ||
| <PackageReference Include="Newtonsoft.Json" Version="*" /> | ||
| <PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="*" /> | ||
|
|
||
| <!-- System.Text.Json is included in the framework — no package needed for .NET 6+ --> | ||
| <!-- Only add explicitly if you need a newer version: --> | ||
| <!-- <PackageReference Include="System.Text.Json" Version="8.0.0" /> --> | ||
| ``` | ||
|
|
||
| **Update using statements:** | ||
| ```csharp | ||
| // Remove: | ||
| using Newtonsoft.Json; | ||
| using Newtonsoft.Json.Linq; | ||
| using Newtonsoft.Json.Serialization; | ||
| using Newtonsoft.Json.Converters; | ||
|
|
||
| // Add: | ||
| using System.Text.Json; | ||
| using System.Text.Json.Serialization; | ||
| using System.Text.Json.Nodes; // For JsonNode (mutable DOM) | ||
| ``` | ||
|
|
||
| ## Validation | ||
|
|
||
| - [ ] All `using Newtonsoft.Json` references removed | ||
| - [ ] All `[JsonProperty]` replaced with `[JsonPropertyName]` | ||
| - [ ] Custom converters use `System.Text.Json.Serialization.JsonConverter<T>` base | ||
| - [ ] `JObject`/`JToken` replaced with `JsonDocument` (read-only) or `JsonNode` (mutable) | ||
| - [ ] API responses match previous JSON format (property casing, null handling) | ||
| - [ ] Deserialization handles edge cases: trailing commas, comments, numbers-as-strings | ||
| - [ ] No `TypeNameHandling` equivalent (security improvement) | ||
| - [ ] `JsonDocument` usages wrapped in `using` statements | ||
|
|
||
| ## Common Pitfalls | ||
|
|
||
| | Pitfall | Solution | | ||
| |---------|----------| | ||
| | Forgetting `PropertyNameCaseInsensitive = true` | Deserialization silently returns default values for all properties | | ||
| | `JsonDocument` not disposed | Memory leak — always `using var doc = JsonDocument.Parse(...)` | | ||
| | Using `JsonElement` after `JsonDocument` is disposed | JsonElement is invalid after dispose; clone with `element.Clone()` if needed | | ||
| | `[JsonIgnore]` from wrong namespace | Both Newtonsoft and System.Text.Json have `[JsonIgnore]` — wrong `using` = attribute ignored | | ||
| | Custom converter reading past the current token | System.Text.Json reader is strict — must read exactly the right tokens | | ||
| | `JsonExtensionData` with `Dictionary<string, object>` | Must be `Dictionary<string, JsonElement>` — not `object` or `JToken` | | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| ``` | ||
49 changes: 49 additions & 0 deletions
49
tests/dotnet/migrating-newtonsoft-to-system-text-json/eval.yaml
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
| 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: | ||
|
|
||
| ```csharp | ||
| using Newtonsoft.Json; | ||
| using Newtonsoft.Json.Converters; | ||
| using Newtonsoft.Json.Linq; | ||
|
|
||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| 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)" | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| expect_tools: ["bash"] | ||
|
danmoseley marked this conversation as resolved.
Outdated
|
||
| timeout: 120 | ||
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.
Uh oh!
There was an error while loading. Please reload this page.