Skip to content
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

Fix PNPM Dependency Parsing Error by Prioritizing Main Dependencies #11291

Merged
merged 4 commits into from
Jan 13, 2025

Conversation

kbukum1
Copy link
Contributor

@kbukum1 kbukum1 commented Jan 13, 2025

What are you trying to accomplish?

This change resolves an issue in PNPM dependency parsing where main dependencies with specifiers were not prioritized, leading to errors or unexpected behavior. By prioritizing main dependencies with specifiers in the lock file, the parser ensures that the main dependency node is retained, while transitive dependencies are grouped under all_versions. This directs the update process to correctly assess the vulnerability status of the main dependency.

The behavior is controlled by the feature flag enable_fix_for_pnpm_no_change_error, which ensures the changes are incremental and non-disruptive for other use cases.

What issues does this affect or fix?

This addresses the "no change" error in PNPM dependency processing, which occurred because main dependencies with specifiers were not prioritized, causing the update process to fail in properly identifying vulnerabilities.

Anything you want to highlight for special attention from reviewers?

The fix adjusts the processing order to prioritize main dependencies with specifiers over others. This approach ensures that:

  1. The main dependency is treated as the primary node.
  2. Transitive dependencies are properly managed under all_versions.

The use of the enable_fix_for_pnpm_no_change_error feature flag ensures that this fix is applied only when the flag is enabled, allowing for safe rollout and evaluation of the changes.

How will you know you've accomplished your goal?

  • The "no change" error in PNPM dependency parsing no longer occurs after this fix.
  • Main dependencies with specifiers are prioritized and processed correctly.
  • Relevant tests, including new ones for validating the prioritization of main dependencies, pass successfully.
  • Other package managers and dependency behaviors remain unaffected.
  • The feature flag can be toggled to verify the behavior change without affecting existing functionality.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

dependency_set << Dependency.new(**dependency_args)
end

dependencies_without_specifiers.each do |dependency_args|
dependency_set << Dependency.new(**dependency_args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip: Instead of processing dependencies without prioritization, we now prioritize main dependencies that include specifiers in the lock file. This ensures that the main dependency node is preserved, while all other versions (transitive dependencies) are grouped under all_versions. By doing this, we direct the update process to specifically check if the main dependency is vulnerable, ensuring a more accurate vulnerability assessment.

@kbukum1 kbukum1 force-pushed the kamil/fix-pnpm-no-change-error branch from 76e1d81 to 7b8c2d3 Compare January 13, 2025 21:46
[{ production: !details["dev"] }]
# Add metadata for subdependencies if marked as a dev dependency.
dependency_args[:subdependency_metadata] = [{ production: !details["dev"] }] if details["dev"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip:The above code is just refined.

@kbukum1 kbukum1 marked this pull request as ready for review January 13, 2025 21:54
@kbukum1 kbukum1 requested a review from a team as a code owner January 13, 2025 21:54
if Dependabot::Experiments.enabled?(:enable_fix_for_pnpm_no_change_error)
return dependencies_with_prioritization
end

dependency_set = Dependabot::FileParsers::Base::DependencySet.new
Copy link
Contributor Author

@kbukum1 kbukum1 Jan 13, 2025

Choose a reason for hiding this comment

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

Review Tip: Both methods return dependencies in a similar manner. However, when the feature flag is enabled, the dependencies_with_prioritization method prioritizes main dependencies over transitive dependencies with the same name but different versions.

@kbukum1 kbukum1 merged commit c4fa116 into main Jan 13, 2025
70 checks passed
@kbukum1 kbukum1 deleted the kamil/fix-pnpm-no-change-error branch January 13, 2025 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants