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 npm_and_yarn engine detection #11392

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

ntkme
Copy link

@ntkme ntkme commented Jan 23, 2025

What are you trying to accomplish?

Detect npm engine version is broken without a lockfile even if the version is specified in package.json in some cases:

  • { "packageManager": "[email protected]" } works
  • { "engines": { "npm": "10.0.0" } } works
  • { "engines": { "npm": ">=10" } } is broken with message No version requirement found for npm

Anything you want to highlight for special attention from reviewers?

The root cause is that while trying to setup the engine, delete_if in ruby was used. delete_if modifies the input array in place, and effectively removes entries from @engines unless they match a very specific format.

The side effect of removing valid entries from @engines breaks the version detection from engines at a later time.

How will you know you've accomplished your goal?

I don't know for sure as I don't know how to test this end to end. I just manually walked through the code and found that the side effect seems to be the problem here.

detected_version = detect_version(name)
# if we have a detected version, we check if it is deprecated or unsupported
if detected_version
package_manager = package_manager_class.new(
detected_version: detected_version.to_s
)
return package_manager if package_manager.deprecated? || package_manager.unsupported?
end
installed_version = installed_version(name)
Dependabot.logger.info("Installed version for #{name}: #{installed_version}")
package_manager_requirement = find_engine_constraints_as_requirement(name)

At line 312, the call to detect_version calls VersionSelector and removes engines entries not matching a specific format due to side effect using delete_if:

 detected_version = detect_version(name)

At line 325, because the engines entries got removed, it fails to detect the constraints:

 package_manager_requirement = find_engine_constraints_as_requirement(name) 

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.

@ntkme ntkme requested a review from a team as a code owner January 23, 2025 16:48
@ntkme
Copy link
Author

ntkme commented Jan 23, 2025

cc @kbukum1 @sachin-sandhu

@abdulapopoola
Copy link
Member

Thanks @ntkme ! Can you please add a test please?

@ntkme ntkme force-pushed the fix-npm branch 2 times, most recently from 424692b to b1b823e Compare January 23, 2025 20:16
@ntkme
Copy link
Author

ntkme commented Jan 23, 2025

@abdulapopoola Test added.

For reference, the added test fails on the main branch with:

  1) Dependabot::NpmAndYarn::PackageManagerHelper#find_engine_constraints_as_requirement when the engines field contains valid constraints when package manager lockfile does not exist returns a requirement for npm with the correct constraints
     Failure/Error: expect(requirement).to be_a(Dependabot::NpmAndYarn::Requirement)
       expected nil to be a kind of Dependabot::NpmAndYarn::Requirement

It passes with the fix in this PR.

@ntkme ntkme mentioned this pull request Jan 23, 2025
1 task
@ntkme ntkme force-pushed the fix-npm branch 2 times, most recently from 7f1f8ae to 1d1c8b2 Compare January 23, 2025 20:55
@kbukum1
Copy link
Contributor

kbukum1 commented Jan 23, 2025

@ntkme ,

Sorry for let reply. Checking it now.

@kbukum1 kbukum1 self-assigned this Jan 23, 2025
@kbukum1
Copy link
Contributor

kbukum1 commented Jan 23, 2025

Thank you, @ntkme, for identifying the issue and proposing a solution. I’ll be making further updates to enhance the stability of the solution. Once it’s ready, I’ll let you know.

CC: @abdulapopoola

@jkowalleck
Copy link

jkowalleck commented Jan 25, 2025

{ "engines": { "npm": ">=10" } } is broken with message No version requirement found for npm

does this PR also fix the following cases?

  • { "engines": { "npm": "^7" } } -- not tested
  • { "engines": { "npm": "^6.2 || ^7" } } -- not tested
  • { "engines": { "npm": "6 - 11" } } -- causes Dependabot detected the following npm requirement for your project: ''.

@ntkme
Copy link
Author

ntkme commented Jan 26, 2025

@jkowalleck As you’ve noticed, there are some version range syntax that are currently not parsable, but supporting those additional syntax is out of my scope of this PR, which is just fixing the side effect of delete_if.

In my opinion additional range syntax can be a separate feature request, but I will leave it to @kbukum1 to decide what to do with other unsupported npm version range syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants