Skip to content

Conversation

@JamieMagee
Copy link
Member

Replace Newtonsoft.Json usage in npm detector classes with System.Text.Json, continuing the project-wide migration effort #231

Changes:

  • Add PackageJson model and related types for deserializing package.json files
  • Add custom JsonConverters for polymorphic fields (author, workspaces, engines)
  • Refactor NpmComponentUtilities to use typed parameters instead of JProperty
  • Refactor NpmLockfileDetectorBase to use JsonDocument for lockfile parsing
  • Update NpmComponentDetector to use PackageJson model
  • Update NpmComponentDetectorWithRoots to use PackageLockV1Dependency model
  • Update NpmLockfile3Detector to use PackageLockV3Package model
  • Update NpmUtilitiesTests to use new API signatures

The existing PackageLock contract models (V1, V2, V3) already used System.Text.Json, so this change leverages those models and adds the missing PackageJson model.

JsonSerializerOptions configured with AllowTrailingCommas=true to handle npm's JSON format which sometimes includes trailing commas.

@JamieMagee JamieMagee requested a review from a team as a code owner November 26, 2025 05:27
@JamieMagee JamieMagee requested review from grvillic and removed request for a team November 26, 2025 05:27
@JamieMagee JamieMagee force-pushed the users/jamagee/npm-system-text-json branch from 024756a to a099889 Compare November 26, 2025 05:28
@github-actions
Copy link

github-actions bot commented Nov 26, 2025

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

@JamieMagee JamieMagee force-pushed the users/jamagee/spdx-system-text-json branch from 56a1717 to b2d4f8e Compare December 1, 2025 15:45
@JamieMagee JamieMagee force-pushed the users/jamagee/npm-system-text-json branch from a099889 to 966e66d Compare December 1, 2025 15:46
@JamieMagee JamieMagee force-pushed the users/jamagee/spdx-system-text-json branch 2 times, most recently from ec4fb32 to 1bfb72a Compare December 1, 2025 18:46
Base automatically changed from users/jamagee/spdx-system-text-json to main December 1, 2025 19:46
@JamieMagee JamieMagee force-pushed the users/jamagee/npm-system-text-json branch from 966e66d to 57b8ce0 Compare December 1, 2025 19:51
Copilot AI review requested due to automatic review settings December 1, 2025 19:51
Copy link

Copilot AI left a 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 completes the migration of npm detectors from Newtonsoft.Json to System.Text.Json as part of project-wide migration effort #231. The changes introduce strongly-typed models for package.json deserialization and refactor all npm detector classes to use System.Text.Json APIs.

Key changes:

  • Introduced PackageJson model with custom JSON converters for polymorphic fields (author, workspaces, engines)
  • Refactored all npm detector base classes and implementations to use typed models instead of JToken/JProperty
  • Updated utility methods to accept typed parameters instead of Newtonsoft.Json types
  • Configured JsonSerializerOptions with AllowTrailingCommas=true for npm JSON compatibility

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
test/Microsoft.ComponentDetection.Orchestrator.Tests/Experiments/LinuxApplicationLayerExperimentTests.cs Added #nullable disable directive for consistency with existing test patterns
test/Microsoft.ComponentDetection.Detectors.Tests/NpmUtilitiesTests.cs Updated tests to use new typed API signatures; removed Newtonsoft.Json imports and JProperty-based test data
src/Microsoft.ComponentDetection.Detectors/npm/NpmLockfileDetectorBase.cs Refactored to use JsonDocument for lockfile parsing; introduced ProcessLockfile abstract method replacing JToken-based processing
src/Microsoft.ComponentDetection.Detectors/npm/NpmLockfile3Detector.cs Implemented new ProcessLockfile method using PackageLockV3Package models; replaced JProperty traversal with typed dictionary lookups
src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentUtilities.cs Updated GetTypedComponent to accept typed parameters; refactored TryGetAllPackageJsonDependencies to use PackageJson model
src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetectorWithRoots.cs Implemented new ProcessLockfile method using PackageLockV1Dependency models; replaced JProperty-based dependency traversal
src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs Refactored to deserialize PackageJson directly; simplified author and engines handling using typed properties
src/Microsoft.ComponentDetection.Detectors/npm/Contracts/PackageJsonWorkspacesConverter.cs New custom converter handling workspaces as array or object with packages field
src/Microsoft.ComponentDetection.Detectors/npm/Contracts/PackageJsonEnginesConverter.cs New custom converter handling engines as object or array (for malformed files)
src/Microsoft.ComponentDetection.Detectors/npm/Contracts/PackageJsonAuthorConverter.cs New custom converter parsing author as string (with regex) or object
src/Microsoft.ComponentDetection.Detectors/npm/Contracts/PackageJsonAuthor.cs New model representing package.json author field
src/Microsoft.ComponentDetection.Detectors/npm/Contracts/PackageJson.cs New model representing package.json structure with all relevant fields
Comments suppressed due to low confidence (1)

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentUtilities.cs:76

  • These 'if' statements can be combined.
        if (versionString is not null && Uri.TryCreate(versionString, UriKind.Absolute, out var parsedUri))
        {
            if (string.Equals(npmRegistryHost, parsedUri.Host, StringComparison.OrdinalIgnoreCase))
            {
                return TryParseNpmRegistryVersion(packageName, parsedUri, out version);
            }
        }

Copilot AI review requested due to automatic review settings December 1, 2025 21:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentUtilities.cs:76

  • These 'if' statements can be combined.
        if (versionString is not null && Uri.TryCreate(versionString, UriKind.Absolute, out var parsedUri))
        {
            if (string.Equals(npmRegistryHost, parsedUri.Host, StringComparison.OrdinalIgnoreCase))
            {
                return TryParseNpmRegistryVersion(packageName, parsedUri, out version);
            }
        }

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 92.29911% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.0%. Comparing base (8769878) to head (2b1ac67).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...Detection.Detectors/npm/NpmLockfileDetectorBase.cs 76.1% 11 Missing and 5 partials ⚠️
...ctors/npm/Contracts/PackageJsonEnginesConverter.cs 80.7% 5 Missing and 6 partials ⚠️
...rs/npm/Contracts/PackageJsonWorkspacesConverter.cs 84.6% 5 Missing and 5 partials ⚠️
...entDetection.Detectors/npm/NpmComponentDetector.cs 62.9% 8 Missing and 2 partials ⚠️
...ectors/npm/Contracts/PackageJsonAuthorConverter.cs 82.0% 5 Missing and 2 partials ⚠️
...entDetection.Detectors/npm/NpmLockfile3Detector.cs 92.9% 2 Missing and 3 partials ⚠️
...ntDetection.Detectors/npm/NpmComponentUtilities.cs 89.4% 2 Missing and 2 partials ⚠️
...ion.Detectors/npm/NpmComponentDetectorWithRoots.cs 94.6% 1 Missing and 2 partials ⚠️
...ntDetection.Detectors/npm/Contracts/PackageJson.cs 91.6% 1 Missing ⚠️
...ction.Detectors/npm/Contracts/PackageJsonAuthor.cs 75.0% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #1572    +/-   ##
======================================
  Coverage   89.9%   90.0%            
======================================
  Files        426     435     +9     
  Lines      36540   37057   +517     
  Branches    2273    2298    +25     
======================================
+ Hits       32877   33376   +499     
- Misses      3202    3209     +7     
- Partials     461     472    +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings December 2, 2025 15:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentUtilities.cs:76

  • These 'if' statements can be combined.
        if (versionString is not null && Uri.TryCreate(versionString, UriKind.Absolute, out var parsedUri))
        {
            if (string.Equals(npmRegistryHost, parsedUri.Host, StringComparison.OrdinalIgnoreCase))
            {
                return TryParseNpmRegistryVersion(packageName, parsedUri, out version);
            }
        }

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

@JamieMagee JamieMagee enabled auto-merge (squash) December 2, 2025 15:50
Copy link
Member

@jpinz jpinz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits. Also, I recently ran into an issue on dependabot-core that has to do with pnpm lockfiles, I don't think I saw anything here for pnpm, only npm and yarn. Obviously not a requirement for this PR just wanted to have it down somewhere.

Replace `Newtonsoft.Json` usage in npm detector classes with `System.Text.Json`, continuing the project-wide migration effort.

Changes:
- Add `PackageJson` model and related types for deserializing `package.json` files
- Add custom `JsonConverters` for polymorphic fields (`author`, `workspaces`, `engines`)
- Refactor `NpmComponentUtilities` to use typed parameters instead of `JProperty`
- Refactor `NpmLockfileDetectorBase` to use `JsonDocument` for lockfile parsing
- Update `NpmComponentDetector` to use `PackageJson` model
- Update `NpmComponentDetectorWithRoots` to use `PackageLockV1Dependency` model
- Update `NpmLockfile3Detector` to use `PackageLockV3Package` model
- Update `NpmUtilitiesTests` to use new API signatures

The existing `PackageLock` contract models (V1, V2, V3) already used `System.Text.Json`, so this change leverages those models and adds the missing `PackageJson` model.

`JsonSerializerOptions` configured with `AllowTrailingCommas=true` to handle npm's JSON format which sometimes includes trailing commas.
Move closing brace for `devDependencies` inside the if block in `CreatePackageJsonFileContent` to avoid generating invalid JSON when there are no dev dependencies. `System.Text.Json` is stricter
than `Newtonsoft.Json` and throws on trailing characters.
Add named capture group for URL in the author regex pattern and extract it in the converter. Previously, URLs in strings like "John Doe <email> (https://example.com)" were matched but not assigned to the Url property.
Flatten nested if statements into a single guard clause for improved readability.
Copilot AI review requested due to automatic review settings December 3, 2025 14:45
@JamieMagee JamieMagee force-pushed the users/jamagee/npm-system-text-json branch from 7492749 to 2b1ac67 Compare December 3, 2025 14:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

@JamieMagee JamieMagee merged commit 0ab7ee0 into main Dec 3, 2025
34 checks passed
@JamieMagee JamieMagee deleted the users/jamagee/npm-system-text-json branch December 3, 2025 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants