Add support for pnpm lock file format 6 #1028

CraigMacomber · 2024-03-12T18:24:25Z

This addresses #503 by doing the following:

Explicitly mark code specific to version 5 as "V5"
Add new code for V6.
Add version detection logic to select which code to run.
Add tests for new code.

The old code wasn't clear about what file formats it actually worked with, and a lot of its tests seems to be from versions earlier than 5 which it didn't really parse correctly (ex: the version it parses isn't present in those versions as it had a different name).
Thus to preserve existing behavior I pointed cases which failed to detect a version through the old V5 code.

The included verification tests didn't cover a lot of cases (link and file references, aliased packages, shared shrink-wrap format, disambiguated deps with peer deps) so I also tested on the package lock from https://github.com/microsoft/FluidFramework which is 44k lines and has all the mentioned edge cases.
If desired, I can add the v5 and v6 versions of that file to the verification tests directory.

This should be in a working state but I need to actually understand how the verification tests results are verifies before this should be merged.

This is my first time working in this codebase, as well as the first time in several years that I'm working in C#, so please be extra careful reviewing this: don't trust that I know what I'm doing.

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs

CraigMacomber · 2024-03-12T18:27:05Z

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs

@@ -30,7 +31,7 @@ public class PnpmComponentDetector : FileComponentDetector

    public override IEnumerable<ComponentType> SupportedComponentTypes { get; } = new[] { ComponentType.Npm };

-    public override int Version { get; } = 5;
+    public override int Version { get; } = 6;


I think I'm supposed to update this because I made changes, but I'd like confirmation that that's the correct thing to do.

When we introduce detectors which may affect the component graph being updated (new component type, new lockfile, etc) we like to introduce them as experimental detectors (see detector lifecycle).

If possible, would you be able to introduce a new detector for V6 so we are able to run the experiments? We can merge the code back into the main detection logic after. #684 has some example configuration.

I'd be happy to help make these changes as well if needed

That sounds like an excellent way to reduce risk in cases like this.

I'd be happy to help make these changes as well if needed

That would be great. I don't know this codebase well (I just spent a day and a half making this as part of a hackathon), so I suspect you could do that much more efficiently than I could. I'm happy to have you take over any part or all of this work.

codecov · 2024-03-12T18:28:31Z

Codecov Report

Attention: Patch coverage is 81.55340% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 75.4%. Comparing base (c06abda) to head (e8f2072).
Report is 21 commits behind head on main.

Files	Patch %	Lines
...tDetection.Detectors/pnpm/PnpmComponentDetector.cs	72.8%	11 Missing and 5 partials ⚠️
...ntDetection.Detectors/pnpm/PnpmParsingUtilities.cs	91.6%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #1028   +/-   ##
=====================================
  Coverage   75.3%   75.4%           
=====================================
  Files        237     241    +4     
  Lines      10423   10512   +89     
  Branches    1043    1054   +11     
=====================================
+ Hits        7858    7927   +69     
- Misses      2272    2286   +14     
- Partials     293     299    +6

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

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmParsingUtilities.cs

CraigMacomber · 2024-03-12T20:01:42Z

It appears that the VS code dev container setup doesn't include PowerShell so I can't run the VerificationTests.ps1 script locally. The instructions in there also make explicit reference to visual studio. It also says "The automation script above will output variables that needs to be replaced in ComponentDetectionIntegrationTests.cs file.": as far as I can tell thats not accurate as it looks like that file pulls in the data from environment variables.

Anyway its unclear to me what the workflow is for adding more test cases to the verification script. I'd like to include my added v6 file works but I don't know how.

CraigMacomber · 2024-03-12T20:52:33Z

Looking at the code coverage, I determined I could use a test for the shared-shrinkwrap case that workspaces use, and one for renamed/aliased dependencies, so I have added both. More testing is possible (ex: file and link cases, peer deps etc.) but I think I'm past the point of diminishing returns.

CraigMacomber · 2024-03-25T22:19:05Z

@microsoft/ose-component-detection-maintainers : I'd like assistance dealing with the snapshot tests as noted in #1028 (comment). If this is not practical to get assistance on, I can revert the extended snapshot test coverage which should get this passing on CI: maybe that would get this reviewed sooner?

cobya · 2024-03-28T15:58:07Z

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs

@@ -30,7 +31,7 @@ public class PnpmComponentDetector : FileComponentDetector

    public override IEnumerable<ComponentType> SupportedComponentTypes { get; } = new[] { ComponentType.Npm };

-    public override int Version { get; } = 5;
+    public override int Version { get; } = 6;


When we introduce detectors which may affect the component graph being updated (new component type, new lockfile, etc) we like to introduce them as experimental detectors (see detector lifecycle).

If possible, would you be able to introduce a new detector for V6 so we are able to run the experiments? We can merge the code back into the main detection logic after. #684 has some example configuration.

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs

cobya · 2024-05-02T19:02:45Z

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs

@@ -30,7 +31,7 @@ public class PnpmComponentDetector : FileComponentDetector

    public override IEnumerable<ComponentType> SupportedComponentTypes { get; } = new[] { ComponentType.Npm };

-    public override int Version { get; } = 5;
+    public override int Version { get; } = 6;


I'd be happy to help make these changes as well if needed

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs

cobya · 2024-05-02T19:04:18Z

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs

+            ProcessDependencySet(package);
+        }
+
+        void ProcessDependencySet(PnpmHasDependenciesV6 item)


nit: can these be declared as explicit functions rather than locals for RecordDependencyGraphFromFileV6?

I choose to do it this way so they could be scoped to v6 specific code, and not require as many parameters. They could of course be moved to a larger scope, given more specific names, and have everything passed in: that just seemed messier to me. If you refactor the v5 and v6 detector to be more separated, moving these to a larger scope would be more attractive.

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmParsingUtilities.cs

cobya · 2024-05-02T19:06:21Z

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmParsingUtilities.cs

+
+        // Strip parenthesized suffices from package. These hold peed dep related information that is unneeded here.
+        // An example of a dependency path with these: /webpack-cli@4.10.0(webpack-bundle-analyzer@4.10.1)(webpack-dev-server@4.6.0)(webpack@5.89.0)
+        var fullPackageNameAndVersion = pnpmDependencyPath.Split("(")[0];


is this guaranteed to always have [0]? do we need to do index checks beforehand?

My understanding is splitting a string always returns at least one part, even if there is no separator or the string is empty, so this should be fine. There likely is a more explicit way to do this (for example with a regular expression), but this should work fine.

cobya · 2024-05-02T19:08:53Z

It appears that the VS code dev container setup doesn't include PowerShell so I can't run the VerificationTests.ps1 script locally. The instructions in there also make explicit reference to visual studio. It also says "The automation script above will output variables that needs to be replaced in ComponentDetectionIntegrationTests.cs file.": as far as I can tell thats not accurate as it looks like that file pulls in the data from environment variables.

Anyway its unclear to me what the workflow is for adding more test cases to the verification script. I'd like to include my added v6 file works but I don't know how.

I'll follow up internally here as the verification CI tests use a private repo which we can add the additional test file to.

cobya · 2024-05-02T19:10:56Z

@microsoft/ose-component-detection-maintainers : I'd like assistance dealing with the snapshot tests as noted in #1028 (comment). If this is not practical to get assistance on, I can revert the extended snapshot test coverage which should get this passing on CI: maybe that would get this reviewed sooner?

When adding new components like this, the verification test failures are expected as it is performing a diff based on the last "known good version" so adding new files will trip the failure. I'll verify before merging but you shouldn't need to worry about this for now.

cobya · 2024-05-15T17:16:38Z

Closing this as #1110 has been opened with the related experiments changes.

CraigMacomber added 5 commits March 11, 2024 18:56

Split resources files by version

e68b8ba

Update v6 file to lockfileVersion: '6.1'

a112328

Add v6 support

7290ad3

Fix V6 detection

210291f

Better docs, fix tests

e688425

CraigMacomber commented Mar 12, 2024

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmComponentDetector.cs Show resolved Hide resolved

CraigMacomber commented Mar 12, 2024

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/pnpm/PnpmParsingUtilities.cs Show resolved Hide resolved

Tidy up diff, and add more tests

61a2d66

Fix test

d3989b5

CraigMacomber marked this pull request as ready for review March 12, 2024 20:27

CraigMacomber requested a review from a team as a code owner March 12, 2024 20:27

CraigMacomber requested a review from annaowens March 12, 2024 20:27

Add workspace and renamed dependency tests

a223ba0

Merge branch 'main' into pnpmv6

e8f2072

CraigMacomber mentioned this pull request Mar 25, 2024

Add support for pnpm lockfile v6 #503

Closed

cobya reviewed May 2, 2024

View reviewed changes

cobya added version:minor type:feature Feature (new functionality) detector:pnpm The pnpm detector labels May 2, 2024

cobya mentioned this pull request May 15, 2024

Add pnpm v6 support as an experimental detector #1110

Merged

cobya closed this May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pnpm lock file format 6 #1028

Add support for pnpm lock file format 6 #1028

CraigMacomber commented Mar 12, 2024 •

edited

Loading

CraigMacomber Mar 12, 2024

cobya Mar 28, 2024

cobya May 2, 2024

CraigMacomber May 2, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

CraigMacomber commented Mar 12, 2024

CraigMacomber commented Mar 12, 2024

CraigMacomber commented Mar 25, 2024

cobya Mar 28, 2024

cobya May 2, 2024

cobya May 2, 2024

CraigMacomber May 2, 2024

cobya May 2, 2024

CraigMacomber May 2, 2024

cobya commented May 2, 2024

cobya commented May 2, 2024

cobya commented May 15, 2024

Add support for pnpm lock file format 6 #1028

Add support for pnpm lock file format 6 #1028

Conversation

CraigMacomber commented Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

CraigMacomber commented Mar 12, 2024

CraigMacomber commented Mar 12, 2024

CraigMacomber commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cobya commented May 2, 2024

cobya commented May 2, 2024

cobya commented May 15, 2024

CraigMacomber commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading