Migrate simple JSON serialization from Newtonsoft.Json to System.Text.Json #1566
Conversation
….Json Convert `FileWritingService`, `ScanCommand`, Mariner2ArtifactFilter, and `DetectorProcessingService` to use `System.Text.Json` for serialization. `LinuxScanner` retained on Newtonsoft for now due to `SyftOutput` union types requiring custom converters.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1566 +/- ##
=======================================
- Coverage 90.1% 90.1% -0.1%
=======================================
Files 426 426
Lines 35981 35975 -6
Branches 2221 2221
=======================================
- Hits 32442 32435 -7
- Misses 3079 3080 +1
Partials 460 460 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates JSON serialization from Newtonsoft.Json to System.Text.Json for four files: FileWritingService, ScanCommand, Mariner2ArtifactFilter, and DetectorProcessingService. The migration focuses on straightforward serialization scenarios, while complex use cases (like LinuxScanner with union types) remain on Newtonsoft.Json. The changes maintain backward compatibility by keeping model classes dual-annotated with both Newtonsoft.Json and System.Text.Json attributes during the transition.
Key Changes
- Replaced direct
JsonSerializerandJsonTextWriterusage withSystem.Text.Json.JsonSerializerstatic methods - Added
JsonSerializerOptionsconfiguration withJavaScriptEncoder.UnsafeRelaxedJsonEscapingandWriteIndentedsettings inFileWritingService - Updated imports: removed
using Newtonsoft.Json;and addedusing System.Text.Json;(andSystem.Text.Encodings.Webwhere needed)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.ComponentDetection.Orchestrator/Services/DetectorProcessingService.cs | Updated telemetry serialization to use System.Text.Json |
| src/Microsoft.ComponentDetection.Orchestrator/Commands/ScanCommand.cs | Migrated console output serialization to System.Text.Json with indented formatting |
| src/Microsoft.ComponentDetection.Detectors/linux/Filters/Mariner2ArtifactFilter.cs | Updated telemetry record serialization to System.Text.Json |
| src/Microsoft.ComponentDetection.Common/FileWritingService.cs | Refactored file writing methods to use System.Text.Json with custom serialization options including relaxed JSON escaping |
| public sealed class ScanCommand : AsyncCommand<ScanSettings> | ||
| { | ||
| private const string ManifestRelativePath = "ScanManifest_{timestamp}.json"; | ||
| private static readonly JsonSerializerOptions IndentedJsonOptions = new() { WriteIndented = true }; |
There was a problem hiding this comment.
The IndentedJsonOptions used for console output (line 94) doesn't include JavaScriptEncoder.UnsafeRelaxedJsonEscaping, while the FileWritingService uses it (FileWritingService.cs:24). This creates inconsistent JSON formatting between console and file output - the console output will have more aggressively escaped characters (e.g., + becomes \u002B). Consider adding Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping to maintain consistency with the file output format.
| containerDetails = result.ContainerDetails; | ||
|
|
||
| record.AdditionalTelemetryDetails = result.AdditionalTelemetryDetails != null ? JsonConvert.SerializeObject(result.AdditionalTelemetryDetails) : null; | ||
| record.AdditionalTelemetryDetails = result.AdditionalTelemetryDetails != null ? JsonSerializer.Serialize(result.AdditionalTelemetryDetails) : null; |
There was a problem hiding this comment.
The JsonSerializer.Serialize call doesn't specify any JsonSerializerOptions, which means it uses default serialization settings. This will produce more aggressively escaped JSON compared to Newtonsoft.Json's default behavior. For consistency with other parts of the codebase (e.g., FileWritingService), consider adding a static JsonSerializerOptions field with Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping to maintain similar output formatting to the previous Newtonsoft.Json implementation.
JamieMagee
force-pushed
the
users/jamagee/migrate-simple-json-to-stj
branch
from
58d9a84 to
9fd0bfc
Compare
| [System.Text.Json.Serialization.JsonIgnore] // System.Text.Json - type is handled by [JsonPolymorphic] discriminator | ||
| public abstract ComponentType Type { get; } | ||
|
|
||
| /// <summary>Gets the id of the component.</summary> |
There was a problem hiding this comment.
The Id property is missing the System.Text.Json attribute [JsonPropertyName(\"id\")]. While the Newtonsoft.Json attribute is present, the corresponding System.Text.Json attribute is required for serialization to work correctly with System.Text.Json. This will cause the Id property to not be serialized when using System.Text.Json. Add [JsonPropertyName(\"id\")] on line 57 before the existing Newtonsoft.Json attribute, following the dual-attribute pattern used elsewhere in the codebase.
| /// <summary>Gets the id of the component.</summary> | |
| /// <summary>Gets the id of the component.</summary> | |
| [JsonPropertyName("id")] |
JamieMagee
force-pushed
the
users/jamagee/migrate-simple-json-to-stj
branch
2 times, most recently
from
868c94e to
dc179eb
Compare
JamieMagee
force-pushed
the
users/jamagee/migrate-simple-json-to-stj
branch
from
dc179eb to
788c53e
Compare
Convert
FileWritingService,ScanCommand,Mariner2ArtifactFilter, andDetectorProcessingServiceto useSystem.Text.Jsonfor serialization.LinuxScannerretained on Newtonsoft for now due toSyftOutputunion types requiring custom converters.